-
Notifications
You must be signed in to change notification settings - Fork 354
content: Use RealmStore.serverThumbnailFormats for thumbnails #1987
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
aff2e37
75f1553
e715d12
7d51241
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -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'; | ||||||
|
|
||||||
|
|
@@ -539,7 +540,7 @@ class ImagePreviewNode extends BlockContentNode { | |||||
| const ImagePreviewNode({ | ||||||
| super.debugHtmlNode, | ||||||
| required this.srcUrl, | ||||||
| required this.thumbnailUrl, | ||||||
| required this.thumbnail, | ||||||
| required this.loading, | ||||||
| required this.originalWidth, | ||||||
| required this.originalHeight, | ||||||
|
|
@@ -551,15 +552,16 @@ class ImagePreviewNode extends BlockContentNode { | |||||
| /// authentication credentials to the request. | ||||||
| final String srcUrl; | ||||||
|
|
||||||
| /// The thumbnail URL of the image. | ||||||
| /// The thumbnail URL of the image and whether it has an animated version. | ||||||
| /// | ||||||
| /// This may be a relative URL string. It also may not work without adding | ||||||
| /// authentication credentials to the request. | ||||||
| /// 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, | ||||||
| /// or is a version that doesn't offer thumbnails. | ||||||
| /// It will also be null when [loading] is true. | ||||||
| final String? thumbnailUrl; | ||||||
| final ImageThumbnailLocator? thumbnail; | ||||||
|
Comment on lines
-562
to
+564
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it'd be helpful to split this commit:
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. |
||||||
|
|
||||||
| /// A flag to indicate whether to show the placeholder. | ||||||
| /// | ||||||
|
|
@@ -576,27 +578,64 @@ class ImagePreviewNode extends BlockContentNode { | |||||
| bool operator ==(Object other) { | ||||||
| return other is ImagePreviewNode | ||||||
| && other.srcUrl == srcUrl | ||||||
| && other.thumbnailUrl == thumbnailUrl | ||||||
| && other.thumbnail == thumbnail | ||||||
| && other.loading == loading | ||||||
| && other.originalWidth == originalWidth | ||||||
| && other.originalHeight == originalHeight; | ||||||
| } | ||||||
|
|
||||||
| @override | ||||||
| int get hashCode => Object.hash('ImagePreviewNode', | ||||||
| srcUrl, thumbnailUrl, loading, originalWidth, originalHeight); | ||||||
| srcUrl, thumbnail, loading, originalWidth, originalHeight); | ||||||
|
|
||||||
| @override | ||||||
| void debugFillProperties(DiagnosticPropertiesBuilder properties) { | ||||||
| super.debugFillProperties(properties); | ||||||
| properties.add(StringProperty('srcUrl', srcUrl)); | ||||||
| properties.add(StringProperty('thumbnailUrl', thumbnailUrl)); | ||||||
| properties.add(DiagnosticsProperty<ImageThumbnailLocator>('thumbnail', thumbnail)); | ||||||
| properties.add(FlagProperty('loading', value: loading, ifTrue: "is loading")); | ||||||
| properties.add(DoubleProperty('originalWidth', originalWidth)); | ||||||
| properties.add(DoubleProperty('originalHeight', originalHeight)); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /// 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({ | ||||||
| required this.urlPath, | ||||||
| required this.hasAnimatedVersion, | ||||||
| }) : assert(urlPath.startsWith(urlPathPrefix)); | ||||||
|
|
||||||
| final String urlPath; | ||||||
| final bool hasAnimatedVersion; | ||||||
|
|
||||||
| static const urlPathPrefix = '/user_uploads/thumbnail/'; | ||||||
|
|
||||||
| @override | ||||||
| bool operator ==(Object other) { | ||||||
| if (other is! ImageThumbnailLocator) return false; | ||||||
| return urlPath == other.urlPath | ||||||
| && hasAnimatedVersion == other.hasAnimatedVersion; | ||||||
| } | ||||||
|
|
||||||
| @override | ||||||
| int get hashCode => Object.hash(urlPath, hasAnimatedVersion); | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
|
||||||
|
|
||||||
| @override | ||||||
| void debugFillProperties(DiagnosticPropertiesBuilder properties) { | ||||||
| super.debugFillProperties(properties); | ||||||
| properties.add(StringProperty('urlPath', urlPath)); | ||||||
| properties.add(FlagProperty('hasAnimatedVersion', value: hasAnimatedVersion, | ||||||
| ifTrue: 'animatable', | ||||||
| ifFalse: 'not animatable')); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| class InlineVideoNode extends BlockContentNode { | ||||||
| const InlineVideoNode({ | ||||||
| super.debugHtmlNode, | ||||||
|
|
@@ -1399,7 +1438,7 @@ class _ZulipContentParser { | |||||
| if (imgElement.className == 'image-loading-placeholder') { | ||||||
| return ImagePreviewNode( | ||||||
| srcUrl: href, | ||||||
| thumbnailUrl: null, | ||||||
| thumbnail: null, | ||||||
| loading: true, | ||||||
| originalWidth: null, | ||||||
| originalHeight: null, | ||||||
|
|
@@ -1411,19 +1450,21 @@ class _ZulipContentParser { | |||||
| } | ||||||
|
|
||||||
| final String srcUrl; | ||||||
| final String? thumbnailUrl; | ||||||
| if (src.startsWith('/user_uploads/thumbnail/')) { | ||||||
| final ImageThumbnailLocator? thumbnail; | ||||||
| if (src.startsWith(ImageThumbnailLocator.urlPathPrefix)) { | ||||||
| // For why we recognize this as the thumbnail form, see discussion: | ||||||
| // https://chat.zulip.org/#narrow/channel/412-api-documentation/topic/documenting.20inline.20images/near/2279872 | ||||||
| srcUrl = href; | ||||||
| thumbnailUrl = src; | ||||||
| thumbnail = ImageThumbnailLocator( | ||||||
| urlPath: src, | ||||||
| hasAnimatedVersion: imgElement.attributes['data-animated'] == 'true'); | ||||||
| } else { | ||||||
| // Known cases this handles: | ||||||
| // - `src` starts with CAMO_URI, a server variable (e.g. on Zulip Cloud | ||||||
| // it's "https://uploads.zulipusercontent.net/" in 2025-10). | ||||||
| // - `src` matches `href`, e.g. from pre-thumbnailing servers. | ||||||
| srcUrl = src; | ||||||
| thumbnailUrl = null; | ||||||
| thumbnail = null; | ||||||
| } | ||||||
|
|
||||||
| double? originalWidth, originalHeight; | ||||||
|
|
@@ -1447,7 +1488,7 @@ class _ZulipContentParser { | |||||
|
|
||||||
| return ImagePreviewNode( | ||||||
| srcUrl: srcUrl, | ||||||
| thumbnailUrl: thumbnailUrl, | ||||||
| thumbnail: thumbnail, | ||||||
| loading: false, | ||||||
| originalWidth: originalWidth, | ||||||
| originalHeight: originalHeight, | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -32,6 +32,14 @@ mixin RealmStore on PerAccountStoreBase, UserGroupStore { | |||||
| Duration get serverTypingStartedWaitPeriod => Duration(milliseconds: serverTypingStartedWaitPeriodMilliseconds); | ||||||
| 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. | ||||||
|
|
||||||
|
|
@@ -166,6 +174,12 @@ mixin ProxyRealmStore on RealmStore { | |||||
| @override | ||||||
| int get serverTypingStartedWaitPeriodMilliseconds => realmStore.serverTypingStartedWaitPeriodMilliseconds; | ||||||
| @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; | ||||||
|
|
@@ -230,6 +244,11 @@ class RealmStoreImpl extends HasUserGroupStore with RealmStore { | |||||
| serverTypingStartedExpiryPeriodMilliseconds = initialSnapshot.serverTypingStartedExpiryPeriodMilliseconds, | ||||||
| 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, | ||||||
|
|
@@ -374,6 +393,15 @@ class RealmStoreImpl extends HasUserGroupStore with RealmStore { | |||||
| @override | ||||||
| final int serverTypingStartedWaitPeriodMilliseconds; | ||||||
|
|
||||||
| @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; | ||||||
| @override | ||||||
|
|
@@ -432,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; | ||||||
|
Comment on lines
+478
to
+479
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about + instead of *? That's cheaper to compute and not really any conceptually less simple; and it seems likely to give the exact same ordering on real sets of formats. |
||||||
| return aMaxResolution - bMaxResolution; | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Otherwise this would go wrong if the values differed by half or more of the max range of the int type. (Not likely for this particular data; but best to set an example of the pattern that is consistently correct.) |
||||||
| } | ||||||
|
|
||||||
| void handleCustomProfileFieldsEvent(CustomProfileFieldsEvent event) { | ||||||
| customProfileFields = _sortCustomProfileFields(event.fields); | ||||||
| } | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like information for the ImageThumbnailLocator doc.