GNOME Bugzilla – Bug 523159
add thumbnail purge plug-in
Last modified: 2008-09-21 00:27:15 UTC
It would be nice to have a new function like: void gnome_thumbnail_purge_cache (); which would delete files in ~/.thumbnails based on two gconf parameters, something like: /desktop/gnome/thumbnail_cache/max_size (default = 256 MB?) /desktop/gnome/thumbnail_cache/max_age (default = 90 days?) Then, gThumb could call this function occasionally, or better yet, gnome-session or gdm could call it on logout. There is currently no cache cleaning in gnome, and it is well known that large caches slow down login noticeably (bug 430123). Does this sound like a good idea? I might be able to supply a patch, eventually, if it sounds OK. - Mike
Created attachment 107610 [details] [review] rough thumbnail cache purge patch Here is an initial patch to show what I had in mind. It is not complete - the age and size limits are hard-coded; they need to be replaced by gconf keys. - Mike
Created attachment 107622 [details] [review] v2 of thumbnail cache purge patch Here is an updated patch that uses gconf keys to set the age/size limits. The keys would have to be added to the libgnome schemas, in a separate patch. - Mike
Alexander Larsson suggests this would be better as a part of gnome-settings-daemon... - Mike
Created attachment 107867 [details] [review] Patch to add thumbnail cache cleaning plugin This is my first attempt to add a plugin to gsd. I need some help however. I don't understand what the correct method of debugging plugins under jhbuild is. Could a gsd guru give me some clues here? How can I log easily messages to the console (or a file) from gsd? Is it supposed to work properly with jhbuild and dbus and all that? The plugin doesn't seem to work, currently. I'm not a great programmer, so all pointers are welcome! - Mike
Created attachment 107868 [details] [review] Patch with corrected copyright info
(In reply to comment #4) > I need some help however. I don't understand what the correct method of > debugging plugins under jhbuild is. Could a gsd guru give me some clues here? > How can I log easily messages to the console (or a file) from gsd? Is it > supposed to work properly with jhbuild and dbus and all that? The easiest way to do that is probably 1) make sure gnome-session cannot restart g-s-d automatically (ie. remove it from where dbus expects it to be) 2) kill g-s-d 3) manually run g-s-d with the --debug switch. It will dump output from g_debug and friends to the console > The plugin doesn't seem to work, currently. From a quick once-over I've got a few comments on the patch, but I'll hold back until it actually works. ;-)
Created attachment 107883 [details] [review] working purge-cache patch Thanks for the debugging pointers! That really helped. This version of the patch appears to work, subject to my limited testing... - Mike
Ok, so there. Looks pretty good overall. I can't actually try it right now, so comments from code inspection only. --- /dev/null 2008-03-23 14:36:07.997130374 -0400 +++ plugins/clean-thumbs/Makefile.am 2008-03-23 14:11:21.000000000 -0400 @@ -0,0 +1,49 @@ +NULL = Please drop useless cruft like this. --- /dev/null 2008-03-23 14:36:07.997130374 -0400 +++ plugins/clean-thumbs/gsd-clean-thumbs-manager.h 2008-03-23 06:32:46.000000000 -0400 @@ -0,0 +1,57 @@ +/* -*- Mode: C; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 8 -*- + * + * Copyright (C) 2007 William Jon McCann <mccann@jhu.edu> That looks... not quite right. --- /dev/null 2008-03-23 14:36:07.997130374 -0400 +++ plugins/clean-thumbs/gsd-clean-thumbs-manager.c 2008-03-23 15:26:10.000000000 -0400 +#include <...> The list of includes looks a bit excessive. Is all of that really needed? +#define GCONF_BINDING_DIR "/desktop/gnome/thumbnail_cache" Convention is to use hyphens instead of underscores for gconf dirs. (I'm aware of g-s-d not following that. Talk about errors of the past...) +enum { + PROP_0, +}; Please remove. +static GList * +read_dir_for_purge (const char *path, GList *files) +{ + DIR *dir; + struct dirent *dirent; Please use glib/gio for traversing directories/stat'ing etc. +static void +purge_old_thumbnails (ThumbData *info, PurgeData *purge_data) +{ + if ((purge_data->now - info->mtime) > purge_data->max_age) { + info->size = 0; + g_unlink (info->path); + g_debug ("clean-thumbs: removing %s, older than %ld days", + info->path, + purge_data->max_age); Indentation seems to be off with all the debug statements. There also seem to be a few more than we would want in a shipped binary (especially in hot code paths). +static int +sort_file_mtime (ThumbData *file1, ThumbData *file2) +{ + if (file1->mtime < file2->mtime) return -1; + if (file1->mtime > file2->mtime) return 1; + return 0; return file1->mtime - file2->mtime ? +static int +get_gconf_int_with_nonzero_default (char *key, int default_value) +{ ... + value = gconf_client_get (client, key, NULL); + + if (value == NULL) { + res = default_value; + gconf_client_set_int (client, key, default_value, NULL); Don't modify GConf database. +static gboolean +purge_cache (gpointer data) { + + char *path; + GList *files = NULL; No need to initialize files. + + purge_data = g_new0 (PurgeData, 1); No need to allocate this on the heap. Use the stack. + if (purge_data->max_age >= 0) + g_list_foreach (files, (GFunc) purge_old_thumbnails, purge_data); + + if ((purge_data->total_size > purge_data->max_size) && (purge_data->max_size >= 0)) { I feel like in both of these cases it should be > 0 instead of >= 0. + GList *scan = NULL; No need to initialize scan here. +gboolean +gsd_clean_thumbs_manager_start (GsdCleanThumbsManager *manager, + GError **error) +{ + g_debug ("Starting clean_thumbs manager"); + + register_config_callback (manager, + GCONF_BINDING_DIR, + (GConfClientNotifyFunc) bindings_callback); Running a full purge_cache each time someone modifies some settings seems a bit over the top. Store the last time we ran and recalculate from there. You also need to reconfigure the plugin and the timeout callback in this case. + manager->priv->timeout_cb = 0; + + /* Clean once on start-up */ + purge_cache (manager); That's a particularly bad idea. Session startup is a very congested period, and we certainly don't want to run anything there if we can delay it just a little bit more. Combined with the changes required for the comment above, you could simply add the timeout with a smaller duration (say 5 minutes) for the first run, and automatically reconfigure after that. + /* Clean once a day */ + manager->priv->timeout_cb = g_timeout_add (ONCE_A_DAY, g_timeout_add_seconds + (GtkFunction) purge_cache, GtkFunction? Should be GSourceFunc. +static void +gsd_clean_thumbs_manager_class_init (GsdCleanThumbsManagerClass *klass) +{ + GObjectClass *object_class = G_OBJECT_CLASS (klass); + + object_class->get_property = gsd_clean_thumbs_manager_get_property; + object_class->set_property = gsd_clean_thumbs_manager_set_property; + object_class->constructor = gsd_clean_thumbs_manager_constructor; + object_class->dispose = gsd_clean_thumbs_manager_dispose; + object_class->finalize = gsd_clean_thumbs_manager_finalize; Don't set these if you don't use them anyway. --- /dev/null 2008-03-23 14:36:07.997130374 -0400 +++ plugins/clean-thumbs/clean-thumbs.gnome-settings-plugin.in 2008-03-23 06:27:52.000000000 -0400 @@ -0,0 +1,8 @@ +[GNOME Settings Plugin] +Module=clean-thumbs +IAge=0 +_Name=Clean Thumbs +_Description=A tool to keep the thumbnail cache small and fresh. It's not really a tool.
(In reply to comment #8) > Ok, so there. Looks pretty good overall. I can't actually try it right now, so > comments from code inspection only. Thanks for the fast and thorough review! I have some work to do... > bit more. Combined with the changes required for the comment above, you could > simply add the timeout with a smaller duration (say 5 minutes) for the first > run, and automatically reconfigure after that. Can you point me to a place where this approach is used (automatic reconfig of timeouts)? There are several ways to do this; I'd like to use the most gnomish way... - Mike
(In reply to comment #9) > Can you point me to a place where this approach is used (automatic reconfig of > timeouts)? There are several ways to do this; I'd like to use the most gnomish > way... Actually, some of that was a bit misleading since for some reason I assumed the timeout was configurable. Not sure what would qualify as gnomish here, but the way I'd tackle this is to have two timeouts. A once-only short-term timeout (say 2 minutes) that's started at module startup and whenever the configuration is modified (but don't add another short-term if one's already running), and one continuous long-term timeout (the once-per-day run). Based on recent posts on p.g.o (http://www.gnome.org/~csaavedra/news-2008-03.html#D22, http://log.emmanuelebassi.net/archives/2008/03/time-to-build/) I'm also starting to wonder whether this wouldn't be better turned into a general "housekeeping" module instead of just "clean-thumbs". For now that'd just mean a different name, I guess. I'm not convinced the recently-used case is actually something g-s-d should care about, but the fact remains that there's more stuff than just the thumbnails cache that needs purging every once in a while. This also sort of implies moving the settings from /desktop/gnome/... to /apps/gnome_settings_daemon/...
Jens, OK, I'll use two timeouts and rename the module to "housekeeping". I have a few questions/comments about your comments, if you don't mind: (In reply to comment #8) > +++ plugins/clean-thumbs/Makefile.am 2008-03-23 14:11:21.000000000 -0400 > @@ -0,0 +1,49 @@ > +NULL = > > Please drop useless cruft like this. This was copied directly from the "dummy" module - you might want to strip it out from there. > + GList *files = NULL; > > No need to initialize files. Why - are GLists auto-initialized to NULL? > + purge_data = g_new0 (PurgeData, 1); > > No need to allocate this on the heap. Use the stack. I don't know what this means. (Sorry, my programming knowledge is lumpy.) > + if ((purge_data->total_size > purge_data->max_size) && > (purge_data->max_size >= 0)) { > > I feel like in both of these cases it should be > 0 instead of >= 0. The intent here was that a gconf setting of "0" would correspond to maximum purging (delete everything, for the paranoid). I can modify things so that age/size settings of "0" correspond to "do nothing", but I'm not sure that is better. Is that what you want? (Right now, "-1" would be "do nothing".) - Mike
(In reply to comment #11) > > +++ plugins/clean-thumbs/Makefile.am 2008-03-23 14:11:21.000000000 -0400 > > @@ -0,0 +1,49 @@ > > +NULL = > > > > Please drop useless cruft like this. > > This was copied directly from the "dummy" module - you might want to strip it > out from there. Yeah, I'll do that. Thanks for the hint. > > + GList *files = NULL; > > > > No need to initialize files. > > Why - are GLists auto-initialized to NULL? That's just personal preference, I guess. Instead of + files = read_dir_for_purge (path, files); I'd simply do files = read_dir_for_purge (path, NULL); Doesn't make much of a difference either way. > > + purge_data = g_new0 (PurgeData, 1); > > > > No need to allocate this on the heap. Use the stack. > > I don't know what this means. (Sorry, my programming knowledge is lumpy.) Instead of allocating with g_new and later having to free it, you can simply do PurgeData purge_data; here since the data never leaves the scope of the function and there's no need to do any fancy to release the resources later on. > > + if ((purge_data->total_size > purge_data->max_size) && > > (purge_data->max_size >= 0)) { > > > > I feel like in both of these cases it should be > 0 instead of >= 0. > > The intent here was that a gconf setting of "0" would correspond to maximum > purging (delete everything, for the paranoid). I can modify things so that > age/size settings of "0" correspond to "do nothing", but I'm not sure that is > better. Is that what you want? (Right now, "-1" would be "do nothing".) Hm, I was thinking that a setting of 0 (purge everything) didn't make much sense but if we do have paranoid people there, maybe it does.
Created attachment 107965 [details] [review] updated housekeeping plugin patch Jens, Here is an updated patch. I think it addresses most of your concerns. I kept the zero = paranoid mode. If a delete-everything mode isn't there, people will ask for it. Also, I didn't delete all of the gobject/class stuff which I copied from the dummy module, because I don't understand what exactly is harmless and what is mandatory yet. Also, someone might want to extend the module... - Mike
Almost there. --- plugins/Makefile.am (revision 232) +++ plugins/Makefile.am (working copy) @@ -3,6 +3,7 @@ SUBDIRS = \ a11y-keyboard \ background \ + housekeeping \ clipboard \ dummy \ font \ Index: configure.ac =================================================================== --- configure.ac (revision 232) +++ configure.ac (working copy) @@ -406,6 +406,7 @@ plugins/a11y-keyboard/Makefile plugins/background/Makefile plugins/background/libbackground/Makefile +plugins/housekeeping/Makefile plugins/clipboard/Makefile plugins/dummy/Makefile plugins/font/Makefile Please keep those lists in alphabetical order. --- /dev/null 2008-03-24 16:25:17.996149703 -0400 +++ plugins/housekeeping/Makefile.am 2008-03-24 15:58:42.000000000 -0400 @@ -0,0 +1,47 @@ +libhousekeeping_la_SOURCES = \ + gsd-housekeeping-plugin.h \ + gsd-housekeeping-plugin.c \ + gsd-housekeeping-manager.h \ + gsd-housekeeping-manager.c \ + $(NULL) Please remove all uses of the NULL variable as well and keep source files in alphabetical order. +static GList * +read_dir_for_purge (const char *path, GList *files) +{ ... + if (enum_dir != NULL) { + GFileInfo *info; + while ((info = g_file_enumerator_next_file (enum_dir, NULL, NULL)) != NULL) { Indentation is messed up in lots of places. Use either spaces or tabs, not both. Also, you're leaking info here. +static void +purge_old_thumbnails (ThumbData *info, PurgeData *purge_data) +{ + if (((*purge_data).now - info->mtime) > (*purge_data).max_age) { Why so complicated? purge_data->now is so much easier to read. + GFile *file; + file = g_file_new_for_uri (info->uri); + g_file_delete (file, NULL, NULL); + g_object_unref (file); This seems rather heavy-handed. Why not just save the path instead of the URI and g_unlink like you did before? +static int +get_gconf_int_with_nonzero_default (char *key, int default_value) +{ + /* If the key is unset, we use a non-zero default value. + A zero value corresponds to an extra-paranoid level + of cleaning - it deletes all files. We don't what that Typo. +static void +purge_thumbnail_cache (void) { ... + time (&purge_data.now); g_get_current_time () +gboolean +gsd_housekeeping_manager_start (GsdHousekeepingManager *manager, + GError **error) +{ ... + manager->priv->short_term_cb = 0; Not necessary as long as stop is always called before calling start again (which it is). +static void +gsd_housekeeping_manager_class_init (GsdHousekeepingManagerClass *klass) +{ + GObjectClass *object_class = G_OBJECT_CLASS (klass); + + object_class->constructor = gsd_housekeeping_manager_constructor; + object_class->dispose = gsd_housekeeping_manager_dispose; + object_class->finalize = gsd_housekeeping_manager_finalize; Dispose and finalize can be disposed of. Constructor, too, I believe; easy to test. If somebody wanted to extend the plugin and needed those functions, they could easily be added back.
Created attachment 108013 [details] [review] updated housekeeping patch New patch attached. I can commit once it is ready... - Mike
Created attachment 108015 [details] [review] housekeeping patch, v6 Oops, fixed tab/space issues in one *.h file. - Mike
After a bit of discussion, I'll have to revise my previous statement. The two new GConf keys should really go to /desktop/gnome/thumbnail_cache/. As such, the respective schema definition should be added to libgnome, not g-s-d. Please also remove the TODO from the code. Patch looks fine to me, apart from that. I've asked the release team for their opinion on whether we can add this feature in the next stable release or need to punt to 2.23.
(In reply to comment #14) > + manager->priv->short_term_cb = 0; > > Not necessary as long as stop is always called before calling start again > (which it is). I'm not sure this has been done correctly... where is short_term_cb initialized to zero on the first run? Is _stop called before _start on the first run? If so, aren't we killing off a random timeout before initializing short_term_cb? - Mike
(In reply to comment #17) > The two new GConf keys should really go to /desktop/gnome/thumbnail_cache/. As > such, the respective schema definition should be added to libgnome, not g-s-d. I find it a bit unsettling to have the age/size parameters in one hierarchy and the plugin enable parameter in another - it looks like a potential source of confusion... but I'll put them wherever you want them :-) Do I need to file a new bug for the libgnomeui patch, or can you approve that here? Did you mean "thumbnail-cache", rather than "thumbnail_cache", or is the underscore intentional? > Please also remove the TODO from the code. Patch looks fine to me, apart from > that. I've asked the release team for their opinion on whether we can add this > feature in the next stable release or need to punt to 2.23. If it goes in 2.23, does that mean you need to create a 2.22 branch first, and then the patch is committed to trunk? I'll tweak the patch after I hear back from you about this comment and the previous one. - Mike
> I'm not sure this has been done correctly... where is short_term_cb initialized > to zero on the first run? Is _stop called before _start on the first run? If > so, aren't we killing off a random timeout before initializing short_term_cb? No. GObject takes care to zero out the private part on object initialization. > I find it a bit unsettling to have the age/size parameters in one hierarchy and > the plugin enable parameter in another - it looks like a potential source of > confusion... but I'll put them wherever you want them :-) It makes sense, though. The two keys are defining "desktop behaviour", irrespective of who actually implements it. The functionality could easily move to another component, the g-s-d plugin might be renamed, etc. etc. The plugin keys, however, really only affect this specific implementation in g-s-d. > Do I need to file a new bug for the libgnomeui patch, or can you approve that > here? Works either way. File a separate bug or attach it here as a separate patch and we'll toss it over when the g-s-d part is done. It's libgnome, though, not libgnomeui. > Did you mean "thumbnail-cache", rather than "thumbnail_cache", or is the > underscore intentional? That was intentional. Looking at the GConf keys we already have, there is no standard. Some use "-", some "_", but in the /desktop/gnome directory, most use underscores, so we'll try to blend in. > If it goes in 2.23, does that mean you need to create a 2.22 branch first, and > then the patch is committed to trunk? Yes.
Created attachment 108052 [details] [review] schema patch for libgnome, v1 Here is the patch for libgnome to add the /desktop/gnome/thumbnail_cache parameters.
Created attachment 108053 [details] [review] housekeeping patch against gsd, v8 and here is the gsd patch.
Release team says punt, but g-s-d has branched now, so...
Committed! Thanks for your reviews; I learned a lot... - Mike
Re-assigning as a libgnome bug, to get a review of the pending gconf schema patch. - Mike
The schema patch has been committed. Closing bug as fixed. - Mike
michael
michael, do you think could this be applied to nautilus too? I mean, not everyone has/uses gthumb, but pretty much everyone has nautilus?
Nautilus, gThumb, eog, and other gnome/kde apps all share the same thumbnail cache. The thumbnail cache cleaner thus applies to all of them. - Mike
yes, but it depends on the user running gthumb right? If I understood correctly this is a gthumb-specific plugin, so the (theoretical) user who only uses nautilus and not gthum would not benefit from this?
No, this evolved into a plug-in for gnome-settings-daemon. It is run shortly after you log on. You don't have to run gthumb or nautilus or anything else. - Mike
great, thanks for the good work!
Conflict with F-spot -------------------- F-spot uses these thumbnails, but this new setting causes F-spot to regenerate the thumbnail list over and over again. Is is possible to set this function standard to non-active? See: http://mail.gnome.org/archives/f-spot-list/2008-September/msg00037.html Does anybody sees other possiblities to solve this issue?
To specify this even more: What's the Use Case where this functionality can be useful? If a user browses trough his files in nautilus, they got a thumbnail attached. If you reboot your pc (or logout login Gnome), those thumbnails will be gone? This makes totally no sense. If it slows down your computer, there must be a choice to clean this up manually. Use case 1: John is having a lot of thumbnails in his .Thumbnail directory which slows down the login time of his computer. He wants to make this faster. He installs the new version of Gnome and everything runs ok. Use Case 2: Stijn is an f-spot user. He wants to see his photos right away after F-spot has launched. After installing the new version of Gnome, this is not possible anymore. The solution could be a sort of "Clean up Thumbnails" button in Nautilus or ... that invokes this method manually, in stead of every login or logout. This wouldn't break the current program functionality like F-spot.
Stijn, I don't quite follow what you are saying. By default, the thumbnail cleaner purges the oldest thumbnails, so that the cache is smaller than 64 MB and no thumbnail is older than 60 days. So, by default, the cleaner should NOT be deleting thumbnails that were just created. This would happen if /desktop/gnome/thumbnail_cache/maximum_age or /desktop/gnome/thumbnail_cache/maximum_size were set to zero, however. What are /desktop/gnome/thumbnail_cache/maximum_age and /desktop/gnome/thumbnail_cache/maximum_size set to in your system? Maybe the distro screwed up the schemas? Here is the default schema: http://svn.gnome.org/viewvc/libgnome/trunk/schemas/desktop_gnome_thumbnail_cache.schemas.in?view=markup&pathrev=3711 - Mike
(In reply to comment #35) > I don't quite follow what you are saying. By default, the thumbnail cleaner > purges the oldest thumbnails, so that the cache is smaller than 64 MB and no > thumbnail is older than 60 days. > > So, by default, the cleaner should NOT be deleting thumbnails that were just > created. Not exactly. If more than 64MB of thumbnails where just created, next time the plugin runs it deletes the oldest (which are actually brand new) so that the 64MB limit is satisfied. > > This would happen if /desktop/gnome/thumbnail_cache/maximum_age or > /desktop/gnome/thumbnail_cache/maximum_size were set to zero, however. > > What are /desktop/gnome/thumbnail_cache/maximum_age and > /desktop/gnome/thumbnail_cache/maximum_size set to in your system? Maybe the > distro screwed up the schemas? no. checked it. If you ask me, 64MB is too small of a limit. Also, if a user deals with tens of GB of media (very common case) they should have at least 1GB to spare for thumbnails. f-spot developers actually worked around this by dynamically adjusting the maximum_size limit so that it is always more than what's needed for f-spot thumbnails. Yiannis.
> Also, if a user deals with tens of GB of media (very common case) they should have at least 1GB to spare for thumbnails. I disagree. 64 MB are enough for 99% of the directories. Typically we have up to 25 kB - i.e. roughly a maximum of 2500 files is stored at once. However, for not creating endless creation->deletion->creation->... cycles, we should definitly exclude currently displayed directories from thumbnail cleanup Maybe there should be a gnome_thumbnail_inhibit_cleanup (const char *base_directory); gnome_thumbnail_permit_cleanup (const char *base_directory); API talking to the gnome-settings-daemon?