content: Use RealmStore.serverThumbnailFormats for thumbnails #1987

chrisbobbe · 2025-11-13T02:30:04Z

Fixes #1936.

rajveermalviya

Thanks @chrisbobbe! LGTM, just one small comment.

rajveermalviya · 2025-11-17T12:43:44Z

test/model/content_test.dart

-          --data-urlencode 'narrow=[{"operator":"sender", "operand":2187},
+          --data-urlencode 'narrow=[{"operator":"sender", "operand":13313},
                                    {"operator":"stream", "operand":"test here"},
-                                    {"operator":"topic",  "operand":"content"}]' \
+                                    {"operator":"topic",  "operand":"Thumbnails"}]' \


Left from debug?

gnprice

Thanks for taking care of this! Comments below.

gnprice · 2025-11-22T03:11:47Z

lib/model/content.dart

+  /// Use [ImageThumbnailLocatorExtension.resolve] to obtain a suitable URL
+  /// for the current UI need.
+  /// It may not work without adding authentication credentials to the request.


This seems like information for the ImageThumbnailLocator doc.

bump — I meant this whole 3-line paragraph 🙂

gnprice · 2025-11-22T03:14:54Z

lib/model/content.dart

+  }
+
+  @override
+  int get hashCode => Object.hash(urlPath, hasAnimatedVersion);


In hashCode, seed the hash with the class name so that the hash doesn't collide with other objects that happen to also consist of a string and a bool:

Suggested change

int get hashCode => Object.hash(urlPath, hasAnimatedVersion);

int get hashCode => Object.hash('ImageThumbnailLocator', urlPath, hasAnimatedVersion);

gnprice · 2025-11-22T03:19:26Z

test/model/content_test.dart

+        thumbnail: ImageThumbnailLocator(
+          urlPath: '/user_uploads/thumbnail/2/ce/nvoNL2LaZOciwGZ-FYagddtK/image.jpg/840x560.webp',
+          hasAnimatedVersion: false,
+        ),


nit: can keep these a bit more compact:

Suggested change

thumbnail: ImageThumbnailLocator(

urlPath: '/user_uploads/thumbnail/2/ce/nvoNL2LaZOciwGZ-FYagddtK/image.jpg/840x560.webp',

hasAnimatedVersion: false,

),

thumbnail: ImageThumbnailLocator(hasAnimatedVersion: false,

urlPath: '/user_uploads/thumbnail/2/ce/nvoNL2LaZOciwGZ-FYagddtK/image.jpg/840x560.webp'),

gnprice · 2025-11-22T03:21:42Z

test/model/content_test.dart

+          urlPath: '/user_uploads/thumbnail/2/9f/tZ9c5ZmsI_cSDZ6ZdJmW8pt4/2c8d985d.gif/840x560-anim.webp',
+          hasAnimatedVersion: true,


Hmm. From this URL path, it sounds like the given path is the animated version. That makes the name "has animated version" seem a bit misleading.

The parser shows the attribute name in the HTML is data-animated. The most direct translation of that would be to call this field just animated. How about we use that name?

gnprice · 2025-11-22T03:25:05Z

lib/model/content.dart

-  final String? thumbnailUrl;
+  final ImageThumbnailLocator? thumbnail;


I think it'd be helpful to split this commit:

An NFC prep commit makes the change on this line, and the other changes that makes necessary, with the new class having only urlPath.

Then a separate commit adds hasAnimatedVersion (or animated per another comment).

There are a lot of places that change in boring ways just from moving this URL path out to the new class, so it'd make the substantive changes easier to read if they were in a separate commit from that.

Very helpful, thanks 🙂

gnprice · 2025-11-22T03:41:20Z

lib/widgets/image.dart

