content: Handle thumbnails #820

rajveermalviya · 2024-07-17T23:04:13Z

Thumbnails	Placeholders
flutter-thumbnails-edit.mp4	flutter-thumbnails-placeholder.mp4

Fixes: #799

gnprice

Thanks @rajveermalviya! Taking this straight to integration review because it interacts with the server's 9.0 release timeline.

Here's a partial review before I go AFK for dinner. I've read just the model code so far (lib/model/content.dart).

gnprice · 2024-07-18T00:52:45Z

lib/model/content.dart

+    properties.add(FlagProperty('loading', value: loading,
+      ifTrue: "is loading", ifFalse: "not loading"));


nit: hide property when it has its usual boring value

Suggested change

properties.add(FlagProperty('loading', value: loading,

ifTrue: "is loading", ifFalse: "not loading"));

properties.add(FlagProperty('loading', value: loading, ifTrue: "is loading"));

gnprice · 2024-07-18T01:01:38Z

lib/model/content.dart

+  const ImageNode({
+    super.debugHtmlNode,
+    required this.srcUrl,
+    required this.thumbnailUrl,
+    required this.loading,
+  });


Let's split this into two constructors:

Suggested change

const ImageNode({

super.debugHtmlNode,

required this.srcUrl,

required this.thumbnailUrl,

required this.loading,

});

const ImageNode({

super.debugHtmlNode,

required String this.srcUrl,

required String this.thumbnailUrl,

}) : loading = false;

and then an ImageNode.loading that makes those other fields null.

That way the constructors enforce that there are these two mutually exclusive configurations.

… Actually, though, scratch that — because I think we want to model this a bit differently.

Even if the thumbnail is still pending, that's no reason to stop the user from opening the lightbox. So let's always have the original URL, which is the one from the a element.

gnprice · 2024-07-18T01:02:43Z

lib/model/content.dart

+      return ImageNode(
+        srcUrl: null,
+        thumbnailUrl: null,
+        loading: true,
+        debugHtmlNode: debugHtmlNode);


~~so in particular:~~

Suggested change

return ImageNode(

srcUrl: null,

thumbnailUrl: null,

loading: true,

debugHtmlNode: debugHtmlNode);

return ImageNode.loading(debugHtmlNode: debugHtmlNode);

gnprice · 2024-07-18T01:15:22Z

lib/model/content.dart

-    return ImageNode(srcUrl: src, debugHtmlNode: debugHtmlNode);
+    final String srcUrl;
+    final String? thumbnailUrl;
+    if (src.startsWith('/user_uploads/thumbnail/')) {


When this condition is false, let's have a check that src == href, and return unimplemented if they're unequal.

I believe those were always the same when thumbnailing isn't involved; if there are other cases that exist, I'd like to learn about them, either in manual testing as we use the app or as part of #190.

There seems to be multiple cases where src != href, for example src urls starting with /external_content/ and https://uploads.zulipusercontent.net/.
Should they be added one by one explicitly (using startsWith)?

they be added one by one explicitly

Did this for now.

Yep, that sounds good.

Oh, it looks like we don't have a test case that exercises the /external_content/ case, though. Do you have an example? That'd be good to add.

gnprice

Thanks again @rajveermalviya for building this! Done with a review — comments below. Generally it looks great.

The comments are mostly small, but there are a couple that I think might mean glitches a user could spot:

on _buildAppBarFooter and the height of the app bar
on addPostFrameCallback vs. scheduleFrameCallback

gnprice · 2024-07-18T02:20:55Z

lib/widgets/content.dart

+    final Uri? resolvedSrcUrl;
+    final Uri? resolvedThumbnailUrl;
+    if (thumbnailUrl == null) {
+      resolvedSrcUrl = store.tryResolveUrl(srcUrl);


This gets initialized the same way in both cases, right? So that can be pulled out of the conditional to simplify.

gnprice · 2024-07-18T02:28:19Z

lib/widgets/content.dart

        Navigator.of(context).push(getLightboxRoute(
          context: context,
          message: message,
-          src: resolvedSrc,
+          src: resolvedSrcUrl!,
+          thumbnailUrl: thumbnailUrl == null ? null : resolvedThumbnailUrl,


This line (and the similar one just below) is a bit puzzling to read. If thumbnailUrl is null, there's a fallback above that will fill in resolvedThumbnailUrl… but this line seems to be suggesting that that fallback shouldn't entirely be accepted as filling the role of a thumbnail URL.

In that case, it's probably cleanest if the name resolvedThumbnailUrl doesn't refer to the fallback, so that it's just store.tryResolveUrl(thumbnailUrl) or null. It looks like the fallback only ends up getting used in one place below, so the fallback can go there, something like resolvedThumbnailUrl ?? resolvedSrcUrl.

gnprice · 2024-07-18T02:30:16Z

lib/widgets/content.dart

          src: resolvedSrc,
-          mediaType: MediaType.video));
+          mediaType: MediaType.video,
+          thumbnailUrl: null));


