GNOME Bugzilla – Bug 770267
Link the original and shared copies in Tracker
Last modified: 2016-09-09 12:50:10 UTC
With the patches from bug 751181, once an image has been uploaded to an online account, it is picked up by our online miners and duplicated in the UI. Instead, we should somehow merge the local and shared copy so that they are shown as a single entry. We can do this by adding a relation (say, nie:relatedTo) between the two items in Tracker's database. It will be even nicer if this metadata is also embedded in the original local file. It will let us reconstruct the relation if we loose the database, or copy the file to another computer.
Created attachment 334068 [details] [review] query-builder: Add insert and relate remote objects queries
Created attachment 334069 [details] [review] share-point-online: Add remote object in tracker
Created attachment 334071 [details] [review] share-point-google: Add remote object after uploading image
Review of attachment 334071 [details] [review]: ::: src/photos-share-point-google.c @@ +28,2 @@ #include "photos-base-item.h" +#include "photos-filterable.h" This header is unused. @@ +109,3 @@ + + g_task_return_boolean (task, TRUE); + g_print ("Everything was done cleanly!\n"); This debug message should be removed. @@ +129,3 @@ + + id = gdata_entry_get_id (GDATA_ENTRY (file_entry)); + title = gdata_entry_get_title (GDATA_ENTRY (file_entry)); We don't need the title for de-dupication, right? If I understand correctly, you are inserting the title to have something meaningful to show in the UI if the remote item is accessed. Sadly, the title is not enough for that. We need everything from account_miner_job_process_photo (in gnome-online-miners/src/gom-gdata-miner.c). @@ +172,3 @@ } + /* Map remote to local object and vice-versa */ This comment should be removed. @@ +320,3 @@ +photos_share_point_google_finalize (GObject *object) +{ + G_OBJECT_CLASS (photos_share_point_google_parent_class)->dispose (object); Eek! Chaining up to dispose in finalize. Should be removed if it remains unused.
Review of attachment 334069 [details] [review]: ::: src/photos-share-point-online.c @@ +26,2 @@ #include "config.h" +#include <tracker-sparql.h> Missing newline. @@ +39,3 @@ { GIcon *icon; + PhotosTrackerQueue *queue; Nitpick: please retain the ordering. @@ +321,3 @@ + { + if (gexiv2_metadata_has_tag (meta, "Xmp.xmp.gnome-photos.google.title")) + gexiv2_metadata_clear_tag (meta, "Xmp.xmp.gnome-photos.google.title"); We can't have Google-specific code in these generic base classes. @@ +328,3 @@ + gexiv2_metadata_set_tag_string (meta, "Xmp.xmp.gnome-photos.google.title", data->remote_title); + + identifier = g_strconcat ("google:picasaweb:", data->remote_id, NULL); Ditto. Also, as we discussed on IRC, we should not do this for RAW images. Since RAW files can have so many MIME types, I think the safer option is to go for a whitelist consisting of image/jpeg, image/png and image/jp2. @@ +389,3 @@ + photos_share_point_online_relate_objects (self, cancellable, remote_urn, item_urn); + + /* Metadata Embed logic flows from here */ These comments are not necessary. @@ +413,3 @@ + priv = photos_share_point_online_get_instance_private (self); + query = photos_query_builder_insert_remote_object (state, data->remote_title, data->remote_id); + photos_tracker_queue_update_blank (priv->queue, First of all, PhotosTrackerQueue is not thread-safe. So we can't call it from separate threads. Secondly, photos_tracker_queue_update_blank is an async call, not sync. So it doesn't make sense to call from a thread. @@ +454,3 @@ + g_task_set_task_data (task, data, (GDestroyNotify) photos_share_point_online_data_free); + + g_task_run_in_thread (task, photos_share_point_online_create_entry_in_thread_func); We can't use a thread here. See above.
Review of attachment 334068 [details] [review]: ::: src/photos-query-builder.c @@ +351,2 @@ +PhotosQuery * +photos_query_builder_relate_objects (PhotosSearchContextState *state, const gchar *obj1, const gchar *obj2) domain_urn and range_urn would be better names than obj1 and obj2. @@ +366,3 @@ + gchar *sparql; + + identifier = g_strconcat ("google:picasaweb:", id, NULL); Cannot have Google-specific code here.
(In reply to Debarshi Ray from comment #0) > It will be even nicer if this metadata is also embedded in the original > local file. It will let us reconstruct the relation if we loose the > database, or copy the file to another computer. Thank you for your review, Debarshi. At this point, when I am about to change the code according to the issues pointed out, I want to ask that: Shouldn't share-point-* do only the embedding metadata stuff? Rest of the things (i.e. inserting remote object in tracker's database and relating them) should be present somewhere else (maybe item-manager ?) The reason is to support of what you said above. If I see correctly, "reconstruct the relation" will be same as what we do after the upload. So, question is about redundant code present in gnome-photos. I may be getting too much ahead of myself here.
(In reply to Umang Jain from comment #7) > (In reply to Debarshi Ray from comment #0) > > > It will be even nicer if this metadata is also embedded in the original > > local file. It will let us reconstruct the relation if we loose the > > database, or copy the file to another computer. > > Thank you for your review, Debarshi. > > At this point, when I am about to change the code according to the issues > pointed out, I want to ask that: > > Shouldn't share-point-* do only the embedding metadata stuff? Rest of the > things (i.e. inserting remote object in tracker's database and relating > them) should be present somewhere else (maybe item-manager ?) > > The reason is to support of what you said above. If I see correctly, > "reconstruct the relation" will be same as what we do after the upload. So, > question is about redundant code present in gnome-photos. We don't have time to implement reconstructing the Tracker database from the embedded information. Right now, let's concentrate only on inserting the relations into the database, embedding the information in the original file once the upload is done, and using the relations in Tracker to do the merging. Thinking more about it, I think the code to embed the metadata should be a BaseItem method. eg., photos_base_item_embed_metadata_async (...). This method should then be called by the SharePoint.
Review of attachment 334069 [details] [review]: ::: src/photos-share-point-online.c @@ +326,3 @@ + gexiv2_metadata_clear_tag (meta, "Xmp.xmp.gnome-photos.google.id"); + + gexiv2_metadata_set_tag_string (meta, "Xmp.xmp.gnome-photos.google.title", data->remote_title); Do we really need to embed the title of the remote item? The ID should be enough, isn't it?
Created attachment 334192 [details] [review] query-builder: Add remote object queries
Created attachment 334193 [details] [review] share-point-google: Add remote object and relate with local resource
Created attachment 334194 [details] [review] base-item: Add metadata to local resource
Created attachment 334251 [details] [review] query-builder: Add remote object equipment related queries
Created attachment 334252 [details] [review] share-point-google: Add and relate remote object manually
Created attachment 334253 [details] [review] base-item: Add remote id as metadata to local resource
Review of attachment 334252 [details] [review]: ::: src/photos-share-point-google.c @@ +215,3 @@ + gint64 timestamp; + gboolean flash; + Nitpick: the newline isn't needed. @@ +227,3 @@ + const gchar *flash_on; + const gchar *alternate_uri; + Ditto. @@ +237,3 @@ + + flash_off = "http://www.tracker-project.org/temp/nmm#flash-off"; + flash_on = "http://www.tracker-project.org/temp/nmm#flash-on"; It would be better to use: g_quark_to_string (FLASH_OFF) ,and g_quark_to_string (FLASH_ON)
Review of attachment 334253 [details] [review]: ::: src/photos-base-item.c @@ +2167,3 @@ + if ((g_strcmp0 (mime_type, "image/png") == 0) || + (g_strcmp0 (mime_type, "image/jpeg") == 0)|| + (g_strcmp0 (mime_type, "image/jpg") == 0)) Is there a reason why you are checking for both image/jpeg and image/jpg? I think image/jpeg should enough, but maybe you have a reason? We should also check for image/jp2 for JPEG-2000s.
(In reply to Debarshi Ray from comment #17) > Review of attachment 334253 [details] [review] [review]: > > ::: src/photos-base-item.c > @@ +2167,3 @@ > + if ((g_strcmp0 (mime_type, "image/png") == 0) || > + (g_strcmp0 (mime_type, "image/jpeg") == 0)|| > + (g_strcmp0 (mime_type, "image/jpg") == 0)) > > Is there a reason why you are checking for both image/jpeg and image/jpg? I > think image/jpeg should enough, but maybe you have a reason? We should also > check for image/jp2 for JPEG-2000s. Ah, yes! I failed to notice that this is mime type, not the extension of the image. I wonder what was I thinking while writing this piece :/ Apologies.
Review of attachment 334252 [details] [review]: + if (model != NULL || make != NULL) + { + query = photos_query_builder_get_equipment_query (state, item_urn); + photos_tracker_queue_select (self->queue, + query->sparql, + cancellable, + photos_google_share_point_get_equipment, + g_object_ref (task), + g_object_unref); + } The idea here is that, if we get make or model in uploaded file entry, there is surely a nfo:equipment set for the local object. We then query the Equipment of local object and set the equipment as nfo:equipment of the remote object. Also, include g_return_if_fail and g_warning if we get make/model but could not get equipment of local image (Ideally, this should not happen).
Review of attachment 334252 [details] [review]: As mentioned earlier on IRC, I am thinking that instead of duplicating all the SPARQL inserts and queries in gnome-photos, we can re-use the code that is already in gnome-online-miners. We can add a new D-Bus method and just use that. I have filed bug 770899 for that.
Created attachment 334871 [details] [review] application: Add photos_application_get_miner
Created attachment 334872 [details] [review] gom-miner: Add InsertSharedContent
Created attachment 334873 [details] [review] share-point-google: Call InsertSharedContent after the item is uploaded
Review of attachment 334253 [details] [review]: ::: src/photos-base-item.c @@ +2169,3 @@ + (g_strcmp0 (mime_type, "image/jpg") == 0)) + { + meta = gexiv2_metadata_new (); Nitpick: s/metadata/meta/ for consistency. @@ +2173,3 @@ + uri = photos_base_item_get_uri (item); + file = g_file_new_for_uri (uri); + path = g_file_get_path (file); We can't take the URI and convert it to a path in BaseItem because it won't work for some of the remote BaseItem sub-classes. eg., GoogleItem. @@ +2186,3 @@ + gexiv2_metadata_clear_tag (meta, tag); + + gexiv2_metadata_set_tag_string (meta, tag, id); I think we need a multi-valued tag (ie. gexiv2_metadata_set_tag_multiple) because the same source can be uploaded multiple times. I also wonder if we should also save the GoaAccount:identity. ::: src/photos-base-item.h @@ +102,3 @@ +void photos_base_item_add_metadata_async (PhotosBaseItem *self, + gchar *id, It should be 'const gchar *', not 'gchar *'.
Created attachment 334958 [details] [review] application: Register XMP namespace
Created attachment 334959 [details] [review] base-item: Add a metadata_add_shared virtual method
Created attachment 334960 [details] [review] base-item: Add photos_base_item_metadata_add_shared_async
Created attachment 334961 [details] [review] local-item: Embed XMP metadata identifying shared copies
Review of attachment 334961 [details] [review]: ::: src/photos-local-item.c @@ +209,3 @@ + goto out; + + account_identities = gexiv2_metadata_get_tag_multiple (metadata, "Xmp.gnome.photos-account-identity"); This doesn't work. Unless a namespace and its properties are defined inside exiv2, it isn't possible (at least not in an easy way) to add arrays via gexiv2. Let's use GVariant's de/serialization for this. We only need an array of tuples of strings, anyway.
Created attachment 335036 [details] [review] local-item: Embed XMP metadata identifying shared copies
Created attachment 335037 [details] [review] share-point-google: Add the shared copy's ID to the original
Created attachment 335145 [details] [review] utils: Add photos_utils_object_list_free_full
Created attachment 335146 [details] [review] item-manager: Add photos_item_manager_wait_for_changes_async
Created attachment 335147 [details] [review] share-point-google: Deal with the URN changing due to metadata changes
The previous three patches are meant to work around bug 771042
Given that we are at the end of GNOME 3.21/3.22 cycle, I think we should adjust the scope of this bug and close it. The underlying plumbing to perform merging is now in place. We insert nie:links and nie:relatedTo between the origin and shared versions, and embed a 'pointer' to the shared copy if the original is a local file. With this and the changes from bug 770342 we should be in a good place. If we implement merging in a future version of gnome-photos, it should retroactively work with images shared by older versions.