+    final store = PerAccountStoreWidget.of(context);
+    ThumbnailFormat? bestCandidate;
+
+    final animateIfSupported = animationMode.resolve(context);
+    if (hasAnimatedVersion && animateIfSupported) {
+      bestCandidate ??= _bestFormatOf(store.sortedAnimatedThumbnailFormats,
+                          width: widthPhysicalPx, height: heightPhysicalPx);
+    }
+
+    bestCandidate ??= _bestFormatOf(store.sortedStillThumbnailFormats,
+                        width: widthPhysicalPx, height: heightPhysicalPx);


nit: join up the bestCandidate computation as one stanza (and separate it from getting store)

gnprice · 2025-11-22T03:45:41Z

lib/widgets/image.dart

+    if (bestCandidate == null) {
+      // Odd if we'd need to fall back to the format encoded in [locator]'s path.
+      // Seems theoretically possible though:
+      // maybe this format isn't used now, for new uploads,
+      // but it was used in the past, including for this image.
+      return store.realmUrl.replace(path: urlPath);


This is normal if there are no thumbnail formats at all, right?

In fact I think if this happens, it must be the case that there are no still thumbnail formats.

By "there are no thumbnail formats at all", do you mean server_thumbnail_formats in the /register response was empty? I assume the common reason for that is that no uploaded images have ever been thumbnailed, and in that scenario this code won't be reached.

This condition is about what to do when server_thumbnail_formats was empty but somehow we have a thumbnail URL in our hands. A thumbnail URL encodes a thumbnail format at the end, like "/840x560.webp". I'm assuming this can happen if an image was thumbnailed but then later the server admin said "hmm actually we don't want to do thumbnailing for new uploads anymore". We can be optimistic that the thumbnail is still reachable and go ahead and request it.

I see. I think the "Odd if …" confused me — it sounded like it was saying this case is odd if it means we need to fall back.

Does this version cover it?

Suggested change

if (bestCandidate == null) {

// Odd if we'd need to fall back to the format encoded in [locator]'s path.

// Seems theoretically possible though:

// maybe this format isn't used now, for new uploads,

// but it was used in the past, including for this image.

return store.realmUrl.replace(path: urlPath);

if (bestCandidate == null) {

// There are no known thumbnail formats applicable; and yet we have this

// thumbnail locator, indicating this image has a thumbnail.

// Unlikely but seems theoretically possible:

// maybe thumbnailing isn't used now, for new uploads,

// but it was used in the past, including for this image.

// Anyway, fall back to the format encoded in this locator's path.

return store.realmUrl.replace(path: urlPath);

gnprice · 2025-11-22T03:54:37Z

lib/widgets/image.dart

+      return store.realmUrl.replace(path: urlPath);
+    }
+
+    final lastSlashIndex = urlPath.lastIndexOf('/');


These lines both assume that the URL string in urlPath is indeed just a path, and doesn't also contain a query and/or fragment.

I think it'd be reasonable for the API to guarantee that, but it doesn't appear to now (at https://zulip.com/api/message-formatting#images ). Without such a guarantee, a query component (though maybe not a fragment) would be a reasonable choice for this bit of API in isolation — e.g. Gravatar does that, as seen in our GravatarUrl.

(OTOH we do know it starts with a path component, because it starts with /u.)

So it'd be good to discuss that in #api documentation or #api design. Also to have the ImageThumbnailLocator constructor assert it, like it already does for the start of the path. A suitable check would be that there is no ? or # character.

Makes sense; I've started a discussion: #api documentation > `server_thumbnail_formats` in register response @ 💬

Following that discussion, let's have this accommodate a query. (And it's fine if it also accepts a fragment; the spec might start promising that won't appear, since it wouldn't make much sense, but our code won't need to try to enforce that.)

One way to do that is to first find the query, if any, using .indexOf('?'). (The character ? can't appear in a URL path string: https://url.spec.whatwg.org/#path-absolute-url-string .)

gnprice · 2025-11-22T04:01:38Z

test/widgets/image_test.dart

+        'https://chat.example/user_uploads/thumbnail/1/2/a/pic.jpg/1000x2000.png');
+    });
+
+    testWidgets('animated version does not exist', (tester) async {


nit: probably clearest to put this test before its sibling — this seems like the simpler case to think about, and covers most of the interesting logic (and is surely the most common case)

Then the other test can focus just on what changes when hasAnimatedVersion is true.

gnprice · 2025-11-22T04:08:14Z

test/widgets/image_test.dart

+      doCheck(tester, 250, 450, false, hasAnimatedVersion: false,
+        'https://chat.example/user_uploads/thumbnail/1/2/a/pic.jpg/500x900.jpg');
+      doCheck(tester, 300, 250, true, hasAnimatedVersion: false,
+        'https://chat.example/user_uploads/thumbnail/1/2/a/pic.jpg/840x560.webp');


Hmm, why do these get different results? Both formats should be big enough for both these desired sizes, right?

… Ah, I see: it's because they're in physical pixels.

Would be good to have a comment highlighting that. 🙂 That's also one of the key points this test is making, so it's good to make it explicit.

Soon we'll add a field on this for the `data-animated` attribute, then write some logic to help UI code choose a thumbnail format from RealmStore.serverThumbnailFormats.

As suggested by Greg: zulip#1987 (comment)

Fixes zulip#1936.

chrisbobbe · 2025-12-06T02:16:52Z

Thanks! Revision pushed.

gnprice

Thanks! Comments below.

gnprice · 2025-12-11T22:52:24Z

test/model/content_test.dart

+  static final imagePreviewSingleAnimated = ContentExample(
+    'single image preview, with animated version',


nit: something like

Suggested change

static final imagePreviewSingleAnimated = ContentExample(

'single image preview, with animated version',

static final imagePreviewSingleAnimated = ContentExample(

'single image preview, animated',

since the animated version is the primary one

gnprice · 2025-12-11T22:52:48Z

lib/model/content.dart

+  /// Use [ImageThumbnailLocatorExtension.resolve] to obtain a suitable URL
+  /// for the current UI need.
+  /// It may not work without adding authentication credentials to the request.


bump — I meant this whole 3-line paragraph 🙂

gnprice · 2025-12-11T22:53:36Z

pubspec.lock

      name: matcher
-      sha256: dc58c723c3c24bf8d3e2d3ad3f2f9d7bd9cf43ec6feaa64181775e60190153f2
+      sha256: "12956d0ad8390bbcc63ca2e1469c0619946ccb52809807067a7020d57e647aa6"
      url: "https://pub.dev"
    source: hosted
-    version: "0.12.17"
+    version: "0.12.18"


Looks like unintended extraneous changes here.

gnprice · 2025-12-11T22:54:12Z

lib/model/content.dart

-  final String? thumbnailUrl;
+  final ImageThumbnailLocator? thumbnail;


Very helpful, thanks 🙂

gnprice · 2025-12-11T23:13:08Z

test/widgets/image_test.dart

+        'https://chat.example/user_uploads/thumbnail/1/2/a/pic.jpg/500x850.jpg');
+      doCheck(tester, 250, 425, false, animated: false,
+        'https://chat.example/user_uploads/thumbnail/1/2/a/pic.jpg/500x850.jpg');
+      // Different output from previous because it's is in physical pixels.


Hmm, this doesn't really explain how that connects to the test data. It also sounds like it's saying the difference is that either this item or the previous one is in physical pixels (and the other isn't).

I'll push an added commit regrouping these and adding a few more comments.

gnprice · 2025-12-11T23:19:50Z

lib/widgets/image.dart

+    if (bestCandidate == null) {
+      // Odd if we'd need to fall back to the format encoded in [locator]'s path.
+      // Seems theoretically possible though:
+      // maybe this format isn't used now, for new uploads,
+      // but it was used in the past, including for this image.
+      return store.realmUrl.replace(path: urlPath);


I see. I think the "Odd if …" confused me — it sounded like it was saying this case is odd if it means we need to fall back.

Does this version cover it?

Suggested change

if (bestCandidate == null) {

// Odd if we'd need to fall back to the format encoded in [locator]'s path.

// Seems theoretically possible though:

// maybe this format isn't used now, for new uploads,

// but it was used in the past, including for this image.

return store.realmUrl.replace(path: urlPath);

if (bestCandidate == null) {

// There are no known thumbnail formats applicable; and yet we have this

// thumbnail locator, indicating this image has a thumbnail.

// Unlikely but seems theoretically possible:

// maybe thumbnailing isn't used now, for new uploads,

// but it was used in the past, including for this image.

// Anyway, fall back to the format encoded in this locator's path.

return store.realmUrl.replace(path: urlPath);

gnprice · 2025-12-11T23:32:30Z

lib/widgets/image.dart

+      return store.realmUrl.replace(path: urlPath);
+    }
+
+    final lastSlashIndex = urlPath.lastIndexOf('/');


Following that discussion, let's have this accommodate a query. (And it's fine if it also accepts a fragment; the spec might start promising that won't appear, since it wouldn't make much sense, but our code won't need to try to enforce that.)

One way to do that is to first find the query, if any, using .indexOf('?'). (The character ? can't appear in a URL path string: https://url.spec.whatwg.org/#path-absolute-url-string .)

chrisbobbe requested a review from rajveermalviya November 13, 2025 02:30

chrisbobbe assigned rajveermalviya Nov 13, 2025

chrisbobbe added the maintainer review PR ready for review by Zulip maintainers label Nov 13, 2025

rajveermalviya approved these changes Nov 17, 2025

View reviewed changes

rajveermalviya added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Nov 17, 2025

rajveermalviya assigned gnprice and unassigned rajveermalviya Nov 17, 2025

rajveermalviya requested a review from gnprice November 17, 2025 13:11

chrisbobbe force-pushed the pr-thumbnail-formats branch from b94e0fb to 7d51241 Compare November 18, 2025 20:08

gnprice reviewed Nov 22, 2025

View reviewed changes

chrisbobbe added 6 commits December 5, 2025 16:36

api: Add serverThumbnailFormats to initial snapshot

ec52b06

store: Add RealmStore.serverThumbnailFormats

6342a22

content [nfc]: Add ImageThumbnailLocator, wrapping thumbnail URLs

9316aac

Soon we'll add a field on this for the `data-animated` attribute, then write some logic to help UI code choose a thumbnail format from RealmStore.serverThumbnailFormats.

content: Parse data-animated on image-preview HTML

ac55b0e

image [nfc]: Rename ImageanimationMode.resolve to shouldAnimate

f950f0c

As suggested by Greg: zulip#1987 (comment)

content: Use RealmStore.serverThumbnailFormats for thumbnails

c7bb1d8

Fixes zulip#1936.

chrisbobbe force-pushed the pr-thumbnail-formats branch from 7d51241 to c7bb1d8 Compare December 6, 2025 02:16

image test [nfc]: Explain ImageThumbnailLocator.resolve tests a bit more

6cb46b9

gnprice reviewed Dec 11, 2025

View reviewed changes

	int get hashCode => Object.hash(urlPath, hasAnimatedVersion);
	int get hashCode => Object.hash('ImageThumbnailLocator', urlPath, hasAnimatedVersion);

		urlPath: '/user_uploads/thumbnail/2/9f/tZ9c5ZmsI_cSDZ6ZdJmW8pt4/2c8d985d.gif/840x560-anim.webp',
		hasAnimatedVersion: true,

		final String? thumbnailUrl;
		final ImageThumbnailLocator? thumbnail;

		static final imagePreviewSingleAnimated = ContentExample(
		'single image preview, with animated version',

content: Use RealmStore.serverThumbnailFormats for thumbnails #1987

Are you sure you want to change the base?

content: Use RealmStore.serverThumbnailFormats for thumbnails #1987

Uh oh!

Conversation

chrisbobbe commented Nov 13, 2025

Uh oh!

rajveermalviya left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Dec 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Dec 6, 2025

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chrisbobbe Dec 6, 2025 •

edited

Loading