nit: keep order the same between this and the other getLightboxRoute call above — helps keep it easy to compare

gnprice · 2024-07-18T02:42:11Z

lib/widgets/lightbox.dart

+  final PreferredSizeWidget Function(BuildContext context)? buildAppBarFooter;
  final Widget? Function(
    BuildContext context, Color color, double elevation) buildBottomAppBar;


Hmm, this is a tricky naming problem. 🙂 I don't have a totally non-confusing name to offer, but I think this would be less confusing:

Suggested change

final PreferredSizeWidget Function(BuildContext context)? buildAppBarFooter;

final Widget? Function(

BuildContext context, Color color, double elevation) buildBottomAppBar;

/// For [AppBar.bottom].

final PreferredSizeWidget Function(BuildContext context)? buildAppBarBottom;

final Widget? Function(

BuildContext context, Color color, double elevation) buildBottomAppBar;

The thing is that, at least for me, "footer" really wants to mean the bottom of the whole page or screen.

gnprice · 2024-07-18T02:49:53Z

lib/widgets/lightbox.dart

+                frameBuilder: thumbnailUrl == null ? null
+                  : (context, child, frame, wasSynchronouslyLoaded) {


These two callbacks (frameBuilder and loadingBuilder) have some nontrivial logic in them, so let's pull them out separately to give them a bit more room to breathe and make them easier to read.

Probably cleanest is to make them private methods on this class, and say frameBuilder: _frameBuilder. The thumbnail == null checks can move inside the methods — to get the same effect as the callback having been null, the method can just return its child argument.

gnprice · 2024-07-18T03:27:32Z

lib/widgets/lightbox.dart

+                      _loadingProgress = progress;
+                      // This function is called in a build method and setState
+                      // can't be called in a build method, so delay it.
+                      SchedulerBinding.instance.addPostFrameCallback((_) { if (mounted) setState(() {}); });


Should this be:

Suggested change

SchedulerBinding.instance.addPostFrameCallback((_) { if (mounted) setState(() {}); });

SchedulerBinding.instance.scheduleFrameCallback((_) { if (mounted) setState(() {}); });

The addPostFrameCallback doc seems pretty stern about saying that it doesn't cause a new frame to actually happen, and as a result the callback might not get called for a while if ever.

Even the scheduleFrameCallback version seems not quite optimal in that it means the indicator will lag one frame behind the progress reflected in the actual image. But that's tolerable.

gnprice · 2024-07-18T03:33:25Z

lib/widgets/lightbox.dart

@@ -19,20 +20,22 @@ import 'store.dart';
 //   fly to an image preview with a different URL, following a message edit
 //   while the lightbox was open.
 class _LightboxHeroTag {
-  _LightboxHeroTag({required this.messageId, required this.src});
+  _LightboxHeroTag({required this.messageId, required this.src, required this.thumbnailUrl});


I think we don't need the thumbnail URL in the tag for the hero. The role of the tag is only for equality checks to match up a hero from one frame to the next (and in particular from one page/screen to the next); and src should be enough to disambiguate, as there shouldn't be multiple images with the same src but different thumbnailUrl (let alone within the same message).

gnprice · 2024-07-18T03:44:48Z

test/model/content_test.dart

+  static const imageSingleWithThumbnail = ContentExample(
+    'single image with thumbnail',


Generally let's give the nice short simple names to things that represent the current way of doing things, and let the old legacy way have a longer name to distinguish itself. So:

Suggested change

static const imageSingleWithThumbnail = ContentExample(

'single image with thumbnail',

static const imageSingle = ContentExample(

'single image',

and the old imageSingle can be imageSingleNoThumbnail.

I guess that principle could also point toward putting thumbnails on all the other image-related examples, but let's skip that. In fact most of them are already a bit artificial, with the use of /user_avatars/2/realm/icon.png — I chose that after some experimentation as a value that kept the HTML shorter than it would usually be (while still working in real life).

gnprice · 2024-07-18T03:50:15Z

test/model/content_test.dart

+
+  static const imageClusterWithThumbnails = ContentExample(
+    'multiple images with thumbnails',
+    "https://chat.zulip.org/user_avatars/2/realm/icon.png?version=3\nhttps://chat.zulip.org/user_avatars/2/realm/icon.png?version=4",


This doesn't look like it matches the HTML below 🙂

Relatedly, let's include a link to the example message that had this Markdown source and produced this HTML. That will help us toward automatically validating in the future that the Markdown source in these examples actually corresponds to the given HTML. See this issue for eventually adding that validation:

content test: Record URLs of sample messages, and validate them #617

and this previous PR subthread for background: #603 (comment)

So in this example I believe it would look like:

Suggested change

"https://chat.zulip.org/user_avatars/2/realm/icon.png?version=3\nhttps://chat.zulip.org/user_avatars/2/realm/icon.png?version=4",

https://chat.zulip.org/#narrow/stream/7-test-here/near/1893154

"https://chat.zulip.org/user_avatars/2/realm/icon.png?version=3\nhttps://chat.zulip.org/user_avatars/2/realm/icon.png?version=4",

except with the corrected Markdown.

(After having that idea back in that subthread on #603 in April, it looks like I forgot about it while we were iterating on your videos PR in the weeks after that, oops. Anyway, let's start doing it for new content examples from this point on — it's generally pretty easy to do when you're making the examples in the first place and they're fresh, and will be more of a pain to go back and fill in later.)

gnprice · 2024-07-18T04:00:23Z

test/model/content_test.dart

+  static const imageClusterWithThumbnails = ContentExample(
+    'multiple images with thumbnails',


This can get the same "NoThumbnails" treatment as the single-image case above.

rajveermalviya · 2024-07-18T06:37:11Z

lib/widgets/content.dart

-          resolvedSrc,
-          filterQuality: FilterQuality.medium)));
+      child: node.loading
+        ? const CupertinoActivityIndicator()


Also forgot to flag this earlier, the reason for not using the loader-{black,white}.svg asset here is that, currently animated icons can't be included in ZulipIcons font (don't know if animated fonts are even possible), I tried and it just displayed a single (first) frame.

So, for now used the CupertinoActivityIndicator widget which conveniently is similar to the svg.

Handy that that's there!

Yeah, I suspect animated fonts are not possible. If we were determined to use the icons from those SVGs, we could find a way (if nothing else, we could rasterize them to high-enough-resolution PNGs). But using this widget instead is a lot simpler and SGTM.

Would you post a screenshot and a video showing what this placeholder looks like? That doesn't require any fiddling on the server — the quick hack I used just now is:

--- lib/model/content.dart +++ lib/model/content.dart @@ -1064,7 +1064,7 @@ class _ZulipContentParser { if (href == null) { return UnimplementedBlockContentNode(htmlNode: divElement); } - if (imgElement.className == 'image-loading-placeholder') { + if (true || imgElement.className == 'image-loading-placeholder') { return ImageNode( srcUrl: href, thumbnailUrl: null,

Added a video showing placeholders in the PR description.

rajveermalviya · 2024-07-18T06:37:43Z

Thanks for the review @gnprice, pushed a new revision PTAL :)

gnprice

Thanks @rajveermalviya for the revision! Just a few nits in this round, below; plus please post a screenshot and video as per #820 (comment) above.

gnprice · 2024-07-19T00:24:42Z

lib/model/content.dart

+  /// It may be null if Server didn't generate a thumbnail, or doesn't support
+  /// it yet. It will also be null when [loading] is true.


Suggested change

/// It may be null if Server didn't generate a thumbnail, or doesn't support

/// it yet. It will also be null when [loading] is true.

///

/// This will be null if the server hasn't yet generated a thumbnail,

/// or is a version that doesn't offer thumbnails.

/// It will also be null when [loading] is true.

(in particular I found the meaning of "yet" a bit hard to parse in the current revision)

gnprice · 2024-07-19T00:29:57Z

lib/widgets/lightbox.dart

+  PreferredSizeWidget? _buildAppBarBottom(BuildContext context) {
+    if (_loadingProgress == null) {
+      return null;


Cool, moving this here (from the call site) is a nice simplification too.

gnprice · 2024-07-19T00:33:44Z

lib/widgets/lightbox.dart

+      _loadingProgress = progress;
+      // This function is called in a build method and setState
+      // can't be called in a build method, so delay it.
+      SchedulerBinding.instance.scheduleFrameCallback((_) { if (mounted) setState(() {}); });


nit: lots going on on this line (lots of control flow, with two function literals and a conditional); split it up

In fact I think it benefits from being expanded slightly more, to turn the if-mounted check into an early return to match the pattern it usually appears as:

Suggested change

SchedulerBinding.instance.scheduleFrameCallback((_) { if (mounted) setState(() {}); });

SchedulerBinding.instance.scheduleFrameCallback((_) {

if (!mounted) return;

setState(() {});

});

gnprice · 2024-07-19T00:39:45Z

lib/widgets/lightbox.dart

+      // This function is called in a build method and setState
+      // can't be called in a build method, so delay it.


Given the discussion in #mobile-team about how these delayed callbacks are generally to be avoided, let's include in the comment an indication of why in this case it's the best available option. That'll help the reader not wonder if we just didn't think it through; if they want to try to find a solution we missed, it'll let them see the information we'd found so they can start from there.

So:

Suggested change

// This function is called in a build method and setState

// can't be called in a build method, so delay it.

// The [Image.network] API lets us learn progress information only at

// its build time. That's too late for updating the progress indicator,

// so delay that update to the next frame. For discussion, see:

// https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/addPostFrameCallback/near/1893539

// https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/addPostFrameCallback/near/1894124

gnprice

Oh and two more comments — I'd missed some of the changes in my reading a minute ago. One is a still-smaller nit; the other asks for one more test case.

gnprice · 2024-07-19T00:51:14Z

lib/model/content.dart

+    } else if (src.startsWith('/external_content/')
+      || src.startsWith('https://uploads.zulipusercontent.net/')) {
+      srcUrl = src;


nit: indent if condition's continuation to not look like part of the body:

Suggested change

} else if (src.startsWith('/external_content/')

|| src.startsWith('https://uploads.zulipusercontent.net/')) {

srcUrl = src;

} else if (src.startsWith('/external_content/')

|| src.startsWith('https://uploads.zulipusercontent.net/')) {

srcUrl = src;

gnprice · 2024-07-19T00:54:14Z

lib/model/content.dart

-    return ImageNode(srcUrl: src, debugHtmlNode: debugHtmlNode);
+    final String srcUrl;
+    final String? thumbnailUrl;
+    if (src.startsWith('/user_uploads/thumbnail/')) {


Yep, that sounds good.

Oh, it looks like we don't have a test case that exercises the /external_content/ case, though. Do you have an example? That'd be good to add.

rajveermalviya · 2024-07-19T02:48:49Z

Thanks for the review @gnprice, pushed a new revision PTAL.

gnprice

Thanks for the revision! Everything looks good, modulo one question below.

@alya see the placeholders demonstrated above (and any frame of that video works as a screenshot). They're slightly different from the design we have in web, because this is easiest for reasons discussed at #820 (comment) . Given the release timeline (and because this version seems fine) I won't block merging this to wait for feedback on the design, but we can always adjust it later if desired.

gnprice · 2024-07-19T02:52:24Z

test/model/content_test.dart

+    'single image external',
+    "https://upload.wikimedia.org/wikipedia/commons/7/78/Verregende_bloem_van_een_Helenium_%27El_Dorado%27._22-07-2023._%28d.j.b%29.jpg",


Is there an example message for this on chat.zulip.org which you can link to?

Ha, yeah I'd just noticed I forgot to add that and pushed a new revision with that.

Fixes: zulip#799

gnprice · 2024-07-19T03:02:07Z

Merged! Thanks again @rajveermalviya for the fast work building this and the swift revisions.

gnprice added the integration review Added by maintainers when PR may be ready for integration label Jul 18, 2024

gnprice self-requested a review July 18, 2024 00:51

gnprice reviewed Jul 18, 2024

View reviewed changes

rajveermalviya force-pushed the pr-thumbnails branch from c61a99b to 016ce48 Compare July 18, 2024 06:29

rajveermalviya commented Jul 18, 2024

View reviewed changes

rajveermalviya requested a review from gnprice July 18, 2024 06:37

rajveermalviya force-pushed the pr-thumbnails branch 2 times, most recently from 45fc61d to e7c434a Compare July 18, 2024 08:50

gnprice reviewed Jul 19, 2024

View reviewed changes

rajveermalviya force-pushed the pr-thumbnails branch from e7c434a to c704eaf Compare July 19, 2024 02:35

rajveermalviya requested a review from gnprice July 19, 2024 02:48

gnprice reviewed Jul 19, 2024

View reviewed changes

rajveermalviya force-pushed the pr-thumbnails branch from c704eaf to beb25fa Compare July 19, 2024 02:57

rajveermalviya added 2 commits July 18, 2024 20:00

content: Support image thumbnails and placeholder

7db3044

lightbox: Support thumbnail to original image hero transition

3d93479

Fixes: zulip#799

gnprice force-pushed the pr-thumbnails branch from beb25fa to 3d93479 Compare July 19, 2024 03:01

gnprice merged commit 3d93479 into zulip:main Jul 19, 2024
1 check passed

rajveermalviya deleted the pr-thumbnails branch July 19, 2024 03:03

gnprice mentioned this pull request Jul 22, 2024

Show thumbnail at main image's size in lightbox #830

Closed

		properties.add(FlagProperty('loading', value: loading,
		ifTrue: "is loading", ifFalse: "not loading"));

		frameBuilder: thumbnailUrl == null ? null
		: (context, child, frame, wasSynchronouslyLoaded) {

	SchedulerBinding.instance.addPostFrameCallback((_) { if (mounted) setState(() {}); });
	SchedulerBinding.instance.scheduleFrameCallback((_) { if (mounted) setState(() {}); });

		static const imageSingleWithThumbnail = ContentExample(
		'single image with thumbnail',

	"https://chat.zulip.org/user_avatars/2/realm/icon.png?version=3\nhttps://chat.zulip.org/user_avatars/2/realm/icon.png?version=4",
	https://chat.zulip.org/#narrow/stream/7-test-here/near/1893154
	"https://chat.zulip.org/user_avatars/2/realm/icon.png?version=3\nhttps://chat.zulip.org/user_avatars/2/realm/icon.png?version=4",

		static const imageClusterWithThumbnails = ContentExample(
		'multiple images with thumbnails',

		/// It may be null if Server didn't generate a thumbnail, or doesn't support
		/// it yet. It will also be null when [loading] is true.

-  /// It may be null if Server didn't generate a thumbnail, or doesn't support
-  /// it yet. It will also be null when [loading] is true.
+  ///
+  /// This will be null if the server hasn't yet generated a thumbnail,
+  /// or is a version that doesn't offer thumbnails.
+  /// It will also be null when [loading] is true.

		// This function is called in a build method and setState
		// can't be called in a build method, so delay it.

-      // This function is called in a build method and setState
-      // can't be called in a build method, so delay it.
+      // The [Image.network] API lets us learn progress information only at
+      // its build time.  That's too late for updating the progress indicator,
+      // so delay that update to the next frame.  For discussion, see:
+      //   https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/addPostFrameCallback/near/1893539
+      //   https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/addPostFrameCallback/near/1894124

		'single image external',
		"https://upload.wikimedia.org/wikipedia/commons/7/78/Verregende_bloem_van_een_Helenium_%27El_Dorado%27._22-07-2023._%28d.j.b%29.jpg",

content: Handle thumbnails #820

content: Handle thumbnails #820

Conversation

rajveermalviya commented Jul 17, 2024 • edited Loading

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajveermalviya Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

rajveermalviya Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajveermalviya commented Jul 18, 2024

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajveermalviya commented Jul 19, 2024

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice commented Jul 19, 2024

rajveermalviya commented Jul 17, 2024 •

edited

Loading

rajveermalviya Jul 18, 2024 •

edited

Loading

rajveermalviya Jul 18, 2024 •

edited

Loading