Skip to content

Commit c7bb1d8

Browse files
chrisbobbechrisbobbe
committed
content: Use RealmStore.serverThumbnailFormats for thumbnails
Fixes #1936.
1 parent f950f0c commit c7bb1d8

File tree

6 files changed

+221
-22
lines changed

6 files changed

+221
-22
lines changed

‎lib/model/content.dart‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import 'package:html/parser.dart';
55

66
import '../api/model/model.dart';
77
import '../api/model/submessage.dart';
8+
import '../widgets/image.dart';
89
import 'code_block.dart';
910
import 'katex.dart';
1011

@@ -553,7 +554,8 @@ class ImagePreviewNode extends BlockContentNode {
553554

554555
/// The thumbnail URL of the image and whether it has an animated version.
555556
///
556-
/// [ImageThumbnailLocator.urlPath] is a relative URL string.
557+
/// Use [ImageThumbnailLocatorExtension.resolve] to obtain a suitable URL
558+
/// for the current UI need.
557559
///
558560
/// This will be null if the server hasn't yet generated a thumbnail,
559561
/// or is a version that doesn't offer thumbnails.
@@ -598,6 +600,9 @@ class ImagePreviewNode extends BlockContentNode {
598600

599601
/// Data to locate an image thumbnail,
600602
/// and whether the image has an animated version.
603+
///
604+
/// Use [ImageThumbnailLocatorExtension.resolve] to obtain a suitable URL
605+
/// for the current UI need.
601606
@immutable
602607
class ImageThumbnailLocator extends DiagnosticableTree {
603608
ImageThumbnailLocator({

‎lib/model/realm.dart‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,12 @@ mixin RealmStore on PerAccountStoreBase, UserGroupStore {
3333
int get serverTypingStartedWaitPeriodMilliseconds;
3434

3535
List<ThumbnailFormat> get serverThumbnailFormats;
36+
/// A digest of [serverThumbnailFormats]: sorted by max resolution, ascending,
37+
/// and filtered to those with `animated: true`.
38+
List<ThumbnailFormat> get sortedAnimatedThumbnailFormats;
39+
/// A digest of [serverThumbnailFormats]: sorted by max resolution, ascending,
40+
/// and filtered to those with `animated: false`.
41+
List<ThumbnailFormat> get sortedStillThumbnailFormats;
3642

3743
//|//////////////////////////////////////////////////////////////
3844
// Realm settings.
@@ -170,6 +176,10 @@ mixin ProxyRealmStore on RealmStore {
170176
@override
171177
List<ThumbnailFormat> get serverThumbnailFormats => realmStore.serverThumbnailFormats;
172178
@override
179+
List<ThumbnailFormat> get sortedAnimatedThumbnailFormats => realmStore.sortedAnimatedThumbnailFormats;
180+
@override
181+
List<ThumbnailFormat> get sortedStillThumbnailFormats => realmStore.sortedStillThumbnailFormats;
182+
@override
173183
bool get realmAllowMessageEditing => realmStore.realmAllowMessageEditing;
174184
@override
175185
GroupSettingValue? get realmCanDeleteAnyMessageGroup => realmStore.realmCanDeleteAnyMessageGroup;
@@ -235,6 +245,10 @@ class RealmStoreImpl extends HasUserGroupStore with RealmStore {
235245
serverTypingStoppedWaitPeriodMilliseconds = initialSnapshot.serverTypingStoppedWaitPeriodMilliseconds,
236246
serverTypingStartedWaitPeriodMilliseconds = initialSnapshot.serverTypingStartedWaitPeriodMilliseconds,
237247
serverThumbnailFormats = initialSnapshot.serverThumbnailFormats,
248+
_sortedAnimatedThumbnailFormats = _filterAndSortThumbnailFormats(
249+
initialSnapshot.serverThumbnailFormats, animated: true),
250+
_sortedStillThumbnailFormats = _filterAndSortThumbnailFormats(
251+
initialSnapshot.serverThumbnailFormats, animated: false),
238252
realmAllowMessageEditing = initialSnapshot.realmAllowMessageEditing,
239253
realmCanDeleteAnyMessageGroup = initialSnapshot.realmCanDeleteAnyMessageGroup,
240254
realmCanDeleteOwnMessageGroup = initialSnapshot.realmCanDeleteOwnMessageGroup,
@@ -381,6 +395,12 @@ class RealmStoreImpl extends HasUserGroupStore with RealmStore {
381395

382396
@override
383397
final List<ThumbnailFormat> serverThumbnailFormats;
398+
@override
399+
List<ThumbnailFormat> get sortedAnimatedThumbnailFormats => _sortedAnimatedThumbnailFormats;
400+
final List<ThumbnailFormat> _sortedAnimatedThumbnailFormats;
401+
@override
402+
List<ThumbnailFormat> get sortedStillThumbnailFormats => _sortedStillThumbnailFormats;
403+
final List<ThumbnailFormat> _sortedStillThumbnailFormats;
384404

385405
@override
386406
final bool realmAllowMessageEditing;
@@ -440,6 +460,23 @@ class RealmStoreImpl extends HasUserGroupStore with RealmStore {
440460
return displayFields.followedBy(nonDisplayFields).toList();
441461
}
442462

463+
static List<ThumbnailFormat> _filterAndSortThumbnailFormats(
464+
List<ThumbnailFormat> initialServerThumbnailFormats, {
465+
required bool animated,
466+
}) {
467+
return initialServerThumbnailFormats
468+
.where((format) => format.animated == animated)
469+
.toList()
470+
..sort(_compareThumbnailFormats);
471+
}
472+
473+
/// A comparator to sort formats by max-width plus max-height, ascending.
474+
static int _compareThumbnailFormats(ThumbnailFormat a, ThumbnailFormat b) {
475+
final aValue = a.maxWidth + a.maxHeight;
476+
final bValue = b.maxWidth + b.maxHeight;
477+
return aValue.compareTo(bValue);
478+
}
479+
443480
void handleCustomProfileFieldsEvent(CustomProfileFieldsEvent event) {
444481
customProfileFields = _sortCustomProfileFields(event.fields);
445482
}

‎lib/widgets/content.dart‎

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -637,11 +637,13 @@ class MessageImagePreview extends StatelessWidget {
637637

638638
// TODO image hover animation
639639
final srcUrl = node.srcUrl;
640-
final thumbnailUrl = node.thumbnail?.urlPath;
640+
final thumbnailLocator = node.thumbnail;
641641
final store = PerAccountStoreWidget.of(context);
642642
final resolvedSrcUrl = store.tryResolveUrl(srcUrl);
643-
final resolvedThumbnailUrl = thumbnailUrl == null
644-
? null : store.tryResolveUrl(thumbnailUrl);
643+
final resolvedThumbnailUrl = thumbnailLocator?.resolve(context,
644+
width: MessageMediaContainer.width,
645+
height: MessageMediaContainer.height,
646+
animationMode: ImageAnimationMode.animateConditionally);
645647

646648
// TODO if src fails to parse, show an explicit "broken image"
647649

@@ -736,6 +738,12 @@ class MessageMediaContainer extends StatelessWidget {
736738
final void Function()? onTap;
737739
final Widget? child;
738740

741+
/// The container's width, in logical pixels.
742+
static const width = 150.0;
743+
744+
/// The container's height, in logical pixels.
745+
static const height = 100.0;
746+
739747
@override
740748
Widget build(BuildContext context) {
741749
return GestureDetector(
@@ -751,8 +759,8 @@ class MessageMediaContainer extends StatelessWidget {
751759
child: Padding(
752760
padding: const EdgeInsets.all(1),
753761
child: SizedBox(
754-
height: 100,
755-
width: 150,
762+
width: width,
763+
height: height,
756764
child: child))))));
757765
}
758766
}

‎lib/widgets/image.dart‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@ import 'package:flutter/foundation.dart';
22
import 'package:flutter/widgets.dart';
33

44
import '../api/core.dart';
5+
import '../api/model/initial_snapshot.dart';
6+
import '../model/content.dart';
57
import 'store.dart';
68

79
/// Like [Image.network], but includes [authHeader] if [src] is on-realm.
@@ -149,3 +151,57 @@ enum ImageAnimationMode {
149151
}
150152
}
151153
}
154+
155+
extension ImageThumbnailLocatorExtension on ImageThumbnailLocator {
156+
/// Chooses an appropriate format from [PerAccountStore.serverThumbnailFormats],
157+
/// represented as an absolute URL.
158+
///
159+
/// [height] and [width] are in logical pixels.
160+
///
161+
/// Requires an ancestor [PerAccountStoreWidget].
162+
Uri? resolve(
163+
BuildContext context, {
164+
required double width,
165+
required double height,
166+
required ImageAnimationMode animationMode,
167+
}) {
168+
final devicePixelRatio = MediaQuery.devicePixelRatioOf(context);
169+
final widthPhysicalPx = (width * devicePixelRatio).ceil();
170+
final heightPhysicalPx = (height * devicePixelRatio).ceil();
171+
172+
final store = PerAccountStoreWidget.of(context);
173+
174+
ThumbnailFormat? bestCandidate;
175+
if (animated && animationMode.shouldAnimate(context)) {
176+
bestCandidate ??= _bestFormatOf(store.sortedAnimatedThumbnailFormats,
177+
width: widthPhysicalPx, height: heightPhysicalPx);
178+
}
179+
bestCandidate ??= _bestFormatOf(store.sortedStillThumbnailFormats,
180+
width: widthPhysicalPx, height: heightPhysicalPx);
181+
182+
if (bestCandidate == null) {
183+
// Odd if we'd need to fall back to the format encoded in [locator]'s path.
184+
// Seems theoretically possible though:
185+
// maybe this format isn't used now, for new uploads,
186+
// but it was used in the past, including for this image.
187+
return store.realmUrl.replace(path: urlPath);
188+
}
189+
190+
final lastSlashIndex = urlPath.lastIndexOf('/');
191+
return store.realmUrl.replace(
192+
path: '${urlPath.substring(0, lastSlashIndex)}/${bestCandidate.name}');
193+
}
194+
195+
ThumbnailFormat? _bestFormatOf(
196+
List<ThumbnailFormat> sortedCandidates, {
197+
required int width,
198+
required int height,
199+
}) {
200+
ThumbnailFormat? result;
201+
for (final candidate in sortedCandidates) {
202+
result = candidate;
203+
if (candidate.maxWidth >= width && candidate.maxHeight >= height) break;
204+
}
205+
return result;
206+
}
207+
}

‎pubspec.lock‎

Lines changed: 8 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -622,14 +622,6 @@ packages:
622622
url: "https://pub.dev"
623623
source: hosted
624624
version: "1.0.5"
625-
js:
626-
dependency: transitive
627-
description:
628-
name: js
629-
sha256: "53385261521cc4a0c4658fd0ad07a7d14591cf8fc33abbceae306ddb974888dc"
630-
url: "https://pub.dev"
631-
source: hosted
632-
version: "0.7.2"
633625
json_annotation:
634626
dependency: "direct main"
635627
description:
@@ -698,18 +690,18 @@ packages:
698690
dependency: transitive
699691
description:
700692
name: matcher
701-
sha256: dc58c723c3c24bf8d3e2d3ad3f2f9d7bd9cf43ec6feaa64181775e60190153f2
693+
sha256: "12956d0ad8390bbcc63ca2e1469c0619946ccb52809807067a7020d57e647aa6"
702694
url: "https://pub.dev"
703695
source: hosted
704-
version: "0.12.17"
696+
version: "0.12.18"
705697
material_color_utilities:
706698
dependency: transitive
707699
description:
708700
name: material_color_utilities
709-
sha256: f7142bb1154231d7ea5f96bc7bde4bda2a0945d2806bb11670e30b850d56bdec
701+
sha256: "9c337007e82b1889149c82ed242ed1cb24a66044e30979c44912381e9be4c48b"
710702
url: "https://pub.dev"
711703
source: hosted
712-
version: "0.11.1"
704+
version: "0.13.0"
713705
meta:
714706
dependency: transitive
715707
description:
@@ -1079,10 +1071,10 @@ packages:
10791071
dependency: "direct dev"
10801072
description:
10811073
name: test
1082-
sha256: "8f0eb7fa76b7d05a4f3707e0dbd581babef5b0915ca508b757cf15d0cabb56cb"
1074+
sha256: "77cc98ea27006c84e71a7356cf3daf9ddbde2d91d84f77dbfe64cf0e4d9611ae"
10831075
url: "https://pub.dev"
10841076
source: hosted
1085-
version: "1.27.0"
1077+
version: "1.28.0"
10861078
test_api:
10871079
dependency: "direct dev"
10881080
description:
@@ -1095,10 +1087,10 @@ packages:
10951087
dependency: transitive
10961088
description:
10971089
name: test_core
1098-
sha256: bad9916601a4f2ef6e4dbc466fb712e4b42cf4917c3fd428b018f51984fce13b
1090+
sha256: f1072617a6657e5fc09662e721307f7fb009b4ed89b19f47175d11d5254a62d4
10991091
url: "https://pub.dev"
11001092
source: hosted
1101-
version: "0.6.13"
1093+
version: "0.6.14"
11021094
timing:
11031095
dependency: transitive
11041096
description:

‎test/widgets/image_test.dart‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,17 @@ import 'package:checks/checks.dart';
22
import 'package:flutter/widgets.dart';
33
import 'package:flutter_test/flutter_test.dart';
44
import 'package:zulip/api/core.dart';
5+
import 'package:zulip/api/model/initial_snapshot.dart';
6+
import 'package:zulip/model/content.dart';
7+
import 'package:zulip/model/store.dart';
58
import 'package:zulip/widgets/image.dart';
69
import 'package:zulip/widgets/store.dart';
710

811
import '../example_data.dart' as eg;
912
import '../model/binding.dart';
13+
import '../model/test_store.dart';
1014
import '../test_images.dart';
15+
import 'test_app.dart';
1116

1217
void main() {
1318
TestZulipBinding.ensureInitialized();
@@ -51,4 +56,100 @@ void main() {
5156
check(tester.takeException()).isA<AssertionError>();
5257
});
5358
});
59+
60+
group('ImageThumbnailLocator.resolve', () {
61+
late PerAccountStore store;
62+
63+
Future<void> prepare(WidgetTester tester) async {
64+
addTearDown(testBinding.reset);
65+
66+
final exampleFormats = [
67+
ThumbnailFormat(name: '840x560.webp',
68+
maxWidth: 840, maxHeight: 560, animated: false, format: 'webp'),
69+
ThumbnailFormat(name: '840x560-anim.webp',
70+
maxWidth: 840, maxHeight: 560, animated: true, format: 'webp'),
71+
ThumbnailFormat(name: '500x850.jpg',
72+
maxWidth: 500, maxHeight: 850, animated: false, format: 'jpg'),
73+
ThumbnailFormat(name: '500x850-anim.jpg',
74+
maxWidth: 500, maxHeight: 850, animated: true, format: 'jpg'),
75+
ThumbnailFormat(name: '1000x1000.webp',
76+
maxWidth: 1000, maxHeight: 1000, animated: false, format: 'webp'),
77+
ThumbnailFormat(name: '1000x2000-anim.png',
78+
maxWidth: 1000, maxHeight: 2000, animated: true, format: 'png'),
79+
ThumbnailFormat(name: '1000x1000-anim.webp',
80+
maxWidth: 1000, maxHeight: 1000, animated: true, format: 'webp'),
81+
ThumbnailFormat(name: '1000x2000.png',
82+
maxWidth: 1000, maxHeight: 2000, animated: false, format: 'png'),
83+
];
84+
85+
await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot(
86+
serverThumbnailFormats: exampleFormats,
87+
));
88+
89+
store = await testBinding.globalStore.perAccount(eg.selfAccount.id);
90+
await store.addUser(eg.selfUser);
91+
92+
await tester.pumpWidget(TestZulipApp(accountId: eg.selfAccount.id));
93+
await tester.pump();
94+
}
95+
96+
void doCheck(
97+
WidgetTester tester,
98+
double width,
99+
double height,
100+
bool animateIfSupported,
101+
String expected, {
102+
required bool animated,
103+
}) {
104+
final locator = ImageThumbnailLocator(
105+
urlPath: '/user_uploads/thumbnail/1/2/a/pic.jpg/840x560.webp',
106+
animated: animated,
107+
);
108+
109+
final context = tester.element(find.byType(Placeholder));
110+
final result = locator.resolve(context,
111+
width: width, height: height,
112+
animationMode: animateIfSupported
113+
? ImageAnimationMode.animateAlways
114+
: ImageAnimationMode.animateNever);
115+
check(result.toString()).equals(expected);
116+
}
117+
118+
testWidgets('animated version does not exist', (tester) async {
119+
tester.view.devicePixelRatio = 2.0;
120+
addTearDown(tester.view.resetDevicePixelRatio);
121+
122+
await prepare(tester);
123+
124+
doCheck(tester, 200, 200, false, animated: false,
125+
'https://chat.example/user_uploads/thumbnail/1/2/a/pic.jpg/500x850.jpg');
126+
doCheck(tester, 250, 425, true, animated: false,
127+
'https://chat.example/user_uploads/thumbnail/1/2/a/pic.jpg/500x850.jpg');
128+
doCheck(tester, 250, 425, false, animated: false,
129+
'https://chat.example/user_uploads/thumbnail/1/2/a/pic.jpg/500x850.jpg');
130+
// Different output from previous because it's is in physical pixels.
131+
doCheck(tester, 300, 250, true, animated: false,
132+
'https://chat.example/user_uploads/thumbnail/1/2/a/pic.jpg/840x560.webp');
133+
doCheck(tester, 300, 250, false, animated: false,
134+
'https://chat.example/user_uploads/thumbnail/1/2/a/pic.jpg/840x560.webp');
135+
doCheck(tester, 750, 1000, false, animated: false,
136+
'https://chat.example/user_uploads/thumbnail/1/2/a/pic.jpg/1000x2000.png');
137+
});
138+
139+
testWidgets('animated version exists', (tester) async {
140+
tester.view.devicePixelRatio = 2.0;
141+
addTearDown(tester.view.resetDevicePixelRatio);
142+
143+
await prepare(tester);
144+
145+
doCheck(tester, 200, 200, false, animated: true,
146+
'https://chat.example/user_uploads/thumbnail/1/2/a/pic.jpg/500x850.jpg');
147+
doCheck(tester, 250, 425, true, animated: true,
148+
'https://chat.example/user_uploads/thumbnail/1/2/a/pic.jpg/500x850-anim.jpg');
149+
doCheck(tester, 250, 425, false, animated: true,
150+
'https://chat.example/user_uploads/thumbnail/1/2/a/pic.jpg/500x850.jpg');
151+
doCheck(tester, 750, 1000, false, animated: true,
152+
'https://chat.example/user_uploads/thumbnail/1/2/a/pic.jpg/1000x2000.png');
153+
});
154+
});
54155
}

0 commit comments

Comments
 (0)