settings [nfc]: Centralize logic for checking device animation settings #1957
Conversation
chrisbobbe
added
the
integration review
lib/model/settings.dart
Outdated
| /// Always show the animated version, ignoring device settings. | ||
| animateAlways, |
There was a problem hiding this comment.
Do we have a use case for this value? I'd think not, and I don't see it used in this version.
There was a problem hiding this comment.
I'm thinking of when the user asks to see the animation, which they haven't seen yet because "Auto-Play Animated Images" is off. Maybe by opening an image preview in the lightbox, or by doing that and then pressing a "play" button that we would add?
There was a problem hiding this comment.
I agree "ignoring device settings" doesn't sound right for that use case though :)
lib/model/settings.dart
Outdated
| /// Always show the still version, ignoring device settings. | ||
| animateNever, |
There was a problem hiding this comment.
And then I wouldn't think of this as "ignoring" settings — it's more like "even if allowed by device settings". (After all, there isn't really a setting that means "please use animations whenever at all possible, even where inappropriate".)
lib/model/settings.dart
Outdated
| @@ -1,3 +1,4 @@ | |||
| import 'package:flutter/widgets.dart'; | |||
There was a problem hiding this comment.
I don't love introducing a widgets import in model code. (Looks like we have two of those so far.)
I think widgets/emoji.dart would be a good home for this logic.
In fact: even apart from the need for resolve to import the widgets library, I think widgets/emoji.dart is a more natural home for the enum itself than model/settings.dart is. That's because the enum values don't represent possible values of a setting, but rather possible choices for how different parts of our UI might configure a shared widget. IOW it's more about the widgets than it is about a setting.
There was a problem hiding this comment.
Since this will also be useful for animated image content in Zulip messages, how about maybe lib/widgets/content.dart, or maybe a new file lib/widgets/image.dart? (And the latter might be a nicer home for RealmContentNetworkImage than the former, actually?)
There was a problem hiding this comment.
Sure, a widgets/image.dart file sounds good. RealmContentNetworkImage has long been a bit of an awkward fit in the file where it is.
lib/model/settings.dart
Outdated
| /// Callers should try to check whether an image is animated, | ||
| /// i.e. whether it has separate still and animated versions. | ||
| /// This can't be done perfectly (in 2025-10) | ||
| /// because of animated emoji that were uploaded before Zulip Server 5 | ||
| /// and don't have `still_url` filled in: | ||
| /// https://github.com/zulip/zulip/issues/36339 . |
There was a problem hiding this comment.
I'm not sure what this is saying a caller needs to do.
Is the "can't be done perfectly" just a reference to the fact that ImageEmojiDisplay.resolvedStillUrl is nullable? In that case, the second sentence seems like it belongs more on that field. Or perhaps better yet, on RealmEmojiItem.stillUrl which drives it.
This is only used with image emojis, for now, but it'll also be useful for zulip#1936, suppressing image animations in image previews. Related: zulip#1936
chrisbobbe
force-pushed
the
pr-centralize-should-animate-checks
branch
from
d3479af to
1699aa5
Compare
|
Thanks for the review! Revision pushed. |
|
Thanks! Looks good; merging. |
2 participants
Stacked atop #1928.