GNOME Bugzilla – Bug 768057
Support animated images
Last modified: 2018-01-21 17:48:04 UTC
Created attachment 330388 [details] [review] Patch to support animated images in Sushi Here is a patch that adds support for animated images to Sushi.
Review of attachment 330388 [details] [review]: Thanks, this looks mostly good. Just a few comments below. ::: src/js/viewers/image.js @@ +130,3 @@ + + destroy : function () { + this._ending = true; I don't think you need this private state, since you remove the mainloop source below -- so there should not be a case where the timeout callback is called after destroy() is called. @@ +138,3 @@ + + _onTimeoutCompleted : function() { + Mainloop.source_remove(this._timeoutId); Isn't this guaranteed to be non-zero at this point? @@ +143,3 @@ + this._texture.set_from_pixbuf(pix); + this._timeoutId = 0; + }, You can return true instead of manually restarting the timeout to re-schedule it again with the same parameters.
Created attachment 356665 [details] [review] Updated patch fixing GIF rendering. Okay. Here's an updated patch taking into account your comments.
Review of attachment 356665 [details] [review]: Thanks, some more minor comments... getting there. ::: src/js/viewers/image.js @@ +44,3 @@ this.moveOnClick = true; this.canFullScreen = true; + this._ending = false; This field is unused, so could be removed. @@ +137,3 @@ + + _advanceImage : function () { + this._timeoutId = 0; I don't think this will ever be called without this._timeoutId being set, so you could remove the check (unless I'm missing something).
Created attachment 356720 [details] [review] Updated patch fixing GIF rendering. Okay. I have removed all unnecessary checks and the use of the _ending flag.
Review of attachment 356720 [details] [review]: Thanks! Looks good to me now. Do you have a GNOME git account? If not, I will push the patch for you.
(In reply to Cosimo Cecchi from comment #5) > Review of attachment 356720 [details] [review] [review]: > > Thanks! Looks good to me now. Do you have a GNOME git account? If not, I > will push the patch for you. I don't have a GNOME git account.
Thanks again, I pushed this to master now.
I notice sushi didn't get a 3.26 release, will there be a sushi-3.28 release in March which includes this fix? Fedora, at least, is still on sushi-3.24.0 which does not animate GIF previews, so it'd be great to get it out there.
*** Bug 710842 has been marked as a duplicate of this bug. ***