GNOME Bugzilla – Bug 686895
file-info: catch thumbnail files in large directory as well
Last modified: 2018-05-24 14:46:51 UTC
See attached patch
Created attachment 227309 [details] [review] file-info: catch thumbnail files in large directory as well When building the file attribute table info for local files, use thumbnail paths in $XDG_CACHE_DIR/thumbnails/large in addition to $XDG_CACHE_DIR/thumbnails/normal. Failing to do this would cause an application that creates large thumbnails by default to never find any value for G_FILE_ATTRIBUTE_THUMBNAIL_PATH, with no G_FILE_ATTRIBUTE_THUMBNAILING_FAILED set, which might cause the application to either think thumbnailing is still in progress, or blindly requeue thumbnail operations in a loop. Large thumbnails are generally preferred, so we now default to the path of a large thumbnail (in case both are present).
Review of attachment 227309 [details] [review]: Excellent commit message, patch looks correct.
Attachment 227309 [details] pushed as d681b58 - file-info: catch thumbnail files in large directory as well Thanks, pushed to master.
Created attachment 229417 [details] [review] file-info: Don't leak the filename if thumbnailing failed The previous commit introduced a tiny memory leak. This one fixes it.
Review of attachment 229417 [details] [review]: Nice catch, looks good to me.
Thanks for the quick review. Pushed to master.
Thinking about this a bit more, we should probably use a separate attribute for this because otherwise there is no way for an application to force bigger thumbnails to be generated. eg., Photos wants to show bigger thumbnails, but most people have the smaller ones created because of Nautilus or something, so there is no way to explicitly specify larger thumbnails. Reopening.
Created attachment 291184 [details] [review] file-info: Allow specifically asking for large thumbnails
See bug 690255 for the gnome-photos bug about bad quality thumbnails.
Review of attachment 291184 [details] [review]: ::: gio/gfileinfo.h @@ +698,3 @@ + * + * A key in the "thumbnail" namespace for getting the path to the + * large thumbnail image. Corresponding #GFileAttributeType is what is "large"? ::: gio/glocalfileinfo.c @@ +1316,3 @@ } + + if (!found && !large) I don't like that we only fall back to "normal" if "large" is found, even in the case that we are requesting the normal sized thumbnail. It seems like we should return the normal thumbnail if it is requested and the large if it is requested. @@ +1967,3 @@ + + if (_g_file_attribute_matcher_matches_id (attribute_matcher, + G_FILE_ATTRIBUTE_ID_THUMBNAIL_PATH_LARGE)) We are also setting the "is valid" and possibly "failed" attributes here, so we should probably check for those. We're also going to end up with "spooky action at a distance" with respect to the 'failed' attribute. It could end up getting set to one thing or another depending on what other attributes were requested, which is just weird. Maybe we want a second copy of that as well? I also generally dislike the fact that we hardcode "gnome-thumbnail-factory" here as a basis for reporting validity/failure, but nowhere do we document this. What is the user supposed to do here in case of invalid thumbnails? The answer is clearly "call gnome-thumbnail-factory" but we don't document that.
The situation with the fallback between 'large' and 'normal' is particularly awful because, presently, we will actually take an invalid large thumbnail in preference to a valid normal one.
After some thought, here's what I think needs to happen: 1) handle the two types of thumbnails completely independently with a separate set of all three attributes (path, valid, failed) for each 2) don't fallback between different thumbnails at a given size 3) check for all three attributes on the matcher 4) document that this is bound to the thumbnailer in gnome-desktop optionally we may want to consider changing our current behaviour of returning a path (or setting 'failed') even when the is-valid flag is set to FALSE. if the is-valid flag is FALSE then it's because the current thumbnail (or failure) is no longer valid for the current file at that location.
(In reply to comment #12) > After some thought, here's what I think needs to happen: > > 1) handle the two types of thumbnails completely independently with a separate > set of all three attributes (path, valid, failed) for each > > 2) don't fallback between different thumbnails at a given size > > 3) check for all three attributes on the matcher > > 4) document that this is bound to the thumbnailer in gnome-desktop I agree pretty much with your entire analysis here, except that I would document the last point only in a way that allows us to change the implementation later. I think longer term we really want thumbnails to be a more integral part of the platform instead of having gnome-desktop take that role.
Created attachment 292955 [details] [review] file-info: Don't ignore thumbnail::failed and thumbnail::is-valid
Created attachment 292956 [details] [review] Revert "file-info: catch thumbnail files in large directory as well"
Created attachment 292957 [details] [review] file-info: Add a set of attributes for large thumbnails
Debarshi, your patches look good to me. Here's another thought though: if all an application cares is whether a thumbnail of any sort exists, after this change the only way to know is to query both the regular and large attributes. I think this is a bit overkill, since not all applications will care about the size of the thumbnail. An alternative design that makes that operation easier would be to have explicit NORMAL and LARGE set of attributes, and keep the current unspecified one as a catch-all that looks in both places. At the end of the day though, I don't see why we would ever want to generate non-large thumbnails these days... That would probably require a change in the gnome-desktop API too though.
(In reply to comment #17) Thanks for taking a look, Cosimo. > An alternative design that makes that operation easier would be to have > explicit NORMAL and LARGE set of attributes, and keep the current unspecified > one as a catch-all that looks in both places. If we end up retaining the catch-all, then it will still need some fixing. See comment 11 > At the end of the day though, I don't see why we would ever want to generate > non-large thumbnails these days... That would probably require a change in the > gnome-desktop API too though. Some releases ago when I spoke to Alex about the GVfs side of things (bug 688685), he was initially concerned about the extra IO in loading larger images when the application only needs smaller ones.
(In reply to comment #17) > Debarshi, your patches look good to me. > Here's another thought though: if all an application cares is whether a > thumbnail of any sort exists The only case that I could imagine this is true is if the application (through lack of a dependency on gnome-desktop) is incapable of calling for creation of the thumbnail if it does not exist. Any app that would show a thumbnail if it existed but not create one for itself in order to show sounds a bit broken (ie: changing behaviour based on the content of ~/.cache). So I don't agree too much with this argument... I think the reason we keep going around in circles here is that we don't have a firm set of instructions for exactly which protocol you should follow for how to use this API...
To take my comments a step further: having half of the required API for dealing with thumbnails in GIO is a mistake. We need either to rip it out of finish it up. I don't think we want to rip it out. That means that we need to add an API to GIO to request generation of thumbnails. I don't want the code for this to live in GLib, so that means that we need either to use some sort of giomodule or D-Bus call to a session service in order to get this done. Perhaps both. It might make sense to do this from gvfs. One of the daemons (or a new one) could act as a thumbnailing service. When the user calls a "I need a thumbnail for this file" function in GIO it uses a new method on the the default gvfs to implement that. For the gvfs client library, that would make a D-Bus call to the thumbnailing service.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/621.