Write inline-image widget code more like image-preview widget code

[chrisbobbe]

Greg suggested in #2067 (comment) :

Perhaps as a followup PR, so while this is still fresh in mind but without blocking us merging and releasing the feature: this InlineImage is doing very nearly the same things as the MessageImagePreview widget, especially here in the _buildContent method, but the code looks more different than it is. It'd be nice to refactor them to look more similar so that the real differences are easier to spot.

Ideally, the logic here in _buildContent would become a helper widget that's shared between InlineImage and MessageImagePreview.

This PR is meant to resolve the first paragraph, with the helper-widget suggestion in the second paragraph left for a future PR.

There are two behavior changes that are just about future-proofing in case servers start producing the inline-image HTML form for external images too:

9748f1a content: Anticipate external images in how we choose lightbox thumbnailUrl
684834a content: Anticipate external image URLs in how we choose lightbox src

And one behavior change that affects behavior with current servers:

d85daef content: In inline images, also offer the lightbox when node.loading

[gnprice]

Thanks! A couple of questions below.

I think probably it'd make sense to factor out the helper widget first, and only then merge the branch. That way it gives us some confidence that the direction this is taking is the right one for our eventual destination — i.e. that we want to be mostly making InlineImage more like MessageImagePreview rather than vice versa.

[gnprice]

Why is null preferable here in the non-thumbnail case? (The status quo being that we use resolvedSrc in all cases.)

The commit says:

content: Anticipate external images in how we choose lightbox thumbnailUrl

As of now (2026-01), servers don't put a non-thumbnail URL as `src` in
inline images, except for the loading case when it's the URL of a
spinner image. So this is just something that may be helpful for
future servers. (This widget code has an early return on
`node.loading`, so we don't need to include that as part of this
condition.)

but I don't see why that means we prefer null.

[chrisbobbe]

Ah OK, yes, my reasoning was incomplete:

• If this code is reached, it means we know what URL to request for the lightbox display. The lightbox takes an optional thumbnailUrl so it can show a low-res version of the image while that display image is being fetched.
• When node.loading is true, we shouldn't use resolvedSrc for thumbnailUrl because resolvedSrc is the "spinner" image URL, and we never want to show that spinner image because it's not consistent with our UI. We don't have a thumbnail of the actual image—that's the "loading" that node.loading represents—so we can't tell the lightbox how to show a low-res version of the image while the original is being fetched.
• When src is a thumbnail and node.loading is false, that's the common case, where we can give the lightbox what it wants (a low-res version of the image to show while the original is being fetched). That's unchanged in this commit.
• When src is not a thumbnail and node.loading is false, that's not something we expect current servers to produce (for inline images). To decide whether to pass resolvedSrc as thumbnailUrl, I notice that in this case we choose lightboxDisplayUrl to be identical to resolvedSrc. Therefore we know that resolvedSrc isn't a low-res version of what we'll show in the lightbox, so it can't help the lightbox in the way that it wants. In fact, on first opening the lightbox (when the lightbox image hasn't been loaded and cached), passing null for thumbnailUrl should prevent an unwanted double-request for the same image. (No, I guess actually it would've been loaded and cached, for the message-list display.) This is the actual, desired behavior in image previews, which I think it makes sense to match for inline images. In image previews, we understand that external images produce this case, which is why I framed it that way in the commit message.

If desired, we could give the lightbox different behavior when we don't have a low-res image URL to pass—e.g. it could show a CupertinoActivityIndicator() while the original image is being fetched—but I think that's out of scope here.

[gnprice]

And here, it looks like the effect is to switch in the non-thumbnail case from resolvedOriginalSrc to resolvedSrc.

Reading the commit message a second time, I guess the point is that resolvedOriginalSrc would be null in that case?

content: Anticipate external image URLs in how we choose lightbox src

As of now (2026-01), servers don't put a non-thumbnail URL as `src` in
inline images, except for the loading case (which isn't relevant in
the touched code because of an early return on `node.loading` above
it).

So this is just meant to be helpful for potential future servers that
start producing inline-image HTML for external images, if they handle
the external-image case in much the same way as they already do in
image-preview HTML.

[chrisbobbe]

I guess the point is that resolvedOriginalSrc would be null in that case?

It might be null—we can work that out with the future servers—but I think the point here is that we avoid requesting a URL like https://upload.wikimedia.org/etc/etc except when saving an image to the device, and we use a URL like /external_content/etc/etc/ for both the message content and the lightbox display. We don't know if the actual external URL would be carried by resolvedOriginalSrc, but it plausibly could be, so might as well avoid resolvedOriginalSrc for the lightbox display, as we do in image previews. I'm thinking of Alex's message about image previews, at #api documentation > documenting inline images @ 💬:

There are three things one might do with the resource -- view it in the message feed, view it by itself, and store it on the device. The default for those is (src, src, href) and for modern thumbnails, it's (src, href, href)

[chrisbobbe]

Thanks for the review!

I think probably it'd make sense to factor out the helper widget first, and only then merge the branch. That way it gives us some confidence that the direction this is taking is the right one for our eventual destination — i.e. that we want to be mostly making InlineImage more like MessageImagePreview rather than vice versa.

That makes sense. Revision pushed, this time with that helper widget.

[gnprice]

Thanks! Looks great, and those revised commit messages are helpful. Two nits.

[gnprice]

nit: conventionally (in Flutter upstream), these buildFoo callbacks take positional arguments:

Suggested change

	buildContainer: ({required onTap, required child}) {
	buildContainer: (onTap, child) {

[gnprice]

nit: in this file, the entry point is at the top and callees below; so put this either after InlineImage below, or at the end of all the widgets before _launchUrl

[chrisbobbe]

Thanks for the review! Revision pushed.

[gnprice]

Thanks! Looks good; merging.