Support Animated Gifs Inline #1477
Conversation
|
Ok, don't merge this yet. Doing further testing I appear to be leaking the paintables. I need to go figure out how GLib / Vala / GTK handles deconstruction, I guess 😛 |
psycotica0
force-pushed
the
cv_animated_gif
branch
from
6cd5982
to
3796d02
Compare
|
Ok, I've used a weakref for the delay now. I use a static to avoid capturing In my testing now (with logging not included in this PR) it stops ticking when I leave the conversation with gifs, and starts again when I go back. |
psycotica0
force-pushed
the
cv_animated_gif
branch
from
3796d02
to
63d23c0
Compare
|
Ok, I found more sources of leaks that are unrelated to my changes. These were just already leaking widgets sometimes, but that's more obvious with the animated gifs because the timer keeps ticking even though you can't see it. It makes it a CPU leak and not just a memory leak. So I fixed those in #1480, and then rebased this change on that so I can use the pattern I introduced there, here. |
|
Thanks for your work! For accessibility reasons we should make a setting for this feature. In case the setting is disabled, it would be nice to still have an option to selectively start the animation by clicking on it. |
|
Right, that's fair... but it's definitely more involved. As soon as we have UI that controls the animation, now the image view needs to handle click, and then needs to feed those clicks down to the animatable which it knows things about. I don't disagree with you in the fullness of time and theory, but in practice what's been built is "animated images work just the same as still images and the app doesn't know or care which it's displaying" and everything else involves building "but actually what if it did care and the UI knew which things were animated and which weren't and did different things for each but only in some cases" Maybe someday someone will build inline video support, and then maybe the animated gifs can be represented as looping inline videos with controls and stuff and some kind of auto-play feature? But I don't think I'm ready to build that today. |
In case of click wouldn't the system just use the "viewer" app and that would animate it? |
Oh true! Ok, if what we want is a setting that's either always show animated as animated, or always show animated as static, I think I could make that work without changing too much. Dino's existing settings seem a bit sparse, though. Is this a big enough change that we want to add a setting for it? |
|
Imagine a channel with 10 animating GIF memes... continuously... on going... on and on... :) |
|
Right, yeah. That's why the PR I'm working on right now (but got sidetracked by the memory leak hunt) allows collapsing of images, both animated and otherwise. Just cleaning up my testing on that before publishing... It currently looks like: |
While doing testing I noticed that skeletons were being leaked, and eventually tracked it down to the timer that updates the time label closing over "this" and then keeping the reference alive, potentially for 24 hours. I noticed a few other places in the code doing some version of this, and one of them had the "static and weak pointer" approach, which I pulled out into a util class. Now, we still have to make sure we're passing it a static method instead of a lambda, as that would also close over "this" and render the whole thing useless, but at least most of the annoying parts live in the util class now. Also the call_widget version was doing a weird thing where it was removing itself, but then returning "true"? I'm not sure what that accomplishes, because returning "false" means to not run this again. So I think my new version is the same in practice, but simpler... There are other timeouts in the code that I briefly looked over, but all of them seemed to be relatively short hard-coded durations, so I left them alone. But if any of them are long-lived, it's possible they could also benefit from this class in the future.
This implements a new Paintable I borrowed from a forum and converted into Vala. The PixbufAnimation, if given a non-animated image simply returns it as a one-frame, non-looping, animation and so it shouldn't be a lot more overhead than the original method. It just draws once and then never updates again. So it appears to work with existing images, but also handles animation! Closes dino#386
psycotica0
force-pushed
the
cv_animated_gif
branch
from
63d23c0
to
c572330
Compare
|
I've added some changes to the upstream weak timeout branch, and have updated this to include those changes. |
|
The image collapsing PR is #1483 |
Playing animations seems to be an issue for people with epilepsy (link). Hence, for accessibility reasons, I think that if we add playing animations, we have to make it optional.
Yes, that would be fine. In the final stage, I'd like have an inline click-to-play and click-to-stop feature within Dino, but as an initial step into that direction we can merge it like that. |
This implements a new Paintable I borrowed from a forum and converted into Vala. The PixbufAnimation, if given a non-animated image simply returns it as a one-frame, non-looping, animation and so it shouldn't be a lot more overhead than the original method. It just draws once and then never updates again.
So it appears to work with existing images, but also handles animation!
It's slightly sketchy to still have the
filegetter here. I'm actually not sure if it works, but it doesn't look like anything is calling it. I tried to leave the calling interface the same, but if people would rather I be a bit more invasive, I can change the interface to removefileas a property altogether.What this doesn't do right now is have any way to pause / stop the animation. It's something that's doable (it would just have to not make the timer if it's paused) but that also involves more UI and linkage, etc. And honestly I don't think I want that anyway. What I'd rather have, for all images, is the ability to collapse / hide them, which would solve this problem for both animated and static images that I'm tired of seeing. So I'm probably going to work in that direction instead.
Closes #386