After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 686895 - file-info: catch thumbnail files in large directory as well
file-info: catch thumbnail files in large directory as well
Status: RESOLVED OBSOLETE
Product: glib
Classification: Platform
Component: gio
unspecified
Other All
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks: 686851
 
 
Reported: 2012-10-25 21:41 UTC by Cosimo Cecchi
Modified: 2018-05-24 14:46 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
file-info: catch thumbnail files in large directory as well (2.73 KB, patch)
2012-10-25 21:41 UTC, Cosimo Cecchi
committed Details | Review
file-info: Don't leak the filename if thumbnailing failed (905 bytes, patch)
2012-11-19 23:15 UTC, Debarshi Ray
committed Details | Review
file-info: Allow specifically asking for large thumbnails (8.86 KB, patch)
2014-11-21 15:13 UTC, Debarshi Ray
needs-work Details | Review
file-info: Don't ignore thumbnail::failed and thumbnail::is-valid (1.41 KB, patch)
2014-12-18 07:41 UTC, Debarshi Ray
none Details | Review
Revert "file-info: catch thumbnail files in large directory as well" (3.12 KB, patch)
2014-12-18 07:41 UTC, Debarshi Ray
none Details | Review
file-info: Add a set of attributes for large thumbnails (10.05 KB, patch)
2014-12-18 07:42 UTC, Debarshi Ray
none Details | Review

Description Cosimo Cecchi 2012-10-25 21:41:07 UTC
See attached patch
Comment 1 Cosimo Cecchi 2012-10-25 21:41:09 UTC
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).
Comment 2 Colin Walters 2012-10-26 13:34:31 UTC
Review of attachment 227309 [details] [review]:

Excellent commit message, patch looks correct.
Comment 3 Cosimo Cecchi 2012-10-26 14:48:58 UTC
Attachment 227309 [details] pushed as d681b58 - file-info: catch thumbnail files in large directory as well

Thanks, pushed to master.
Comment 4 Debarshi Ray 2012-11-19 23:15:13 UTC
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.
Comment 5 Cosimo Cecchi 2012-11-19 23:17:30 UTC
Review of attachment 229417 [details] [review]:

Nice catch, looks good to me.
Comment 6 Debarshi Ray 2012-11-19 23:24:33 UTC
Thanks for the quick review. Pushed to master.
Comment 7 Debarshi Ray 2013-02-05 14:21:23 UTC
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.
Comment 8 Debarshi Ray 2014-11-21 15:13:06 UTC
Created attachment 291184 [details] [review]
file-info: Allow specifically asking for large thumbnails
Comment 9 Debarshi Ray 2014-11-21 15:15:08 UTC
See bug 690255 for the gnome-photos bug about bad quality thumbnails.
Comment 10 Allison Karlitskaya (desrt) 2014-11-21 16:18:37 UTC
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.
Comment 11 Allison Karlitskaya (desrt) 2014-11-21 16:47:46 UTC
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.
Comment 12 Allison Karlitskaya (desrt) 2014-11-21 19:09:28 UTC
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.
Comment 13 Cosimo Cecchi 2014-11-25 17:25:22 UTC
(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.
Comment 14 Debarshi Ray 2014-12-18 07:41:09 UTC
Created attachment 292955 [details] [review]
file-info: Don't ignore thumbnail::failed and thumbnail::is-valid
Comment 15 Debarshi Ray 2014-12-18 07:41:38 UTC
Created attachment 292956 [details] [review]
Revert "file-info: catch thumbnail files in large directory as well"
Comment 16 Debarshi Ray 2014-12-18 07:42:11 UTC
Created attachment 292957 [details] [review]
file-info: Add a set of attributes for large thumbnails
Comment 17 Cosimo Cecchi 2015-01-15 05:20:12 UTC
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.
Comment 18 Debarshi Ray 2015-01-15 10:34:20 UTC
(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.
Comment 19 Allison Karlitskaya (desrt) 2015-01-20 18:37:05 UTC
(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...
Comment 20 Allison Karlitskaya (desrt) 2015-01-20 18:43:32 UTC
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.
Comment 21 GNOME Infrastructure Team 2018-05-24 14:46:51 UTC
-- 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.