Skip to content

Multiple Images #486

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

Merged
merged 5 commits into from
Feb 13, 2024
Merged

Multiple Images #486

merged 5 commits into from
Feb 13, 2024

Conversation

sirpengi
Copy link
Contributor

@sirpengi sirpengi commented Jan 18, 2024

Rebased on top of #511 for new style of content tests.

Fixes: #193

Basic Multiple Images

[4-64ecbf0935ea5.png](/user_uploads/57965/lBu8Ccyg4Ex0wo07RZdYnpKg/4-64ecbf0935ea5.png)
[5-64ecbc38c4267.png](/user_uploads/57965/MBk-n_Ok52kDQ_vZzrKJm4oQ/5-64ecbc38c4267.png)
[6-64ecbf34264c2.png](/user_uploads/57965/PWZuqtSLFvyQDQkJBXEJmuDY/6-64ecbf34264c2.png)
[7-64ecb1c909b78.png](/user_uploads/57965/bLx4viPDcyoingbDSiNvg3V5/7-64ecb1c909b78.png)
Flutter Web

Multiple Clusters in same message

# Multiple image clusters in the same message

[4-64ecbf0935ea5.png](/user_uploads/57965/pHPfjDy_9FBEVSu5rnx4yBVJ/4-64ecbf0935ea5.png)
[5-64ecbc38c4267.png](/user_uploads/57965/B8i5AfRqwTJMx_y7Q4U4ejjz/5-64ecbc38c4267.png)

And some other things

[6-64ecbf34264c2.png](/user_uploads/57965/BFABVscknYHOZ8-Sos-vkIUH/6-64ecbf34264c2.png)
[7-64ecb1c909b78.png](/user_uploads/57965/ZE8KqaXEjMJWZu749CDE2gqG/7-64ecb1c909b78.png)
Flutter Web

Layout for profile image

All images so far show landscape images, so confirming here that the layout for a profile image is also what we expect:

[taylor-swift-self-titled-billboard-1240.webp](/user_uploads/57965/dqmQrKyxXK0UfQOUTS5I_y-0/taylor-swift-self-titled-billboard-1240.webp)
Flutter Web

Layout for tiny image

Images are laid out in a 150px by 100px space, and in general are scaled down to be contained inside the space. When the image is smaller than the area, it is centered and un-scaled.

Here's a tiny 50px image:
[Zulip-icon-50px.png](/user_uploads/57965/dfy-dlSZGwEckvtLHAtMtQPZ/Zulip-icon-50px.png)
Flutter Web

Images in a List

The layout works here but is problematic on web:

# Images in a list?
- [4-64ecbf0935ea5.png](/user_uploads/57965/m3W57BzdI0k_4CE0tqJ_B9ma/4-64ecbf0935ea5.png) [6-64ecbf34264c2.png](/user_uploads/57965/32I1XhzrXrM-IvHIGgvzT37N/6-64ecbf34264c2.png)
Flutter Web

Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for building this! Glad it turns out Flutter has that handy Wrap widget that basically takes care of the layout for us.

Generally this looks good; various small comments below. It's also clearly time for me to refactor the content tests so that they can share test data between the model and widgets sides, but that can happen after this change.

