-
Notifications
You must be signed in to change notification settings - Fork 373
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
d6bb85f
634439d
22d589a
c26dbb8
2a11e04
6885cc1
a5b2ef0
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,12 @@ class ImagePreviewNode extends BlockContentNode { | |
| /// authentication credentials to the request. | ||
| final String srcUrl; | ||
|
|
||
| /// The thumbnail URL of the image. | ||
| /// | ||
| /// This may be a relative URL string. It also may not work without adding | ||
| /// authentication credentials to the request. | ||
| /// The thumbnail URL of the image and whether it has an animated version. | ||
| /// | ||
| /// 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; | ||
|
|
||
| /// A flag to indicate whether to show the placeholder. | ||
| /// | ||
|
|
@@ -576,27 +574,69 @@ 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.defaultFormatSrc, | ||
| required this.animated, | ||
| }) : assert(!defaultFormatSrc.isAbsolute), | ||
|
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. This has a surprising semantics. From the implementation: bool get isAbsolute => hasScheme && !hasFragment;Presumably that matches what some old RFC says "absolute" means. But if we expand this out to |
||
| assert(defaultFormatSrc.path.startsWith(srcPrefix)); | ||
|
|
||
| /// A relative URL for the default format, starting with [srcPrefix]. | ||
| /// | ||
| /// It may not work without adding authentication credentials to the request. | ||
| final Uri defaultFormatSrc; | ||
|
|
||
| final bool animated; | ||
|
|
||
| static const srcPrefix = '/user_uploads/thumbnail/'; | ||
|
|
||
| @override | ||
| bool operator ==(Object other) { | ||
| if (other is! ImageThumbnailLocator) return false; | ||
| return defaultFormatSrc == other.defaultFormatSrc | ||
| && animated == other.animated; | ||
| } | ||
|
|
||
| @override | ||
| int get hashCode => Object.hash('ImageThumbnailLocator', defaultFormatSrc, animated); | ||
|
|
||
| @override | ||
| void debugFillProperties(DiagnosticPropertiesBuilder properties) { | ||
| super.debugFillProperties(properties); | ||
| properties.add(StringProperty('defaultFormatSrc', defaultFormatSrc.toString())); | ||
| properties.add(FlagProperty('animated', value: animated, | ||
| ifTrue: 'animated', | ||
| ifFalse: 'not animated')); | ||
| } | ||
| } | ||
|
|
||
| class InlineVideoNode extends BlockContentNode { | ||
| const InlineVideoNode({ | ||
| super.debugHtmlNode, | ||
|
|
@@ -1399,7 +1439,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 +1451,24 @@ class _ZulipContentParser { | |
| } | ||
|
|
||
| final String srcUrl; | ||
| final String? thumbnailUrl; | ||
| if (src.startsWith('/user_uploads/thumbnail/')) { | ||
| final ImageThumbnailLocator? thumbnail; | ||
| if (src.startsWith(ImageThumbnailLocator.srcPrefix)) { | ||
| final parsedSrc = Uri.tryParse(src); | ||
| if (parsedSrc == null) return UnimplementedBlockContentNode(htmlNode: divElement); | ||
|
|
||
| // 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( | ||
| defaultFormatSrc: parsedSrc, | ||
| animated: 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 +1492,7 @@ class _ZulipContentParser { | |
|
|
||
| return ImagePreviewNode( | ||
| srcUrl: srcUrl, | ||
| thumbnailUrl: thumbnailUrl, | ||
| thumbnail: thumbnail, | ||
| loading: false, | ||
| originalWidth: originalWidth, | ||
| originalHeight: originalHeight, | ||
|
|
||
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.
I think it'd be helpful to split this commit:
urlPath.hasAnimatedVersion(oranimatedper another comment).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.
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.
Very helpful, thanks 🙂