GNOME Bugzilla – Bug 684026
Improve gnome-desktop thumbnailing interface and code
Last modified: 2018-09-21 16:34:26 UTC
This (eventually) adds a super simple method to return a thumbnail given a URI and mtime, which is really what most people care about.
Created attachment 224321 [details] [review] thumbnail: Clean up path generation code Put the path calculation code in one spot.
Created attachment 224322 [details] [review] thumbnail: Move thumbnail generation/saving to a few helper functions Another cleanup in preparation for a new "simple" thumbnail API.
Created attachment 224323 [details] [review] thumbnail: Add gnome_desktop_factory_give_me_a_thumbnail This is a simple method on the factory that gives you a thumbnail, maybe from the cache, maybe not, you don't care. You just want a thumbnail.
Review of attachment 224321 [details] [review]: Looks like a great cleanup...I have a couple of minor comments below. Also, why is there a "libgsystem" it submodule change in this patch? ::: libgnome-desktop/gnome-desktop-thumbnail.c @@ +883,3 @@ + } + + Looks like you're leaking the GChecksum @@ -1388,3 @@ - guint8 digest[16]; - gsize digest_len = sizeof (digest); - GError *error; Don't we still need this GError later in the function?
(In reply to comment #4) > Review of attachment 224321 [details] [review]: > > Looks like a great cleanup...I have a couple of minor comments below. > Also, why is there a "libgsystem" it submodule change in this patch? I think I updated to the latest libgsystem by accident. > @@ -1388,3 @@ > - guint8 digest[16]; > - gsize digest_len = sizeof (digest); > - GError *error; > > Don't we still need this GError later in the function? Nope. It was unused before.
Created attachment 224368 [details] [review] thumbnail: Clean up path generation code Put the path calculation code in one spot.
Created attachment 224369 [details] [review] thumbnail: Move thumbnail generation/saving to a few helper functions Another cleanup in preparation for a new "simple" thumbnail API.
Created attachment 224370 [details] [review] thumbnail: Add gnome_desktop_factory_give_me_a_thumbnail This is a simple method on the factory that gives you a thumbnail, maybe from the cache, maybe not, you don't care. You just want a thumbnail.
Review of attachment 224368 [details] [review]: There's still libgsystem bits in this patch. ::: libgnome-desktop/gnome-desktop-thumbnail.c @@ +850,3 @@ +thumbnail_path (const char *uri, + GnomeDesktopThumbnailSize size, + gboolean is_fail) Don't like the "is_fail" argument, could do with being 2 separate functions (even if they share the checksum generation). @@ +865,3 @@ + file = g_strconcat (g_checksum_get_string (checksum), ".png", NULL); + + /* XXX: appname is only used for failed thumbnails. Is this a mistake? */ That obviously needs investigating and fixing.
Review of attachment 224369 [details] [review]: This patch would do with being split up. ::: libgnome-desktop/gnome-desktop-thumbnail.c @@ +1302,1 @@ + dirname = g_path_get_dirname (path); Leaking dirname.
Review of attachment 224370 [details] [review]: ::: libgnome-desktop/gnome-desktop-thumbnail.c @@ +1587,3 @@ + + /* FIXME: we cannot write anything to the thumbnal dir, apparently. + * What should we do here? */ Add error handling? ::: libgnome-desktop/gnome-desktop-thumbnail.h @@ +106,3 @@ int dest_height); +char * gnome_desktop_thumbnail_factory_give_me_a_thumbnail (GnomeDesktopThumbnailFactory *factory, Apart from being childish, that's not what the function does. It gives you a path to the thumbnail, not the thumbnail itself.
(In reply to comment #9) > Review of attachment 224368 [details] [review]: > > There's still libgsystem bits in this patch. I tried to fix this and apparently failed. Do you have any suggestions? > ::: libgnome-desktop/gnome-desktop-thumbnail.c > @@ +865,3 @@ > + file = g_strconcat (g_checksum_get_string (checksum), ".png", NULL); > + > + /* XXX: appname is only used for failed thumbnails. Is this a mistake? */ > > That obviously needs investigating and fixing. Sure, but that's orthagonal. We have to keep compatibility.
(In reply to comment #11) > Review of attachment 224370 [details] [review]: > > ::: libgnome-desktop/gnome-desktop-thumbnail.c > @@ +1587,3 @@ > + > + /* FIXME: we cannot write anything to the thumbnal dir, apparently. > + * What should we do here? */ > > Add error handling? No. If we get here, this is a broken system where we can't write to XDG_CONFIG_DIR. Do we want all applications to handle broken systems by returning a GError*? I guess I can g_warning and return "". > ::: libgnome-desktop/gnome-desktop-thumbnail.h > @@ +106,3 @@ > int dest_height); > > +char * gnome_desktop_thumbnail_factory_give_me_a_thumbnail > (GnomeDesktopThumbnailFactory *factory, > > Apart from being childish, that's not what the function does. It gives you a > path to the thumbnail, not the thumbnail itself. What would you like instead? The function name clearly isn't the best. Suggestions?
(In reply to comment #13) > (In reply to comment #11) > > Review of attachment 224370 [details] [review] [details]: > > > > ::: libgnome-desktop/gnome-desktop-thumbnail.c > > @@ +1587,3 @@ > > + > > + /* FIXME: we cannot write anything to the thumbnal dir, apparently. > > + * What should we do here? */ > > > > Add error handling? > > No. If we get here, this is a broken system where we can't write to > XDG_CONFIG_DIR. Do we want all applications to handle broken systems by > returning a GError*? I guess I can g_warning and return "". g_warning() and further error messages should be generated when the thumbnails fail to save. I doubt that an empty directory will have the desired effect. > > ::: libgnome-desktop/gnome-desktop-thumbnail.h > > @@ +106,3 @@ > > int dest_height); > > > > +char * gnome_desktop_thumbnail_factory_give_me_a_thumbnail > > (GnomeDesktopThumbnailFactory *factory, > > > > Apart from being childish, that's not what the function does. It gives you a > > path to the thumbnail, not the thumbnail itself. > > What would you like instead? The function name clearly isn't the best. > Suggestions? Don't have any, but feel free to ask around, pretty much anything would be better than that in a public API.
(In reply to comment #14) > g_warning() and further error messages should be generated when the thumbnails > fail to save. I doubt that an empty directory will have the desired effect. I don't understand. The only possible case we'll get here is if we can't write anything to the thumbnail directory, in which case we can't give applications a path to anything. g_warning and returning "" is our best option, as far as I'm concerned.
I think it would be pretty useful if gnome_desktop_thumbnail_factory_give_me_a_thumbnail() (or whatever it ends up being called) also took width and height parameters, and did the appropriate scaling internally, since most callers will probably want their thumbnails at a specific size. Might also want to add some convenience wrappers to, e.g., take in a GFile and return a GIcon or something. Depends on your use case, I guess. Other than that (and bearing in mind the comments from the other reviews) this looks like a nice simplification to me.
Created attachment 289025 [details] [review] thumbnail: Clean up path generation code Put the path calculation code in one spot.
Created attachment 289034 [details] [review] thumbnail: Move thumbnail generation/saving to a few helper functions Another cleanup in preparation for a new "simple" thumbnail API.
Looking at both nautilus and totem, heavy users of the thumbnailers, we usually have: - thumbnail being provided by GIO or grilo, if available - the usual _generate_thumbnail(), _save_thumbnail()/_create_failed_thumbnail() sequence The current problems with the API are that there's no cancellables (bug 572961), and no builtin thread pool to queue up thumbnailing operations. Jasper, what was the use case for gnome_desktop_thumbnail_factory_give_me_a_thumbnail()? Was it for something not coming from GIO?
For the gnome-documents & gnome-photos use case: Often there is no GVfs backend for the remote content that we handle. In those cases we plugin our own thumbnailer that calls out to a library (eg., grilo, libgdata, etc.) to get the thumbnail URI, that we fetch and save. > The current problems with the API are that there's no cancellables (bug > 572961), and no builtin thread pool to queue up thumbnailing operations. If we are going to have an inbuilt thread pool, then I would like to have the ability to set my own sort function so that I can have local items thumbnailed before remote ones because they are likely to be faster. I can also think of fancier criteria like thumbnailing items which are currently in the view, etc..
(In reply to comment #20) > For the gnome-documents & gnome-photos use case: > > Often there is no GVfs backend for the remote content that we handle. In those > cases we plugin our own thumbnailer that calls out to a library (eg., grilo, > libgdata, etc.) to get the thumbnail URI, that we fetch and save. What do you do here? Do you use the thumbnailer factory to save it, or do you save it locally for your app? > > The current problems with the API are that there's no cancellables (bug > > 572961), and no builtin thread pool to queue up thumbnailing operations. > > If we are going to have an inbuilt thread pool, then I would like to have the > ability to set my own sort function so that I can have local items thumbnailed > before remote ones because they are likely to be faster. I can also think of > fancier criteria like thumbnailing items which are currently in the view, etc.. Or you could create 2 thread pools, one for local, one for remote.
(In reply to comment #21) > (In reply to comment #20) > > For the gnome-documents & gnome-photos use case: > > > > Often there is no GVfs backend for the remote content that we handle. In those > > cases we plugin our own thumbnailer that calls out to a library (eg., grilo, > > libgdata, etc.) to get the thumbnail URI, that we fetch and save. > > What do you do here? Do you use the thumbnailer factory to save it, or do you > save it locally for your app? We "save" it to gnome_desktop_thumbnail_path_for_uri. Where is saving is done either by g_file_copy or g_output_stream_splice depending on whether there is a GVfs backend to grab the URI or the library offers an input stream for grabbing stuff (like libgdata does). In case we failed to get a thumbnail (either the service doesn't offer them or we simply failed to get one), we can attempt to create it from the actual file when the user opens it for viewing. However, this is not implemented at the moment. > > If we are going to have an inbuilt thread pool, then I would like to have the > > ability to set my own sort function so that I can have local items thumbnailed > > before remote ones because they are likely to be faster. I can also think of > > fancier criteria like thumbnailing items which are currently in the view, etc.. > > Or you could create 2 thread pools, one for local, one for remote. True.
Review of attachment 289025 [details] [review]: ::: libgnome-desktop/gnome-desktop-thumbnail.c @@ +1057,3 @@ +static char * +thumbnail_failed_path (const char *uri, + return file; This parameter seems to be unused
Review of attachment 289034 [details] [review]: ::: libgnome-desktop/gnome-desktop-thumbnail.c @@ +1520,3 @@ + error = NULL; + if (width != NULL && height != NULL) + saved_ok = gdk_pixbuf_save (pixbuf, Any reason not to use the same "ret" variable instead of a separate "saved_ok"?
(In reply to comment #19) > Jasper, what was the use case for > gnome_desktop_thumbnail_factory_give_me_a_thumbnail()? > Was it for something not coming from GIO? I honestly can't remember. It was something Cosimo and I worked on together. It might have been part of the old files search in gnome-shell.
Created attachment 290030 [details] [review] thumbnail: Clean up path generation code Put the path calculation code in one spot.
Created attachment 290031 [details] [review] thumbnail: Move thumbnail generation/saving to a few helper functions Another cleanup in preparation for a new "simple" thumbnail API.
Attachment 290030 [details] pushed as 57c18b8 - thumbnail: Clean up path generation code Attachment 290031 [details] pushed as 97f6f77 - thumbnail: Move thumbnail generation/saving to a few helper functions
Created attachment 291762 [details] [review] thumbnail: Fix thumbnail generation With these patches in 3.15.2 thumbnail generation is broken and it causes thumbnailers to be spawned constantly. I've added a patch that gets it working again here.
Review of attachment 291762 [details] [review]: As mentioned on IRC, could you please split the patch in two fixes, and explain them in the commit message.
Created attachment 292157 [details] [review] thumbnail: Don't fail after successfully creating the cache directory
Created attachment 292158 [details] [review] thumbnail: Include the filename in the thumbnail path
Review of attachment 292157 [details] [review]: Yes!
Review of attachment 292158 [details] [review]: Looks good, but can you add a link to the pre-cleanup code in the commit log? https://git.gnome.org/browse/gnome-desktop/tree/libgnome-desktop/gnome-desktop-thumbnail.c?id=f2d25fa7d6cdaba03679f0fc0921fc5f81bd3944#n1060
Attachment 292158 [details] pushed as ef0f02e - thumbnail: Include the filename in the thumbnail path
I've posted about a replacement API to be included lower down the stack on gtk-devel: https://mail.gnome.org/archives/gtk-devel-list/2018-January/msg00018.html Does this look sufficient for your uses? It's pretty much gnome_desktop_factory_give_me_a_thumbnail() but hopefully extensible. If it's good enough, I'll likely start writing the code, and see what applications I can port.
-- 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/gnome-desktop/issues/43.