GNOME Bugzilla – Bug 638172
Get rid of gconf in GnomeDesktopThumbnailFactory
Last modified: 2011-01-08 09:59:58 UTC
I'm already working on this, I was talking to Vincent and the idea is to use GKey files, similar to normal desktop files, instead of using gsettings. Thumbnailers will install a key file with information about what mime-types it supports and the command to run the thumbnailer.
Created attachment 177251 [details] [review] Don't use GConf to handle external thumbnailers With this patch libgnome-desktop doesn't depend on GConf anymore. Note that this patch depends on bug #638331, since it uses the new gsettings schema for thumbnailers.
Review of attachment 177251 [details] [review]: Coding style problems. Other than that looks alright to me.
(In reply to comment #2) > Review of attachment 177251 [details] [review]: > > Coding style problems. where? I tried to follow the coding style of the file, please tell me where the problems are to fix them. > Other than that looks alright to me. Thanks for the review.
I think Bastien must have meant that you're placing braces on the next line instead of the same line as the statement/conditional.
It seems there are several coding styles in that file already, see: http://git.gnome.org/browse/gnome-desktop/tree/libgnome-desktop/gnome-desktop-thumbnail.c#n100 http://git.gnome.org/browse/gnome-desktop/tree/libgnome-desktop/gnome-desktop-thumbnail.c#n265 .... I haven't noticed actually, I saw the gtk+ coding style somewhere in the file and I thought it was the only one and followed it.
Review of attachment 177251 [details] [review]: I had a very quick look, and it looks okay to me. I'll let Bastien clarify the coding style problems, but after that, it's probably okay to push it. Thanks! ::: libgnome-desktop/gnome-desktop-thumbnail.c @@ +240,3 @@ + thumbs_dirs[length] = NULL; + + return thumbs_dirs; We should also support the user data dir here, I guess.
(In reply to comment #6) > Review of attachment 177251 [details] [review]: > > I had a very quick look, and it looks okay to me. I'll let Bastien clarify the > coding style problems, but after that, it's probably okay to push it. Thanks! > > ::: libgnome-desktop/gnome-desktop-thumbnail.c > @@ +240,3 @@ > + thumbs_dirs[length] = NULL; > + > + return thumbs_dirs; > > We should also support the user data dir here, I guess. Do we really want users to install .thumbnailer files in their home dir?
Yes, I think it makes sense. It can be used to override something, to quickly test something, or whatever. We do this for apps too. And for nearly everything using the XDG directories.
Ok, I'll update the patch then.
Created attachment 177504 [details] [review] Updated patch Updated patch: - It bumps gsettings schemas requirement to 0.1.4 since it uses new thumbnailer schemas - It adds the user data directory to the thumbnailer dir list
I've fixed a memleak (I was leaking try_exec in thumbnailer_reload) and pushed to git master. I'll fix any coding style issues if needed.