Skip to content

Commit b94e0fb

Browse files
chrisbobbechrisbobbe
committed
content: Use RealmStore.serverThumbnailFormats for thumbnails
Fixes #1936.
1 parent 3c3ef40 commit b94e0fb

File tree

5 files changed

+201
-4
lines changed

5 files changed

+201
-4
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
/// It may not work without adding authentication credentials to the request.
558560
///
559561
/// This will be null if the server hasn't yet generated a thumbnail,
@@ -599,6 +601,9 @@ class ImagePreviewNode extends BlockContentNode {
599601

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

‎lib/model/realm.dart‎

Lines changed: 40 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,26 @@ 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 resolution, ascending.
474+
///
475+
/// "Max resolution" means
476+
/// [ThumbnailFormat.maxWidth] * [ThumbnailFormat.maxHeight].
477+
static int _compareThumbnailFormats(ThumbnailFormat a, ThumbnailFormat b) {
478+
final aMaxResolution = a.maxWidth * a.maxHeight;
479+
final bMaxResolution = b.maxWidth * b.maxHeight;
480+
return aMaxResolution - bMaxResolution;
481+
}
482+
443483
void handleCustomProfileFieldsEvent(CustomProfileFieldsEvent event) {
444484
customProfileFields = _sortCustomProfileFields(event.fields);
445485
}

‎lib/widgets/content.dart‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -637,11 +637,12 @@ 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: 150, height: 100,
645+
animationMode: ImageAnimationMode.animateConditionally);
645646

646647
// TODO if src fails to parse, show an explicit "broken image"
647648

‎lib/widgets/image.dart‎

Lines changed: 53 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,54 @@ 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+
/// Requires an ancestor [PerAccountStoreWidget].
160+
Uri? resolve(
161+
BuildContext context, {
162+
required int width,
163+
required int height,
164+
required ImageAnimationMode animationMode,
165+
}) {
166+
final store = PerAccountStoreWidget.of(context);
167+
ThumbnailFormat? bestCandidate;
168+
169+
final animateIfSupported = animationMode.resolve(context);
170+
if (hasAnimatedVersion && animateIfSupported) {
171+
bestCandidate ??= _bestFormatOf(store.sortedAnimatedThumbnailFormats,
172+
width: width, height: height);
173+
}
174+
175+
bestCandidate ??= _bestFormatOf(store.sortedStillThumbnailFormats,
176+
width: width, height: height);
177+
178+
179+
if (bestCandidate == null) {
180+
// Odd if we'd need to fall back to the format encoded in [locator]'s path.
181+
// Seems theoretically possible though:
182+
// maybe this format isn't used now, for new uploads,
183+
// but it was used in the past, including for this image.
184+
return store.realmUrl.replace(path: urlPath);
185+
}
186+
187+
final lastSlashIndex = urlPath.lastIndexOf('/');
188+
return store.realmUrl.replace(
189+
path: '${urlPath.substring(0, lastSlashIndex)}/${bestCandidate.name}');
190+
}
191+
192+
ThumbnailFormat? _bestFormatOf(
193+
List<ThumbnailFormat> sortedCandidates, {
194+
required int width,
195+
required int height,
196+
}) {
197+
ThumbnailFormat? result;
198+
for (final candidate in sortedCandidates) {
199+
result = candidate;
200+
if (candidate.maxWidth >= width && candidate.maxHeight >= height) break;
201+
}
202+
return result;
203+
}
204+
}

‎test/widgets/image_test.dart‎

Lines changed: 98 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,97 @@ 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: '500x900.jpg',
72+
maxWidth: 500, maxHeight: 900, animated: false, format: 'jpg'),
73+
ThumbnailFormat(name: '500x900-anim.jpg',
74+
maxWidth: 500, maxHeight: 900, 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+
int width,
99+
int height,
100+
bool animatedIfSupported,
101+
String expected, {
102+
required bool hasAnimatedVersion,
103+
}) {
104+
final locator = ImageThumbnailLocator(
105+
urlPath: '/user_uploads/thumbnail/1/2/a/pic.jpg/840x560.webp',
106+
hasAnimatedVersion: hasAnimatedVersion,
107+
);
108+
109+
final context = tester.element(find.byType(Placeholder));
110+
final result = locator.resolve(context,
111+
width: width, height: height,
112+
animationMode: animatedIfSupported
113+
? ImageAnimationMode.animateAlways
114+
: ImageAnimationMode.animateNever);
115+
check(result.toString()).equals(expected);
116+
}
117+
118+
testWidgets('animated version exists', (tester) async {
119+
await prepare(tester);
120+
121+
doCheck(tester, 400, 400, false, hasAnimatedVersion: true,
122+
'https://chat.example/user_uploads/thumbnail/1/2/a/pic.jpg/500x900.jpg');
123+
doCheck(tester, 500, 900, true, hasAnimatedVersion: true,
124+
'https://chat.example/user_uploads/thumbnail/1/2/a/pic.jpg/500x900-anim.jpg');
125+
doCheck(tester, 500, 900, false, hasAnimatedVersion: true,
126+
'https://chat.example/user_uploads/thumbnail/1/2/a/pic.jpg/500x900.jpg');
127+
doCheck(tester, 600, 500, true, hasAnimatedVersion: true,
128+
'https://chat.example/user_uploads/thumbnail/1/2/a/pic.jpg/840x560-anim.webp');
129+
doCheck(tester, 600, 500, false, hasAnimatedVersion: true,
130+
'https://chat.example/user_uploads/thumbnail/1/2/a/pic.jpg/840x560.webp');
131+
doCheck(tester, 1500, 2000, false, hasAnimatedVersion: true,
132+
'https://chat.example/user_uploads/thumbnail/1/2/a/pic.jpg/1000x2000.png');
133+
});
134+
135+
testWidgets('animated version does not exist', (tester) async {
136+
await prepare(tester);
137+
138+
doCheck(tester, 400, 400, false, hasAnimatedVersion: false,
139+
'https://chat.example/user_uploads/thumbnail/1/2/a/pic.jpg/500x900.jpg');
140+
doCheck(tester, 500, 900, true, hasAnimatedVersion: false,
141+
'https://chat.example/user_uploads/thumbnail/1/2/a/pic.jpg/500x900.jpg');
142+
doCheck(tester, 500, 900, false, hasAnimatedVersion: false,
143+
'https://chat.example/user_uploads/thumbnail/1/2/a/pic.jpg/500x900.jpg');
144+
doCheck(tester, 600, 500, true, hasAnimatedVersion: false,
145+
'https://chat.example/user_uploads/thumbnail/1/2/a/pic.jpg/840x560.webp');
146+
doCheck(tester, 600, 500, false, hasAnimatedVersion: false,
147+
'https://chat.example/user_uploads/thumbnail/1/2/a/pic.jpg/840x560.webp');
148+
doCheck(tester, 1500, 2000, false, hasAnimatedVersion: false,
149+
'https://chat.example/user_uploads/thumbnail/1/2/a/pic.jpg/1000x2000.png');
150+
});
151+
});
54152
}

0 commit comments

Comments
 (0)