-
Notifications
You must be signed in to change notification settings - Fork 384
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
base: main
Are you sure you want to change the base?
Conversation
|
cc @alya Showing a large image being shrunk, and center-aligned vertically in the flow of text:
Showing a small image: There's a discrepancy here, but it's web's bug: it blows up the image so that it uses more pixels than the image has data for, and it's blurry. I started a discussion: #design > Inline images blown up and blurry @ 💬
Showing images in a table: The table is wider than available space on mobile; I've included a second mobile screenshot where I scrolled the table to see the rest of it.
|
941e8c9 to
301415e
Compare
|
Revision pushed. Having seen @timabbott's comment zulip/zulip#36226 (comment) , I've relaxed the |
rajveermalviya
left a comment
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.
Thanks @chrisbobbe! All LGTM, and tests great on a local server (except I couldn't test the loading state, not sure if there's an easy way to do that).
Moving over to Greg's review.
lib/model/content.dart
Outdated
| //|////////////////////////////////////////////////////////////// | ||
| // Helpers for both [_ZulipInlineContentParser] and [_ZulipContentParser]. | ||
|
|
||
|
|
||
| final _imageDimensionsRegExp = RegExp(r'^(\d+)x(\d+)$'); |
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.
nit: put helpers above rather than below (that's how this parser code is organized in general)
lib/model/content.dart
Outdated
| final dimensionsMatch = _imageDimensionsRegExp.firstMatch(originalDimensions); | ||
| if (dimensionsMatch == null) return null; | ||
| final originalWidth = int.tryParse(dimensionsMatch.group(1)!, radix: 10)?.toDouble(); | ||
| final originalHeight = int.tryParse(dimensionsMatch.group(2)!, radix: 10)?.toDouble(); | ||
| if (originalWidth == null || originalHeight == null) return null; |
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 is some nontrivial logic that feels like it's doing very much the same thing as the other site where this regexp is used. Can that be factored out as a helper function?
(The helper can return an ad-hoc record type with width and height)
lib/model/content.dart
Outdated
| final originalSrc = imgElement.attributes['data-original-src']; | ||
| if (originalSrc == null) return null; |
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 we decided in the chat thread that these images might not be thumbnailed. This isn't yet…
… ah I see, this revision is from before we decided that 🙂. I'll hold off on reviewing further for now, then.
(see #api design > HTML pattern for truly inline images @ 💬)
The instance will always have a format we can fall back to, i.e., defaultFormatSrc, so there's no need for this method's return type to be nullable.
301415e to
8157751
Compare
|
Thanks! Revision pushed, and I've commented at #api design > HTML pattern for truly inline images @ 💬 on how the parser is more permissive of forms that current servers don't produce. |
| return InlineImageNode( | ||
| loading: loading, | ||
| src: thumbnailSrc != null | ||
| ? InlineImageNodeSrcThumbnail(thumbnailSrc) | ||
| : InlineImageNodeSrcOther(src), | ||
| alt: alt, | ||
| originalSrc: originalSrc, | ||
| originalWidth: originalWidth, |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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.
Oh I see, we do use the original size, naturally enough.
Also apparently alt; I'd missed that we use it in two places.
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.
we never look at any of these other fields on InlineImageNode, right?
We use data-original-dimensions, when present, to avoid layout shifts when transitioning out of the loading state. I think data-original-dimensions will still be present in the loading state, from current servers? Because current servers only intend to operate on uploaded images, and we know the dimensions of those. This is supported by the API doc in zulip/zulip#36226:
For image elements presented in Markdown syntax, this placeholder
structure is used:
```html
<img alt="example image"
class="inline-image image-loading-placeholder"
data-original-content-type="image/png"
data-original-dimensions="1050x700"
data-original-src="/user_uploads/path/to/example.png"
src="/path/to/spinner.png">
```| final dimensionsMatch = _imageDimensionsRegExp.firstMatch(originalDimensions); | ||
| if (dimensionsMatch != null) { | ||
| originalWidth = int.tryParse(dimensionsMatch.group(1)!, radix: 10)?.toDouble(); | ||
| originalHeight = int.tryParse(dimensionsMatch.group(2)!, radix: 10)?.toDouble(); | ||
| } |
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 is meant to use the new helper, right?
| final src = imgElement.attributes['src']; | ||
| if (src == null) return null; | ||
| final animated = imgElement.attributes['data-animated'] == 'true'; | ||
| ImageThumbnailLocator? thumbnailSrc; | ||
| if (src.startsWith(ImageThumbnailLocator.srcPrefix)) { | ||
| final srcUrl = Uri.tryParse(src); | ||
| if (srcUrl == null) return null; | ||
| thumbnailSrc = ImageThumbnailLocator(defaultFormatSrc: srcUrl, animated: animated); | ||
| } |
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.
And this is some other logic that seems very similar to some code we already have for the block images. It'd be good to either deduplicate, or if that's annoying for some reason then to adjust the two to be written as similarly as possible — that way one can look at the two of them and work out if there are any subtle discrepancies in behavior that we might not intend.
| class InlineImageNode extends InlineContentNode { | ||
| const InlineImageNode({ | ||
| super.debugHtmlNode, | ||
| required this.loading, | ||
| required this.src, | ||
| required this.alt, | ||
| required this.originalSrc, | ||
| required this.originalWidth, | ||
| required this.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.
Can we unify this with ImagePreviewNode, by giving them a common base class that supplies most of these fields? It seems like they have much the same data on them.
That might also help with unifying more of the parsing logic for them.
| // Don't let tall, thin images take up too much vertical space, | ||
| // which could be annoying to scroll through… | ||
| BoxConstraints(maxWidth: maxHeight * aspectRatio, maxHeight: maxHeight) |
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.
| // Don't let tall, thin images take up too much vertical space, | |
| // which could be annoying to scroll through… | |
| BoxConstraints(maxWidth: maxHeight * aspectRatio, maxHeight: maxHeight) | |
| // Don't let tall, thin images take up too much vertical space, | |
| // which could be annoying to scroll through… | |
| BoxConstraints(maxHeight: maxHeight) |
This seems like it matches the comment better. And I believe it has the exact same effect, producing the same result for the .enforce call below.







Fixes #1913.
This relies on an API guarantee that's implicit in the web PR zulip/zulip#36226, but I've left a PR review suggesting more explicitness: zulip/zulip#36226 (review) . In particular, this assumes that
srcin the inline-image HTML will be a thumbnail URL, i.e. one that starts with '/user_uploads/thumbnail/'.See also some followup issues:
Screenshots coming soon. 🙂