-
Notifications
You must be signed in to change notification settings - Fork 428
content: Support inline images using the  syntax #2067
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1163,6 +1163,10 @@ class _InlineContentBuilder { | |
| return WidgetSpan(alignment: PlaceholderAlignment.middle, | ||
| child: MessageImageEmoji(node: node)); | ||
|
|
||
| case InlineImageNode(): | ||
| return WidgetSpan(alignment: PlaceholderAlignment.middle, | ||
| child: InlineImage(node: node, ambientTextStyle: widget.style)); | ||
|
|
||
| case MathInlineNode(): | ||
| final nodes = node.nodes; | ||
| return nodes == null | ||
|
|
@@ -1320,6 +1324,114 @@ class MessageImageEmoji extends StatelessWidget { | |
| } | ||
| } | ||
|
|
||
| class InlineImage extends StatelessWidget { | ||
| const InlineImage({ | ||
| super.key, | ||
| required this.node, | ||
| required this.ambientTextStyle, | ||
| }); | ||
|
|
||
| final InlineImageNode node; | ||
| final TextStyle ambientTextStyle; | ||
|
|
||
| Widget _buildContent(BuildContext context, {required Size size}) { | ||
| final store = PerAccountStoreWidget.of(context); | ||
|
|
||
| if (node.loading) { | ||
| return CupertinoActivityIndicator(); | ||
| } | ||
|
|
||
| final src = node.src; | ||
| final resolvedSrc = switch (src) { | ||
| ImageNodeSrcThumbnail() => src.value.resolve(context, | ||
| width: size.width, | ||
| height: size.height, | ||
| animationMode: .animateConditionally), | ||
| ImageNodeSrcOther() => store.tryResolveUrl(src.value), | ||
| }; | ||
| if (resolvedSrc == null) { | ||
| // TODO(#265) use an error-case placeholder here | ||
| return SizedBox.shrink(); | ||
| } | ||
|
|
||
| Widget result; | ||
| result = RealmContentNetworkImage( | ||
| // TODO(#265) use an error-case placeholder for `errorBuilder` | ||
| semanticLabel: node.alt, | ||
| resolvedSrc); | ||
|
|
||
| final resolvedOriginalSrc = node.originalSrc == null ? null | ||
| : store.tryResolveUrl(node.originalSrc!); | ||
| if (resolvedOriginalSrc != null) { | ||
| result = GestureDetector( | ||
| onTap: () { | ||
| Navigator.of(context).push(getImageLightboxRoute( | ||
| context: context, | ||
| message: InheritedMessage.of(context), | ||
| messageImageContext: context, | ||
| src: resolvedOriginalSrc, | ||
| thumbnailUrl: resolvedSrc, | ||
| originalWidth: node.originalWidth, | ||
| originalHeight: node.originalHeight)); | ||
| }, | ||
| child: LightboxHero( | ||
| messageImageContext: context, | ||
| src: resolvedOriginalSrc, | ||
|
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. Aha — so the issue with the hero here was that it needed |
||
| child: result)); | ||
| } | ||
|
|
||
| return result; | ||
| } | ||
|
|
||
| @override | ||
| Widget build(BuildContext context) { | ||
| final devicePixelRatio = MediaQuery.devicePixelRatioOf(context); | ||
|
|
||
| // Follow web's max-height behavior (10em); | ||
| // see image_box_em in web/src/postprocess_content.ts. | ||
| final maxHeight = ambientTextStyle.fontSize! * 10; | ||
|
|
||
| final imageSize = (node.originalWidth != null && node.originalHeight != null) | ||
| ? Size(node.originalWidth!, node.originalHeight!) / devicePixelRatio | ||
| // Layout plan when original dimensions are unknown: | ||
| // a [MessageMediaContainer]-sized and -colored rectangle. | ||
| : Size(MessageMediaContainer.width, MessageMediaContainer.height); | ||
|
|
||
| // (a) Don't let tall, thin images take up too much vertical space, | ||
| // which could be annoying to scroll through. And: | ||
| // (b) Don't let small images grow to occupy more physical pixels | ||
| // than they have data for. | ||
| // It looks like web has code for this in web/src/postprocess_content.ts | ||
| // but it doesn't account for the device pixel ratio, in 2026-01. | ||
| // So in web, small images do get blown up and blurry on modern devices: | ||
| // https://chat.zulip.org/#narrow/channel/101-design/topic/Inline.20images.20blown.20up.20and.20blurry/near/2346831 | ||
| final size = BoxConstraints(maxHeight: maxHeight) | ||
| .constrainSizeAndAttemptToPreserveAspectRatio(imageSize); | ||
|
|
||
| Widget child = _buildContent(context, size: size); | ||
|
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 version is missing a ColoredBox, no?
Collaborator
Author
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. Eep, yes, thanks for the catch. |
||
| if (node.alt != null) { | ||
| child = Tooltip( | ||
| message: node.alt, | ||
| // (Instead of setting a semantics label here, | ||
| // we give the alt text to [RealmContentNetworkImage].) | ||
| excludeFromSemantics: true, | ||
| child: child); | ||
| } | ||
|
|
||
| return Padding( | ||
| // Separate images vertically when they flow onto separate lines. | ||
| // (3px follows web; see web/styles/rendered_markdown.css.) | ||
| padding: const EdgeInsets.only(top: 3), | ||
| child: ConstrainedBox( | ||
| constraints: BoxConstraints.loose(size), | ||
| child: AspectRatio( | ||
| aspectRatio: size.aspectRatio, | ||
| child: ColoredBox( | ||
| color: ContentTheme.of(context).colorMessageMediaContainerBackground, | ||
| child: child)))); | ||
| } | ||
| } | ||
|
|
||
| class GlobalTime extends StatelessWidget { | ||
| const GlobalTime({ | ||
| super.key, | ||
|
|
||
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.
Perhaps as a followup PR, so while this is still fresh in mind but without blocking us merging and releasing the feature: this InlineImage is doing very nearly the same things as the MessageImagePreview widget, especially here in the _buildContent method, but the code looks more different than it is. It'd be nice to refactor them to look more similar so that the real differences are easier to spot.
Ideally, the logic here in _buildContent would become a helper widget that's shared between InlineImage and MessageImagePreview.