content test: Deduplicate ad hoc content-preparing code

[chrisbobbe]

This PR makes it so all our tests of Zulip content widgets go through prepareContentBare, which has grown some explicit params that let callers opt in to extra dependencies, such as a PerAccountStoreWidget ancestor. With this, I have two benefits in mind:

1. As Zulip content widgets gain natural dependencies (such as a ThemeExtension for custom light- and dark-theme colors), we can provide them easily, by just adding to this one function.
2. It will be more obvious when a content widget has "extra" dependencies that will become problematic with content: Support Zulip content outside messages (even outside per-account contexts) #488.

Related: #95
Related: #488

[chrisbobbe]

I'm not sure if prepareContentBare is still a good name for this function. There are still some functions called prepareContent left in the file, but they all call prepareContentBare.

[gnprice]

Yeah, "bare" is definitely no longer an accurate word for this function :-)

Comments below. Then here's an idea I had after writing most of those comments and coming back to this:

Instead of having this method take a String html, it can take Widget child, intended to be the BlockContentList or MessageContent. Then have helpers like

  Widget plainContent(String html) {
    return BlockContentList(nodes: parseContent(html).nodes);
  }

  Widget messageContent(String html) {
    return MessageContent(message: eg.streamMessage(content: html),
       content: parseContent(html));
  }

Then call sites can look like

  void testContentSmoke(ContentExample example) {
    testWidgets('smoke: ${example.description}', (tester) async {
      await prepare(tester, plainContent(example.html));

where the prepare method itself might as well just be called prepare.

That makes defaultTextStyleOverride (and wrapWithScaffold) entirely unnecessary too — the caller can easily wrap the result of plainContent or messageContent in whatever other widgets it needs.

[gnprice]

Unlike the other conditionals, this one is replacing widget rather than wrapping it. Probably clearest to group the two cases explicitly together, as one stanza and as an if/else.

[gnprice]

Since we don't know of a case where we need this, let's just cut it — we can easily add it back later if we do find a use for it.

[gnprice]

This is used in just one place, right?

I'd rather handle it with something generic — so the specifics like DefaultTextStyle live at the caller instead. A good way to do that is probably for this to look like

if (builder != null) {
  widget = builder(widget);
}

[gnprice]

This particular feature makes me uneasy to have here in the generic shared helper because there's no easy way to search for the particular test cases that rely on it. One has to look for call sites that use the return value, and potentially trace that data flow onward.

The industrial-strength way to solve that would be to refactor this helper from a function prepareContentBare into being a class instead. Then pushedRoutes would be data on that class — either a public field, or a private one with a method like takePushedRoutes to consume it — and one could look for references to the relevant field or method.

But rather than undertake that, I think a good solution would be to just make this feature a bit more generic, with just enough of a hook for the caller to be able to integrate in the needed way with the MaterialApp: have this prepare method take something like List<NavigatorObserver> navObservers = const []. Then the three call sites stay a few lines longer than in this version, but I think that's OK. Those lines are a tad verbose, but aren't all wasted either — they provide some explicitness which is nice.

If we wanted to simplify the TestNavigatorObserver / pushedRoutes stuff in those call sites, a good direction would be to extend TestNavigatorObserver itself. It's a class in our own test code; we use it in a number of other test files too, and it looks like in every case we do the exact same thing with it.

[chrisbobbe]

That makes defaultTextStyleOverride (and wrapWithScaffold) entirely unnecessary too — the caller can easily wrap the result of plainContent or messageContent in whatever other widgets it needs.

Are you also thinking that callers would also provide their own PerAccountStoreWidget? By centralizing things (with wrapWithPerAccountStoreWidget being an important example), I'm hoping to encourage thinking systematically about the content widgets' dependencies as the widgets develop. If callers have the flexibility to add whatever widgets they need, I wonder if that purpose is kind of defeated. Some dependencies will end up being problematic for #488, right, and I think it'll help if those dependencies are few and well-defined, with names like wrapWithPerAccountStoreWidget. This should help simplify the widgets' migration path toward #488.

A DefaultTextStyle ancestor shouldn't be a problematic dependency for #488. But it is still a dependency, of the vast majority of our content widgets (setting text color as the design specifies), so it seemed helpful to centralize that too, as long as we're thinking systematically about dependencies. It's true that one test wants to fiddle with that DefaultTextStyle, and I guess it's annoying to have a param that's only used that once. But it seemed an OK tradeoff.

[gnprice]

I see. From a #488 perspective, I think the only particularly problematic dependency is MessageContent. So we'll be able to find those with the messageContent helper, or for that matter directly.

Using PerAccountStoreWidget is fine and normal for widgets across most of our UI, everything that's not somewhere independent of any realm. Even with #488, content is still specific to a realm, just not one we're logged into — I expect a realm description can have relative links, which are to be interpreted relative to the realm URL.

So when we go to handle #488, we'll need to invent a new abstraction that allows getting a realm URL without a full-blown PerAccountStore (or even Account), and then we'll migrate a bunch of content widgets to use that. Probably we'll move PerAccountStore.tryResolveUrl to live on that abstraction. I think it'll be pretty straightforward at that point to find the spots that need to be migrated — mostly just look for PerAccountStoreWidget.of in lib/widgets/content.dart. In any case that abstraction doesn't exist yet, so PerAccountStoreWidget is exactly what the content widgets should be using at present.

The wrapWithPerAccountStoreWidget is handling a few more lines of code than most of the others, and it's something a lot of the call sites need. So for that reason I think it basically pulls its weight as a feature of this prepare function, and I don't mind having it there.

[chrisbobbe]

I expect a realm description can have relative links, which are to be interpreted relative to the realm URL.

Yeah, that's probably true.

So when we go to handle #488, we'll need to invent a new abstraction that allows getting a realm URL without a full-blown PerAccountStore (or even Account), and then we'll migrate a bunch of content widgets to use that.

Makes sense.

When we render links that are narrow links, we depend on other data too—about streams, the self-user ID, maybe data on other users (for DM links), maybe message-move history (not sure). We'll want to make sure all that data gets used when we have it, while not crashing in the realm-description context, where we won't have it. A nice extra touch in that context would be an error dialog specific to narrow links, saying that they can't be opened in the normal way.

When we render custom image emoji, and some image previews and (with #656) some video previews, we expect the user's auth credential, so we can request a protected resource on the org's server. So for these elements, we'll want the auth credential to get used when we have it, but we shouldn't crash when we don't have it.

This might not change our calculations, but I wanted to point out that we have more to do about the PerAccountStoreWidget dependency than replacing it with just the realm URL.

[gnprice]

True. I think those will also be pretty straightforward to deal with when we get to #488, though. In particular they'll both come up when we search lib/widgets/content.dart for PerAccountStoreWidget.of.

When we render links that are narrow links, we depend on other data too—about streams, the self-user ID, […]

(Only when we try to open them, not render them in the first place, right? Though it doesn't change the overall point here.)

[chrisbobbe]

Thanks for the review! Revision pushed.

[chrisbobbe]

(Just updated to add a few small code comments.)

[gnprice]

Thanks! Looks good; merging.