content: Use RealmStore.serverThumbnailFormats for thumbnails

Fixes zulip#1936.

Author: chrisbobbe

lib/model/content.dart

@@ -5,6 +5,7 @@ import 'package:html/parser.dart';
 
 import '../api/model/model.dart';
 import '../api/model/submessage.dart';
+import '../widgets/image.dart';
 import 'code_block.dart';
 import 'katex.dart';
 
@@ -553,7 +554,8 @@ class ImagePreviewNode extends BlockContentNode {
 
   /// The thumbnail URL of the image and whether it has an animated version.
   ///
-  /// [ImageThumbnailLocator.urlPath] is a relative URL string.
+  /// 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 will be null if the server hasn't yet generated a thumbnail,
@@ -599,6 +601,9 @@ class ImagePreviewNode extends BlockContentNode {
 
 /// Data to locate an image thumbnail,
 /// and whether the image has an animated version.
+///
+/// Use [ImageThumbnailLocatorExtension.resolve] to obtain a suitable URL
+/// for the current UI need.
 @immutable
 class ImageThumbnailLocator extends DiagnosticableTree {
   ImageThumbnailLocator({

lib/model/realm.dart

@@ -33,6 +33,12 @@ mixin RealmStore on PerAccountStoreBase, UserGroupStore {
   int get serverTypingStartedWaitPeriodMilliseconds;
 
   List<ThumbnailFormat> get serverThumbnailFormats;
+  /// A digest of [serverThumbnailFormats]: sorted by max resolution, ascending,
+  /// and filtered to those with `animated: true`.
+  List<ThumbnailFormat> get sortedAnimatedThumbnailFormats;
+  /// A digest of [serverThumbnailFormats]: sorted by max resolution, ascending,
+  /// and filtered to those with `animated: false`.
+  List<ThumbnailFormat> get sortedStillThumbnailFormats;
 
   //|//////////////////////////////////////////////////////////////
   // Realm settings.
@@ -170,6 +176,10 @@ mixin ProxyRealmStore on RealmStore {
   @override
   List<ThumbnailFormat> get serverThumbnailFormats => realmStore.serverThumbnailFormats;
   @override
+  List<ThumbnailFormat> get sortedAnimatedThumbnailFormats => realmStore.sortedAnimatedThumbnailFormats;
+  @override
+  List<ThumbnailFormat> get sortedStillThumbnailFormats => realmStore.sortedStillThumbnailFormats;
+  @override
   bool get realmAllowMessageEditing => realmStore.realmAllowMessageEditing;
   @override
   GroupSettingValue? get realmCanDeleteAnyMessageGroup => realmStore.realmCanDeleteAnyMessageGroup;
@@ -235,6 +245,10 @@ class RealmStoreImpl extends HasUserGroupStore with RealmStore {
     serverTypingStoppedWaitPeriodMilliseconds = initialSnapshot.serverTypingStoppedWaitPeriodMilliseconds,
     serverTypingStartedWaitPeriodMilliseconds = initialSnapshot.serverTypingStartedWaitPeriodMilliseconds,
     serverThumbnailFormats = initialSnapshot.serverThumbnailFormats,
+    _sortedAnimatedThumbnailFormats = _filterAndSortThumbnailFormats(
+      initialSnapshot.serverThumbnailFormats, animated: true),
+    _sortedStillThumbnailFormats = _filterAndSortThumbnailFormats(
+      initialSnapshot.serverThumbnailFormats, animated: false),
     realmAllowMessageEditing = initialSnapshot.realmAllowMessageEditing,
     realmCanDeleteAnyMessageGroup = initialSnapshot.realmCanDeleteAnyMessageGroup,
     realmCanDeleteOwnMessageGroup = initialSnapshot.realmCanDeleteOwnMessageGroup,
@@ -381,6 +395,12 @@ class RealmStoreImpl extends HasUserGroupStore with RealmStore {
 
   @override
   final List<ThumbnailFormat> serverThumbnailFormats;
+  @override
+  List<ThumbnailFormat> get sortedAnimatedThumbnailFormats => _sortedAnimatedThumbnailFormats;
+  final List<ThumbnailFormat> _sortedAnimatedThumbnailFormats;
+  @override
+  List<ThumbnailFormat> get sortedStillThumbnailFormats => _sortedStillThumbnailFormats;
+  final List<ThumbnailFormat> _sortedStillThumbnailFormats;
 
   @override
   final bool realmAllowMessageEditing;
@@ -440,6 +460,26 @@ class RealmStoreImpl extends HasUserGroupStore with RealmStore {
     return displayFields.followedBy(nonDisplayFields).toList();
   }
 
+  static List<ThumbnailFormat> _filterAndSortThumbnailFormats(
+    List<ThumbnailFormat> initialServerThumbnailFormats, {
+    required bool animated,
+  }) {
+    return initialServerThumbnailFormats
+      .where((format) => format.animated == animated)
+      .toList()
+      ..sort(_compareThumbnailFormats);
+  }
+
+  /// A comparator to sort formats by max resolution, ascending.
+  ///
+  /// "Max resolution" means
+  /// [ThumbnailFormat.maxWidth] * [ThumbnailFormat.maxHeight].
+  static int _compareThumbnailFormats(ThumbnailFormat a, ThumbnailFormat b) {
+    final aMaxResolution = a.maxWidth * a.maxHeight;
+    final bMaxResolution = b.maxWidth * b.maxHeight;
+    return aMaxResolution - bMaxResolution;
+  }
+
   void handleCustomProfileFieldsEvent(CustomProfileFieldsEvent event) {
     customProfileFields = _sortCustomProfileFields(event.fields);
   }

lib/widgets/content.dart

@@ -637,11 +637,13 @@ class MessageImagePreview extends StatelessWidget {
 
     // TODO image hover animation
     final srcUrl = node.srcUrl;
-    final thumbnailUrl = node.thumbnail?.urlPath;
+    final thumbnailLocator = node.thumbnail;
     final store = PerAccountStoreWidget.of(context);
     final resolvedSrcUrl = store.tryResolveUrl(srcUrl);
-    final resolvedThumbnailUrl = thumbnailUrl == null
-      ? null : store.tryResolveUrl(thumbnailUrl);
+    final resolvedThumbnailUrl = thumbnailLocator?.resolve(context,
+      width: MessageMediaContainer.width,
+      height: MessageMediaContainer.height,
+      animationMode: ImageAnimationMode.animateConditionally);
 
     // TODO if src fails to parse, show an explicit "broken image"
 
@@ -736,6 +738,12 @@ class MessageMediaContainer extends StatelessWidget {
   final void Function()? onTap;
   final Widget? child;
 
+  /// The container's width, in logical pixels.
+  static const width = 150.0;
+
+  /// The container's height, in logical pixels.
+  static const height = 100.0;
+
   @override
   Widget build(BuildContext context) {
     return GestureDetector(
@@ -751,8 +759,8 @@ class MessageMediaContainer extends StatelessWidget {
             child: Padding(
               padding: const EdgeInsets.all(1),
               child: SizedBox(
-                height: 100,
-                width: 150,
+                width: width,
+                height: height,
                 child: child))))));
   }
 }

lib/widgets/image.dart

@@ -2,6 +2,8 @@ import 'package:flutter/foundation.dart';
 import 'package:flutter/widgets.dart';
 
 import '../api/core.dart';
+import '../api/model/initial_snapshot.dart';
+import '../model/content.dart';
 import 'store.dart';
 
 /// Like [Image.network], but includes [authHeader] if [src] is on-realm.
@@ -149,3 +151,59 @@ enum ImageAnimationMode {
     }
   }
 }
+
+extension ImageThumbnailLocatorExtension on ImageThumbnailLocator {
+  /// Chooses an appropriate format from [PerAccountStore.serverThumbnailFormats],
+  /// represented as an absolute URL.
+  ///
+  /// [height] and [width] are in logical pixels.
+  ///
+  /// Requires an ancestor [PerAccountStoreWidget].
+  Uri? resolve(
+    BuildContext context, {
+    required double width,
+    required double height,
+    required ImageAnimationMode animationMode,
+  }) {
+    final devicePixelRatio = MediaQuery.devicePixelRatioOf(context);
+    final widthPhysicalPx = (width * devicePixelRatio).ceil();
+    final heightPhysicalPx = (height * devicePixelRatio).ceil();
+
+    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);
+
+    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);
+    }
+
+    final lastSlashIndex = urlPath.lastIndexOf('/');
+    return store.realmUrl.replace(
+      path: '${urlPath.substring(0, lastSlashIndex)}/${bestCandidate.name}');
+  }
+
+  ThumbnailFormat? _bestFormatOf(
+    List<ThumbnailFormat> sortedCandidates, {
+    required int width,
+    required int height,
+  }) {
+    ThumbnailFormat? result;
+    for (final candidate in sortedCandidates) {
+      result = candidate;
+      if (candidate.maxWidth >= width && candidate.maxHeight >= height) break;
+    }
+    return result;
+  }
+}

test/widgets/image_test.dart

@@ -2,12 +2,17 @@ import 'package:checks/checks.dart';
 import 'package:flutter/widgets.dart';
 import 'package:flutter_test/flutter_test.dart';
 import 'package:zulip/api/core.dart';
+import 'package:zulip/api/model/initial_snapshot.dart';
+import 'package:zulip/model/content.dart';
+import 'package:zulip/model/store.dart';
 import 'package:zulip/widgets/image.dart';
 import 'package:zulip/widgets/store.dart';
 
 import '../example_data.dart' as eg;
 import '../model/binding.dart';
+import '../model/test_store.dart';
 import '../test_images.dart';
+import 'test_app.dart';
 
 void main() {
   TestZulipBinding.ensureInitialized();
@@ -51,4 +56,103 @@ void main() {
       check(tester.takeException()).isA<AssertionError>();
     });
   });
+
+  group('ImageThumbnailLocator.resolve', () {
+    late PerAccountStore store;
+
+    Future<void> prepare(WidgetTester tester) async {
+      addTearDown(testBinding.reset);
+
+      final exampleFormats = [
+        ThumbnailFormat(name: '840x560.webp',
+          maxWidth: 840, maxHeight: 560, animated: false, format: 'webp'),
+        ThumbnailFormat(name: '840x560-anim.webp',
+          maxWidth: 840, maxHeight: 560, animated: true, format: 'webp'),
+        ThumbnailFormat(name: '500x900.jpg',
+          maxWidth: 500, maxHeight: 900, animated: false, format: 'jpg'),
+        ThumbnailFormat(name: '500x900-anim.jpg',
+          maxWidth: 500, maxHeight: 900, animated: true, format: 'jpg'),
+        ThumbnailFormat(name: '1000x1000.webp',
+          maxWidth: 1000, maxHeight: 1000, animated: false, format: 'webp'),
+        ThumbnailFormat(name: '1000x2000-anim.png',
+          maxWidth: 1000, maxHeight: 2000, animated: true, format: 'png'),
+        ThumbnailFormat(name: '1000x1000-anim.webp',
+          maxWidth: 1000, maxHeight: 1000, animated: true, format: 'webp'),
+        ThumbnailFormat(name: '1000x2000.png',
+          maxWidth: 1000, maxHeight: 2000, animated: false, format: 'png'),
+      ];
+
+      await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot(
+        serverThumbnailFormats: exampleFormats,
+      ));
+
+      store = await testBinding.globalStore.perAccount(eg.selfAccount.id);
+      await store.addUser(eg.selfUser);
+
+      await tester.pumpWidget(TestZulipApp(accountId: eg.selfAccount.id));
+      await tester.pump();
+    }
+
+    void doCheck(
+      WidgetTester tester,
+      double width,
+      double height,
+      bool animatedIfSupported,
+      String expected, {
+      required bool hasAnimatedVersion,
+    }) {
+      final locator = ImageThumbnailLocator(
+        urlPath: '/user_uploads/thumbnail/1/2/a/pic.jpg/840x560.webp',
+        hasAnimatedVersion: hasAnimatedVersion,
+      );
+
+      final context = tester.element(find.byType(Placeholder));
+      final result = locator.resolve(context,
+        width: width, height: height,
+        animationMode: animatedIfSupported
+          ? ImageAnimationMode.animateAlways
+          : ImageAnimationMode.animateNever);
+      check(result.toString()).equals(expected);
+    }
+
+    testWidgets('animated version exists', (tester) async {
+      tester.view.devicePixelRatio = 2.0;
+      addTearDown(tester.view.resetDevicePixelRatio);
+
+      await prepare(tester);
+
+      doCheck(tester, 200, 200, false, hasAnimatedVersion: true,
+        'https://chat.example/user_uploads/thumbnail/1/2/a/pic.jpg/500x900.jpg');
+      doCheck(tester, 250, 450, true, hasAnimatedVersion: true,
+        'https://chat.example/user_uploads/thumbnail/1/2/a/pic.jpg/500x900-anim.jpg');
+      doCheck(tester, 250, 450, false, hasAnimatedVersion: true,
+        'https://chat.example/user_uploads/thumbnail/1/2/a/pic.jpg/500x900.jpg');
+      doCheck(tester, 300, 250, true, hasAnimatedVersion: true,
+        'https://chat.example/user_uploads/thumbnail/1/2/a/pic.jpg/840x560-anim.webp');
+      doCheck(tester, 300, 250, false, hasAnimatedVersion: true,
+        'https://chat.example/user_uploads/thumbnail/1/2/a/pic.jpg/840x560.webp');
+      doCheck(tester, 750, 1000, false, hasAnimatedVersion: true,
+        'https://chat.example/user_uploads/thumbnail/1/2/a/pic.jpg/1000x2000.png');
+    });
+
+    testWidgets('animated version does not exist', (tester) async {
+      tester.view.devicePixelRatio = 2.0;
+      addTearDown(tester.view.resetDevicePixelRatio);
+
+      await prepare(tester);
+
+      doCheck(tester, 200, 200, false, hasAnimatedVersion: false,
+        'https://chat.example/user_uploads/thumbnail/1/2/a/pic.jpg/500x900.jpg');
+      doCheck(tester, 250, 450, true, hasAnimatedVersion: false,
+        'https://chat.example/user_uploads/thumbnail/1/2/a/pic.jpg/500x900.jpg');
+      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');
+      doCheck(tester, 300, 250, false, hasAnimatedVersion: false,
+        'https://chat.example/user_uploads/thumbnail/1/2/a/pic.jpg/840x560.webp');
+      doCheck(tester, 750, 1000, false, hasAnimatedVersion: false,
+        'https://chat.example/user_uploads/thumbnail/1/2/a/pic.jpg/1000x2000.png');
+    });
+  });
 }