GNOME Bugzilla – Bug 771825
Pass a serialized GDataEntry to the miner when inserting a shared item
Last modified: 2018-01-23 10:09:23 UTC
In bug 770267, we added a InsertSharedContent call that passes the ID of the GDataEntry that was just shared. Instead, let's pass a serialized GDataEntry so that the miner doesn't have to issue a network query to re-construct the GDataEntry. See bug 771823 for a new D-Bus API in the miner to facilitate this. It would be nice to retain support for the existing InsertSharedContent call in case we are running against an old version of the miner.
Created attachment 344488 [details] [review] gom-miner: Add InsertSharedContentWithSerializedEntry
Created attachment 344561 [details] [review] share-point-google: Pass a serialized GDataEntry to miner Passing a serialized GDataEntry to miner so that miner doesn't have to issue a network query to re-construct the GDataEntry.
Review of attachment 344488 [details] [review]: Thanks for the patches, Kartikeya! Could you please attach this one against bug 771823 ? That's the gnome-online-miners counterpart of this one.
Review of attachment 344488 [details] [review]: Oh, I am sorry. I misread your patch. This looks fine. But we also need the implementation (or server-side) of this method in gnome-online-miners (ie. bug 771823).
(In reply to Debarshi Ray from comment #4) > Review of attachment 344488 [details] [review] [review]: > Thanks for the review! > Oh, I am sorry. I misread your patch. This looks fine. But we also need the > implementation (or server-side) of this method in gnome-online-miners (ie. > bug 771823). I'll look forward to that.
Review of attachment 344561 [details] [review]: Looks quite good! I'd suggest updating the copyright notices of the files where you made significant changes. Some comments below: ::: src/photos-share-point-google.c @@ +138,3 @@ + if (g_error_matches(error, G_DBUS_ERROR, G_DBUS_ERROR_UNKNOWN_METHOD)) + { + /*old version of miner is present*/ Nitpick: the comment is not needed because the error is self-explanatory. :) @@ +143,3 @@ + GoaObject *object; + PhotosSource *source; + PhotosSharePointGoogle *source_obj; Nitpick: 'self', not 'source_obj'. The convention is to use 'self' to refer to the instance who owns the method. @@ +164,3 @@ + cancellable, + photos_share_point_google_share_insert_shared_content, + g_object_ref (task)); We shouldn't fall through to the g_task_return_boolean call further below. I'd suggest puttting the fallback code in a separate function (say photos_share_point_google_share_insert_shared_content_fallback). Then we can either call the fallback code or g_task_return_error depending on the error, and 'goto out' in both cases. @@ +192,3 @@ PhotosSharePointGoogleShareData *data; const gchar *account_id; + const gchar *serializedEntry; Nitpick: we use underscore-separated names, not camelCase, for local variables. I think 'file_entry_serialized' will be a better name because you can grep for 'file_entry'. @@ +212,3 @@ + serializedEntry = gdata_parsable_get_xml (GDATA_PARSABLE (data->file_entry)); + + gom_miner_call_insert_shared_content_with_serialized_entry (miner, What do you think about shortenening the name to InsertSharedContentSerialized? In future, this D-Bus call will be used for other online providers like Facebook, and the term 'entry' might not fit all of them. @@ +213,3 @@ + + gom_miner_call_insert_shared_content_with_serialized_entry (miner, + account_id, Nitpick: the alignment is off. @@ +215,3 @@ + account_id, + serializedEntry, + "XML", We should use the actual MIME or content type string. Since this D-Bus call is not specific to Google or libgdata it would be nicer to stick to a standard. For PicasaWeb, it will be "application/atom+xml". Bug 777980 adds new libgdata API to make it easier.
(In reply to Kartikeya Sharma from comment #5) > (In reply to Debarshi Ray from comment #4) > > Oh, I am sorry. I misread your patch. This looks fine. But we also need the > > implementation (or server-side) of this method in gnome-online-miners (ie. > > bug 771823). > > I'll look forward to that. Well, I am looking forward to your gnome-online-miners patch(es). :)
Created attachment 345115 [details] [review] gom-miner: Add InsertSharedContentSerialized
Created attachment 345119 [details] [review] share-point-google: Pass a serialized GDataEntry to miner Passing a serialized GDataEntry to miner so that miner doesn't have to issue a network query to re-construct the GDataEntry.
Created attachment 345353 [details] [review] gom-miner: Add InsertSharedContentSerialized
Created attachment 345358 [details] [review] share-point-google: Pass a serialized GDataEntry to miner Passing a serialized GDataEntry to miner so that miner doesn't have to issue a network query to re-construct the GDataEntry.
-- 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-photos/issues/58.