After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 771825 - Pass a serialized GDataEntry to the miner when inserting a shared item
Pass a serialized GDataEntry to the miner when inserting a shared item
Status: RESOLVED OBSOLETE
Product: gnome-photos
Classification: Applications
Component: general
unspecified
Other All
: Normal enhancement
: ---
Assigned To: GNOME photos maintainer(s)
GNOME photos maintainer(s)
Depends on: 771823
Blocks:
 
 
Reported: 2016-09-22 11:34 UTC by Debarshi Ray
Modified: 2018-01-23 10:09 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gom-miner: Add InsertSharedContentWithSerializedEntry (1.12 KB, patch)
2017-01-29 17:24 UTC, Kartikeya Sharma
reviewed Details | Review
share-point-google: Pass a serialized GDataEntry to miner (4.42 KB, patch)
2017-01-30 15:30 UTC, Kartikeya Sharma
none Details | Review
gom-miner: Add InsertSharedContentSerialized (1.10 KB, patch)
2017-02-07 13:17 UTC, Kartikeya Sharma
none Details | Review
share-point-google: Pass a serialized GDataEntry to miner (4.61 KB, patch)
2017-02-07 14:31 UTC, Kartikeya Sharma
none Details | Review
gom-miner: Add InsertSharedContentSerialized (1.16 KB, patch)
2017-02-09 17:51 UTC, Kartikeya Sharma
none Details | Review
share-point-google: Pass a serialized GDataEntry to miner (4.69 KB, patch)
2017-02-09 18:09 UTC, Kartikeya Sharma
none Details | Review

Description Debarshi Ray 2016-09-22 11:34:44 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.
Comment 1 Kartikeya Sharma 2017-01-29 17:24:37 UTC
Created attachment 344488 [details] [review]
gom-miner: Add InsertSharedContentWithSerializedEntry
Comment 2 Kartikeya Sharma 2017-01-30 15:30:26 UTC
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.
Comment 3 Debarshi Ray 2017-01-31 11:03:18 UTC
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.
Comment 4 Debarshi Ray 2017-01-31 11:08:10 UTC
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).
Comment 5 Kartikeya Sharma 2017-01-31 12:53:15 UTC
(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.
Comment 6 Debarshi Ray 2017-01-31 13:24:52 UTC
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.
Comment 7 Debarshi Ray 2017-01-31 13:36:09 UTC
(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). :)
Comment 8 Kartikeya Sharma 2017-02-07 13:17:19 UTC
Created attachment 345115 [details] [review]
gom-miner: Add InsertSharedContentSerialized
Comment 9 Kartikeya Sharma 2017-02-07 14:31:32 UTC
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.
Comment 10 Kartikeya Sharma 2017-02-09 17:51:49 UTC
Created attachment 345353 [details] [review]
gom-miner: Add InsertSharedContentSerialized
Comment 11 Kartikeya Sharma 2017-02-09 18:09:05 UTC
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.
Comment 12 GNOME Infrastructure Team 2018-01-23 10:09:23 UTC
-- 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.