content: Handle image emoji #2
Conversation
chrisbobbe
commented
Dec 29, 2022
chrisbobbe
commented
… way The implementation already treats them the same, but now the names reflect that. Perhaps one day we'll re-introduce a distinction when we need to.
There was a problem hiding this comment.
Thanks @chrisbobbe for taking care of this! This looks good; small comment below.
One commit-message nit:
$ git uvr --name-only
8089a9551 msglist [nfc]: Explicitly model realm- and zulip-extra emoji the same way
lib/model/content.dart
lib/widgets/content.dart
83005b5ef widgets [nfc]: Make kBaseFontSize a double, instead of inferred int
lib/widgets/content.dart
15d3ba952 content: Handle image emoji
lib/model/content.dart
lib/widgets/content.dart
I'd give all three of these the prefix content.
In particular I've been keeping message_list for the sorts of things that go in the message_list.dart files — basically, everything about the message lists except the contents of the messages. (The contents are a bigger area of work here than they are in the RN app, because we're not using a webview and handing the HTML and CSS to a browser to interpret.)
| top: -1.5, | ||
| child: Image.network( | ||
| adjustedSrc.toString(), | ||
| filterQuality: FilterQuality.low, |
There was a problem hiding this comment.
This should be FilterQuality.medium, same as for avatars and attached images.
At low, it… actually it looks pretty much fine in your screenshot from iOS, but here's what it is for me on Linux desktop:
Pretty pixelated and jagged.
At medium, it looks much better, and pretty similar to what web gets in Chrome on Linux:
(left is Flutter desktop, right is Chrome; the shadow is my window manager telling me the window on the right is focused, by having it cast a shadow over neighboring windows.)
… OK, and trying it on Android, on a physical Pixel 5, both low and medium look good; in fact I can't tell them apart, at least not on this emoji at this size. It seems possible that on that device they actually end up selecting the exact same algorithm.
In your testing, do you find low is actually better than medium? If so, perhaps we should choose different algorithms on different platforms.
In any case we almost surely want the same filterQuality value here as in sender avatars, and likely also in attached images. So let's keep it in sync with those.
There was a problem hiding this comment.
In your testing, do you find
lowis actually better thanmedium? If so, perhaps we should choose different algorithms on different platforms.
No, I just figured that emojis are probably close to the smallest images one would want to render, and so low would be appropriate. Seeing that it looks worse than medium on some platforms, and that we don't have anything close to a performance problem yet, I'm happy to use medium. 🙂
|
Thanks for the review! Revision pushed. |
|
Hmm, I did push my revision to my branch, Ah, I think probably this has to do with this repo's move from |
That page 404s for me, and I can't fetch from that repo over Git. I suspect the issue is that your fork is private (which would have been because this repo was still private when you made the fork), while this repo is now public. I guess I'll get to see the new revision when I'm back from vacation in January 🙂 |
|
My next revision is over at #4. |
