GNOME Bugzilla – Bug 696802
Pickup additional directories configured in Tracker
Last modified: 2014-06-16 08:34:46 UTC
As a web developer, I keep a lot of sprites/website related (usually low-res) images, most of them in my Downloads folder. My Downloads folder is indexed by tracker for faster searching, so as a result when I open photos most of what I see is upscaled low-res icons from various web projects I'm working on, instead of my actual photos which are kept either in my Pictures directory and my Dropbox "Camera Sync" directory. I see two approaches for fixing this problem: 1) Prioritize photos in the Pictures directory so that they appear first in the list 2) Make it possible to configure which folders to display photos from.
Adding Jon to CC because we discussed this briefly at GUADEC. We might want to leave out ~/Downloads. Perhaps the same for gnome-documents too?
*** Bug 709366 has been marked as a duplicate of this bug. ***
(In reply to comment #0) > As a web developer, I keep a lot of sprites/website related (usually low-res) > images, most of them in my Downloads folder. My Downloads folder is indexed by > tracker for faster searching, so as a result when I open photos most of what I > see is upscaled low-res icons from various web projects I'm working on, instead > of my actual photos which are kept either in my Pictures directory and my > Dropbox "Camera Sync" directory. > > 2) Make it possible to configure which folders to display photos from. gnome-photos and gnome-documents use tracker for fetching metadata, you can choose which folders to be tracked using tracker-preferences. ie. add ~/Dropbox/Camera Sync, remove ~/Download for you. But gnome-photos only support three folders (Desktop, Download and Pictures), this is a real bug. (In reply to comment #1) > Adding Jon to CC because we discussed this briefly at GUADEC. We might want to > leave out ~/Downloads. Perhaps the same for gnome-documents too? gnome-documents works fine. If you want to leave out ~/Downloads, use tracker-perferences.
> > Adding Jon to CC because we discussed this briefly at GUADEC. We might want to > > leave out ~/Downloads. Perhaps the same for gnome-documents too? > > gnome-documents works fine. If you want to leave out ~/Downloads, use > tracker-perferences. The only less important problem is that Photos/Document will ignore pictures or documents in XDG directories except three special directories.
Created attachment 271139 [details] [review] Add all tracker directories, using the same way as gnome-documents please review, thanks!
Review of attachment 271139 [details] [review]: Thanks for the patch. While we do acknowledge the original problem in the bug report, we are still undecided on the solution. One option is to have "import to photos" action in, say Web, which will add the item to the Tracker DB in a way that it will show up in the application. Maybe by copying it over to ~/Pictures. At that point we can stop looking at ~/Downloads. The other alternative that has been thrown around is to have an option to monitor other locations.
I think the original problem is that user wants Photos to ignore ~/Downloads and index ~/Dropbox/Camera\ Sync/, but can't. In this case, I suggest using tracker-preferences to meet this need. Photos should works fine just like Documents, but I found it not to be so. Photos correctly leaves out ~/Download but still not index ~/Dropbox/Camera\ Sync/. This is a bug of Photos. It seems that it is a omission while porting from javascript to C. And my patch is only for this bug. I'm not sure whether it is necessary to add an action for download pictures to ~/Picture instead of ~/Download or an option to monitor other folders, since Documents and Music both don't have these.
I‘m sorry for these redundant comments caused by a strange network problem.
(In reply to comment #7) So reading the code again, Documents does indeed pickup locations from Tracker's settings these days. This was added somewhat recently. Sorry about that. > I think the original problem is that user wants Photos to ignore ~/Downloads > and index ~/Dropbox/Camera\ Sync/, but can't. > > In this case, I suggest using tracker-preferences to meet this need. Photos > should works fine just like Documents, but I found it not to be so. Photos > correctly leaves out ~/Download but still not index ~/Dropbox/Camera\ Sync/. But this part is confusing. I think Documents only picks up more locations from Tracker's settings. I can't see how it would leave out ~/Downloads. Looks pretty hard coded to me: https://git.gnome.org/browse/gnome-documents/tree/src/search.js#n345 Moreover your patch also would not omit ~/Downloads. It would only allow the user to add extra locations. Right? > I'm not sure whether it is necessary to add an action for download pictures to > ~/Picture instead of ~/Download or an option to monitor other folders, since > Documents and Music both don't have these. They will eventually have something. As I told you, we are undecided how to handle the ~/Downloads use case. Remember that the whole point of Documents, Photos and the other content applications is to let the user avoid dealing with files and directories. PS: the Search panel is another alternative to tracker-preferences.
Review of attachment 271139 [details] [review]: ::: src/photos-query-builder.c @@ +38,3 @@ +static const gchar *TRACKER_SCHEMA = "org.freedesktop.Tracker.Miner.Files"; +static const gchar *TRACKER_KEY_RECURSIVE_DIRECTORIES = "index-recursive-directories"; + We separate sections in a source file with 2 newlines. So add another one. @@ +344,3 @@ { + gchar *filter = NULL; + GSettings *settings = g_settings_new (TRACKER_SCHEMA); I think settings will get leaked because I don't see a 'g_object_unref (settings)'. Also, put the function call on a separate statement than the definition for consistency with the rest of the code. @@ +348,3 @@ + gchar *tracker_uri = NULL; + + for (; *tracker_dir != NULL; tracker_dir++) I would suggest using 'for (i = 0; tracker_dirs[i] != NULL; i++)', where i is a guint. It would be consistent with the pattern used elsewhere. See photos-search-provider.c and related files. @@ +358,3 @@ + { + tracker_uri = photos_query_builder_convert_path_to_uri (*tracker_dir); + filter = g_strconcat (g_strdup_printf (" || fn:contains (nie:url (?urn), \"%s\")", tracker_uri), It is better to use ' instead of " to quote strings inside SPARQL. First, you don't need to escape ', and using it consistently makes it easier to copy paste the queries from the DEBUG output into tracker-sparql(1). I have fixed the existing code to consistently use '. @@ +369,2 @@ const gchar *path; gchar *pictures_uri; These need to be moved to the top of the file. We do not do variable definitions in the middle of the block.
(In reply to comment #13) > But this part is confusing. I think Documents only picks up more locations from > Tracker's settings. I can't see how it would leave out ~/Downloads. Looks > pretty hard coded to me: > https://git.gnome.org/browse/gnome-documents/tree/src/search.js#n345 > > Moreover your patch also would not omit ~/Downloads. It would only allow the > user to add extra locations. Right? > Yes, the user still have to use Tracker‘s configuration tool to omit/add folders for Photos. > They will eventually have something. As I told you, we are undecided how to > handle the ~/Downloads use case. Remember that the whole point of Documents, > Photos and the other content applications is to let the user avoid dealing with > files and directories. > > PS: the Search panel is another alternative to tracker-preferences. Now, I prefer to add an option to customize which folders Photos will monitor.
Created attachment 271201 [details] [review] Fix patch according to the suggestions Thanks for the review. I've fixed the patch according to the suggestions.
Review of attachment 271201 [details] [review]: Thanks for the patch, Chunyang! This is better. ::: src/photos-query-builder.c @@ +376,2 @@ gchar *pictures_uri; + const gchar *path; This is a spurious change. 'const gchar *path;' got removed and added back. @@ +419,3 @@ g_free (download_uri); g_free (pictures_uri); + g_free (tracker_dir); We need g_strfreev instead of g_free because tracker_dir is a NULL terminated array of strings. @@ +420,3 @@ g_free (pictures_uri); + g_free (tracker_dir); + g_free (tracker_uri); tracker_uri should be freed inside the for loop.
Review of attachment 271201 [details] [review]: ::: src/photos-query-builder.c @@ +387,3 @@ + continue; + } + else We don't need the else branch. It is enough to have the continue above. @@ +392,3 @@ + filter = g_strconcat (g_strdup_printf (" || fn:contains (nie:url (?urn), '%s')", tracker_uri), + filter, + NULL); The previous buffer pointed to by filter and the buffer returned by g_strdup_printf are getting leaked. The easiest (and best) way out is to use GString. See photos-tracker-change-monitor.c for an example.
Created attachment 271917 [details] [review] query-builder: Add Tracker directories Fix patch according the suggestions.
If Photos will display photos from all tracker directories then the bug I described when I opened this one still applies. I want tracker to index my Downloads folder, for fast searching, but I do NOT want Photos displaying photos from there.
(In reply to comment #20) > If Photos will display photos from all tracker directories then the bug I > described when I opened this one still applies. I want tracker to index my > Downloads folder, for fast searching, but I do NOT want Photos displaying > photos from there. Yes, since this one got hijacked, lets use bug 709366 for that.
Changing the summary to reflect reality.
Created attachment 271922 [details] [review] Just update commit message
Review of attachment 271922 [details] [review]: This looks good, but I prefer the general structure of your previous patches. eg., we don't really need GString to create the whole string but only the parts of it with the Tracker locations.
Created attachment 272131 [details] [review] query-builder: Add additional directories configured in Tracker I tweaked your previous patch a bit and added the GString bits.
Thanks for your work on this Chunyang. Well done!
*** Bug 731652 has been marked as a duplicate of this bug. ***