Comment on lines 262 to 265
child: ColoredBox(
color: const Color.fromRGBO(0, 0, 0, 0.03),
child: Padding(
padding: const EdgeInsets.all(1),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The transparent border
referenced in the comment was treated as additional
external padding to the image (space between images) but
it should be inner padding within the gray box.

Interesting, good catch, thanks.

Also
removed some alignment specifications as they were
unnecessary.

This is a different kind of change, though — it sounds like an NFC change. Is that right?

In that case this commit would be cleaner as two commits, one for the substantive change and one for the NFC change.

That would also make it easier to write a useful commit message for each of those commits — "clean up" isn't very informative, and in particular it doesn't at all describe the thing about the transparent border being part of the gray box. The summary lines could then be like:

content: Tweak MessageImage size to match web
content [nfc]: Cut an Align that has no effect

Comment on lines 87 to 94
} else if (node is ImageNode) {
return MessageImage(node: node);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This case shouldn't ever happen anymore, right? We should never have ImageNode as an element of a BlockContentList. (Because they should all be wrapped in ImageNodes nodes.)

Let's express that with an assertion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was still occurring, but only because I didn't handle multiple images in parseImplicitParagraphBlockContentList. Fixing that does indeed make this case impossible.

@@ -309,6 +309,17 @@ class MathBlockNode extends BlockContentNode {
}
}

class ImageNodes extends BlockContentNode {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
class ImageNodes extends BlockContentNode {
class ImageNodeList extends BlockContentNode {

Otherwise the name looks and sounds confusingly similar to ImageNode — which is especially a problem because the two classes are so closely related, so that the same code will often be referring to both.

@@ -219,6 +221,18 @@ class ListItemWidget extends StatelessWidget {
}
}

class MessageImages extends StatelessWidget {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similarly MessageImageList would be a good name

Comment on lines 259 to 261
// The corresponding element on web has a 5px two-sided margin…
// and then a 1px transparent border all around.
padding: const EdgeInsets.only(right: 5, bottom: 5),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// The corresponding element on web has a 5px two-sided margin…
// and then a 1px transparent border all around.
padding: const EdgeInsets.only(right: 5, bottom: 5),
padding: const EdgeInsets.only(right: 5, bottom: 5),

The comment was there to explain where the "1, 1, 6, 6" came from. Now that the code itself corresponds directly to what's in web, it speaks for itself and doesn't need a comment.

Comment on lines 1017 to 1036
// We get a bunch of newline Text nodes between paragraphs.
// A browser seems to ignore these; let's do the same.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment seems to have gotten lost.

@@ -1013,13 +1024,24 @@ class _ZulipContentParser {

List<BlockContentNode> parseBlockContentList(dom.NodeList nodes) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the other call site of parseBlockContent is above, in parseImplicitParagraphBlockContentList. What happens if images appear in a context where that gets called? I.e., at the top level of an element of a list.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The images ended up treated as BlockContentNodes in the list. I've added another commit to handle these cases, which improves how images render inside of lists.

Comment on lines 438 to 446
'<p>'
'<a href="https://chat.zulip.org/user_avatars/2/realm/icon.png?version=3">https://chat.zulip.org/user_avatars/2/realm/icon.png?version=3</a><br>\n'
'<a href="https://chat.zulip.org/user_avatars/2/realm/icon.png?version=4">https://chat.zulip.org/user_avatars/2/realm/icon.png?version=4</a></p>\n'
'<div class="message_inline_image">'
'<a href="https://chat.zulip.org/user_avatars/2/realm/icon.png?version=3">'
'<img src="https://uploads.zulipusercontent.net/f535ba07f95b99a83aa48e44fd62bbb6c6cf6615/68747470733a2f2f636861742e7a756c69702e6f72672f757365725f617661746172732f322f7265616c6d2f69636f6e2e706e673f76657273696f6e3d33"></a></div>'
'<div class="message_inline_image">'
'<a href="https://chat.zulip.org/user_avatars/2/realm/icon.png?version=4">'
'<img src="https://uploads.zulipusercontent.net/8f63bc2632a0e41be3f457d86c077e61b4a03e7e/68747470733a2f2f636861742e7a756c69702e6f72672f757365725f617661746172732f322f7265616c6d2f69636f6e2e706e673f76657273696f6e3d34"></a></div>',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'<p>'
'<a href="https://chat.zulip.org/user_avatars/2/realm/icon.png?version=3">https://chat.zulip.org/user_avatars/2/realm/icon.png?version=3</a><br>\n'
'<a href="https://chat.zulip.org/user_avatars/2/realm/icon.png?version=4">https://chat.zulip.org/user_avatars/2/realm/icon.png?version=4</a></p>\n'
'<div class="message_inline_image">'
'<a href="https://chat.zulip.org/user_avatars/2/realm/icon.png?version=3">'
'<img src="https://uploads.zulipusercontent.net/f535ba07f95b99a83aa48e44fd62bbb6c6cf6615/68747470733a2f2f636861742e7a756c69702e6f72672f757365725f617661746172732f322f7265616c6d2f69636f6e2e706e673f76657273696f6e3d33"></a></div>'
'<div class="message_inline_image">'
'<a href="https://chat.zulip.org/user_avatars/2/realm/icon.png?version=4">'
'<img src="https://uploads.zulipusercontent.net/8f63bc2632a0e41be3f457d86c077e61b4a03e7e/68747470733a2f2f636861742e7a756c69702e6f72672f757365725f617661746172732f322f7265616c6d2f69636f6e2e706e673f76657273696f6e3d34"></a></div>',
'<p>'
'<a href="https://chat.zulip.org/user_avatars/2/realm/icon.png?version=3">https://chat.zulip.org/user_avatars/2/realm/icon.png?version=3</a><br>\n'
'<a href="https://chat.zulip.org/user_avatars/2/realm/icon.png?version=4">https://chat.zulip.org/user_avatars/2/realm/icon.png?version=4</a></p>\n'
'<div class="message_inline_image">'
'<a href="https://chat.zulip.org/user_avatars/2/realm/icon.png?version=3">'
'<img src="https://uploads.zulipusercontent.net/f535ba07f95b99a83aa48e44fd62bbb6c6cf6615/68747470733a2f2f636861742e7a756c69702e6f72672f757365725f617661746172732f322f7265616c6d2f69636f6e2e706e673f76657273696f6e3d33"></a></div>'
'<div class="message_inline_image">'
'<a href="https://chat.zulip.org/user_avatars/2/realm/icon.png?version=4">'
'<img src="https://uploads.zulipusercontent.net/8f63bc2632a0e41be3f457d86c077e61b4a03e7e/68747470733a2f2f636861742e7a756c69702e6f72672f757365725f617661746172732f322f7265616c6d2f69636f6e2e706e673f76657273696f6e3d34"></a></div>',

The div elements aren't nested in the p, so the indentation should reflect that.

]),
]);

testParse('multiple cluster of images',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
testParse('multiple cluster of images',
testParse('multiple clusters of images',

@sirpengi
Copy link
Contributor Author

sirpengi commented Feb 5, 2024

@gnprice this is ready again

Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @sirpengi for the revision! Generally this looks good; one substantive comment below and one nit.

@@ -1003,22 +1015,45 @@ class _ZulipContentParser {
continue;
}
if (currentParagraph.isNotEmpty) consumeParagraph();
result.add(parseBlockContent(node));
final block = parseBlockContent(node);
if (block is ImageNode) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic should get tests too — i.e. something with images nested in a list.


return result;
}

List<BlockContentNode> parseBlockContentList(dom.NodeList nodes) {
assert(_debugParserContext == _ParserContext.block);
final acceptedNodes = nodes.where((node) {
final List<BlockContentNode> blocks = [];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
final List<BlockContentNode> blocks = [];
final List<BlockContentNode> result = [];

That matches the name of the corresponding local in parseImplicitParagraphBlockContentList. The two functions are very similar now (in fact I should probably follow up by refactoring them into one); so it's best to avoid introducing unnecessary differences, in order to help the real differences stand out when comparing them.

Comment on lines 328 to 312
testWidgets('parse multiple images', (tester) async {
// "https://chat.zulip.org/user_avatars/2/realm/icon.png?version=3\nhttps://chat.zulip.org/user_avatars/2/realm/icon.png?version=4"
await prepareContent(tester,
'<p>'
'<a href="https://chat.zulip.org/user_avatars/2/realm/icon.png?version=3">https://chat.zulip.org/user_avatars/2/realm/icon.png?version=3</a><br>\n'
'<a href="https://chat.zulip.org/user_avatars/2/realm/icon.png?version=4">https://chat.zulip.org/user_avatars/2/realm/icon.png?version=4</a></p>\n'
'<div class="message_inline_image">'
'<a href="https://chat.zulip.org/user_avatars/2/realm/icon.png?version=3">'
'<img src="https://uploads.zulipusercontent.net/f535ba07f95b99a83aa48e44fd62bbb6c6cf6615/68747470733a2f2f636861742e7a756c69702e6f72672f757365725f617661746172732f322f7265616c6d2f69636f6e2e706e673f76657273696f6e3d33"></a></div>'
'<div class="message_inline_image">'
'<a href="https://chat.zulip.org/user_avatars/2/realm/icon.png?version=4">'
'<img src="https://uploads.zulipusercontent.net/8f63bc2632a0e41be3f457d86c077e61b4a03e7e/68747470733a2f2f636861742e7a756c69702e6f72672f757365725f617661746172732f322f7265616c6d2f69636f6e2e706e673f76657273696f6e3d34"></a></div>');
final images = tester.widgetList<RealmContentNetworkImage>(find.byType(RealmContentNetworkImage));
check(images.map((i) => i.src.toString()).toList())
.deepEquals([
'https://uploads.zulipusercontent.net/f535ba07f95b99a83aa48e44fd62bbb6c6cf6615/68747470733a2f2f636861742e7a756c69702e6f72672f757365725f617661746172732f322f7265616c6d2f69636f6e2e706e673f76657273696f6e3d33',
'https://uploads.zulipusercontent.net/8f63bc2632a0e41be3f457d86c077e61b4a03e7e/68747470733a2f2f636861742e7a756c69702e6f72672f757365725f617661746172732f322f7265616c6d2f69636f6e2e706e673f76657273696f6e3d34',
]);
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just sent #511 which should simplify writing this sort of test. If you rebase atop that and then convert this PR's content-parsing tests into the new ContentExample form, then for example this test can become:

Suggested change
testWidgets('parse multiple images', (tester) async {
// "https://chat.zulip.org/user_avatars/2/realm/icon.png?version=3\nhttps://chat.zulip.org/user_avatars/2/realm/icon.png?version=4"
await prepareContent(tester,
'<p>'
'<a href="https://chat.zulip.org/user_avatars/2/realm/icon.png?version=3">https://chat.zulip.org/user_avatars/2/realm/icon.png?version=3</a><br>\n'
'<a href="https://chat.zulip.org/user_avatars/2/realm/icon.png?version=4">https://chat.zulip.org/user_avatars/2/realm/icon.png?version=4</a></p>\n'
'<div class="message_inline_image">'
'<a href="https://chat.zulip.org/user_avatars/2/realm/icon.png?version=3">'
'<img src="https://uploads.zulipusercontent.net/f535ba07f95b99a83aa48e44fd62bbb6c6cf6615/68747470733a2f2f636861742e7a756c69702e6f72672f757365725f617661746172732f322f7265616c6d2f69636f6e2e706e673f76657273696f6e3d33"></a></div>'
'<div class="message_inline_image">'
'<a href="https://chat.zulip.org/user_avatars/2/realm/icon.png?version=4">'
'<img src="https://uploads.zulipusercontent.net/8f63bc2632a0e41be3f457d86c077e61b4a03e7e/68747470733a2f2f636861742e7a756c69702e6f72672f757365725f617661746172732f322f7265616c6d2f69636f6e2e706e673f76657273696f6e3d34"></a></div>');
final images = tester.widgetList<RealmContentNetworkImage>(find.byType(RealmContentNetworkImage));
check(images.map((i) => i.src.toString()).toList())
.deepEquals([
'https://uploads.zulipusercontent.net/f535ba07f95b99a83aa48e44fd62bbb6c6cf6615/68747470733a2f2f636861742e7a756c69702e6f72672f757365725f617661746172732f322f7265616c6d2f69636f6e2e706e673f76657273696f6e3d33',
'https://uploads.zulipusercontent.net/8f63bc2632a0e41be3f457d86c077e61b4a03e7e/68747470733a2f2f636861742e7a756c69702e6f72672f757365725f617661746172732f322f7265616c6d2f69636f6e2e706e673f76657273696f6e3d34',
]);
});
testWidgets('multiple images', (tester) async {
const example = ContentExample.imageMultiple;
await prepareContent(tester, example.html);
final expectedImages = (example.expectedNodes[1] as ImageNodeList).images;
final images = tester.widgetList<RealmContentNetworkImage>(find.byType(RealmContentNetworkImage));
check(images.map((i) => i.src.toString()))
.deepEquals(expectedImages.map((n) => n.srcUrl));
});

which I think is a lot easier to digest. The difference, from not duplicating all the HTML of the examples, will be even more when adding those tests of the nested-in-a-list case.

In the code I pasted above, the way expectedImages is computed is slightly hacky, and will have to vary somewhat between different test cases depending on the structure of the example content. I'd be fine with a version that's hacky in that way. It'd be neat, but not necessary, to write a bit of logic for this group of tests that walks the tree of example.expectedNodes to find the ImageNodeList nodes in a generic way.

@sirpengi sirpengi force-pushed the pr-multiple-images branch 2 times, most recently from 5b4ad3b to b36fbac Compare February 9, 2024 15:40
@sirpengi
Copy link
Contributor Author

sirpengi commented Feb 9, 2024

@gnprice ready for review again, the updated content tests really helps to improve things!

Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Generally this all looks good — small comments below.

Comment on lines +317 to +320
final images = tester.widgetList<RealmContentNetworkImage>(
find.byType(RealmContentNetworkImage));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: commit that adds this has it as one line and next otherwise-unrelated commit adds newline; should squash that reformatting into original commit

Comment on lines 1033 to 1040
if (imageNodes.isNotEmpty) {
result.add(ImageNodeList(imageNodes));
imageNodes = [];
// In a context where paragraphs are implicit it
// should be impossible to have more paragraph
// content after image previews.
result.add(ParagraphNode(
wasImplicit: true,
links: null,
nodes: [UnimplementedInlineContentNode(htmlNode: node)]
));
continue;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I see the issue. If I comment this out, then with your handy corresponding test case the output I get is:

  Actual: <ZulipContent
   └─ListNode
     │ unordered
     │
     └─list item
       ├─ParagraphNode
       │ │ was implicit
       │ │
       │ ├─LinkNode#e84f6
       │ │ │ url: "https://chat.zulip.org/user_avatars/2/realm/icon.png"
       │ │ │
       │ │ └─TextNode
       │ │     "icon.png"
       │ │
       │ └─TextNode
       │     " "
       │
       ├─ParagraphNode
       │ │ was implicit
       │ │
       │ └─UnimplementedInlineContentNode
       │     html: "<span>Some content</span>"
       │
       └─ImageNodeList
         └─ImageNode
             srcUrl: "https://chat.zulip.org/user_avatars/2/realm/icon.png">

So the image would appear after the second implicit paragraph, which if this situation ever happened would probably not be what was expected and would look wrong. Good to make sure we have an UnimplementedNode in that case, to make sure we can discover if that does happen and do something about it.

Still, it's regrettable to have this many lines of code for a believed-impossible case — it gets in the way when trying to understand the main logic around it. (Which for this particular function is unfortunately fairly complicated in itself.)

Here's a tightened version — one line shorter from just reflowing the comment paragraph, and then simplifying the ParagraphNode with UnimplementedInlineContentNode to just an UnimplementedBlockContentNode:

Suggested change
if (imageNodes.isNotEmpty) {
result.add(ImageNodeList(imageNodes));
imageNodes = [];
// In a context where paragraphs are implicit it
// should be impossible to have more paragraph
// content after image previews.
result.add(ParagraphNode(
wasImplicit: true,
links: null,
nodes: [UnimplementedInlineContentNode(htmlNode: node)]
));
continue;
}
if (imageNodes.isNotEmpty) {
result.add(ImageNodeList(imageNodes));
imageNodes = [];
// In a context where paragraphs are implicit, it should be impossible
// to have more paragraph content after image previews.
result.add(UnimplementedBlockContentNode(htmlNode: node));
continue;
}

'<div class="message_inline_image">'
'<a href="https://chat.zulip.org/user_avatars/2/realm/icon.png" title="icon.png">'
'<img src="https://chat.zulip.org/user_avatars/2/realm/icon.png"></a></div>'
'<span>Some content</span></li>\n</ul>', [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that the code under test will put an UnimplementedNode here on account of the context in which this appears, the test loses some clarity by using an HTML fragment that would be unimplemented in any context and even if that bit of code were deleted. Instead we can use some HTML that's totally boring, just plain text:

Suggested change
'<span>Some content</span></li>\n</ul>', [
'more text</li>\n</ul>', [

'<br>\n</p>\n</blockquote>',
[QuotationNode([MathBlockNode(texSource: r'\lambda')])]);

static const singleImage = ContentExample(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's going to be a lot of statics on this class (there are already kind of a lot), so it'll need some care to help keep it possible to find the example one is looking for. One thing that I think will help is to make related examples all sort together alphabetically, by putting at the start of the name the word that describes the subject area of the example. So in particular I'd like to have all the examples in this PR take names starting with "image", even at the cost of making the names read less like natural English phrases.

For this one:

Suggested change
static const singleImage = ContentExample(
static const image = ContentExample(

or could also be imagePlain or imageSingle.

]),
]);

static const contentAfterImageCluster = ContentExample(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
static const contentAfterImageCluster = ContentExample(
static const imageClusterThenContent = ContentExample(

]);

static const contentAfterImageCluster = ContentExample(
'content after image cluster',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(But it's fine and good for these description strings to remain in more natural English syntax, like this is.)

@sirpengi sirpengi force-pushed the pr-multiple-images branch 2 times, most recently from e121da4 to 09475cf Compare February 12, 2024 22:49
@sirpengi
Copy link
Contributor Author

@gnprice ready to go again!

@gnprice
Copy link
Member

gnprice commented Feb 13, 2024

Thanks! All now looks good — merging.

@gnprice gnprice merged commit 61032ad into zulip:main Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show image previews side-by-side when consecutive
2 participants