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 775147 - Determine if sync UI should be present in 3.24
Determine if sync UI should be present in 3.24
Status: RESOLVED FIXED
Product: epiphany
Classification: Core
Component: Sync
3.23.x
Other Linux
: Normal blocker
: ---
Assigned To: Michael Catanzaro
Epiphany Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-11-26 17:14 UTC by Michael Catanzaro
Modified: 2016-12-28 14:29 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Move sync support behind --enable-firefox-sync flag (16.80 KB, patch)
2016-12-06 18:48 UTC, Michael Catanzaro
none Details | Review
Move sync support behind --enable-firefox-sync flag (15.85 KB, patch)
2016-12-06 19:14 UTC, Michael Catanzaro
committed Details | Review
Move sync support behind --enable-firefox-sync flag (15.85 KB, patch)
2016-12-07 00:26 UTC, Michael Catanzaro
committed Details | Review

Description Michael Catanzaro 2016-11-26 17:14:50 UTC
We have excellent progress on sync support thanks to Gabriel. But the user interface in the preferences dialog still needs to go through design review, and we probably don't want to expose the feature to users until it can sync more than just bookmarks. Gabriel is working on this, but it's not clear what level of progress to expect during this cycle. Also, it remains to be determined if we should block on Firefox compatibility or not; the status quo is that we use Firefox Sync but cannot actually share bookmarks with Firefox, which is pretty confusing. So while what we have so far is excellent and OK to have enabled as a secret GSettings option, it's not really ready for end users yet.

We should review the situation before the UI freeze in February and consider whether or not to hide the preferences dialog tab in the 3.24 release.
Comment 1 Michael Catanzaro 2016-12-06 18:48:59 UTC
Created attachment 341501 [details] [review]
Move sync support behind --enable-firefox-sync flag

We'll most likely leave it disabled for 3.24 so it has more time to bake
until 3.26.
Comment 2 Michael Catanzaro 2016-12-06 19:14:45 UTC
Created attachment 341502 [details] [review]
Move sync support behind --enable-firefox-sync flag

We'll most likely leave it disabled for 3.24 so it has more time to bake
until 3.26.
Comment 3 Gabriel Ivașcu 2016-12-06 19:32:32 UTC
Review of attachment 341502 [details] [review]:

Looks good to me, should be committed.

::: src/bookmarks/ephy-bookmark.c
@@ +211,3 @@
+  FILE *fp;
+  gsize num_bytes;
+  guint8 *bytes;

Since bookmark IDs exist only because the Sync Storage Server enforces them, you shouldn't have gone to the trouble of keeping them randomly generated. As far as I'm concerned, they could all have the same constant value or be NULL if Sync is disabled. But I guess this doesn't matter so much, so we can keep it this way to be 100% sure that nothing breaks.
Comment 4 Michael Catanzaro 2016-12-06 20:03:48 UTC
(In reply to Gabriel Ivascu from comment #3)
> Review of attachment 341502 [details] [review] [review]:
> Since bookmark IDs exist only because the Sync Storage Server enforces them,
> you shouldn't have gone to the trouble of keeping them randomly generated.
> As far as I'm concerned, they could all have the same constant value or be
> NULL if Sync is disabled. But I guess this doesn't matter so much, so we can
> keep it this way to be 100% sure that nothing breaks.

My concern is: what happens for bookmarks that get generated without random IDs when sync is disabled, then the user upgrades and has sync enabled and now has a bunch of bookmarks with no ID. Do you have code to handle that anywhere...?
Comment 5 Michael Catanzaro 2016-12-06 20:05:24 UTC
Hm, actually I guess if the ID is being set randomly in _init, then it's not even persistent across runs unless later set explicitly with ephy_bookmark_set_id, and we can get rid of it...?
Comment 6 Gabriel Ivașcu 2016-12-06 21:46:34 UTC
(In reply to Michael Catanzaro from comment #4)
> My concern is: what happens for bookmarks that get generated without random
> IDs when sync is disabled, then the user upgrades and has sync enabled and
> now has a bunch of bookmarks with no ID. Do you have code to handle that
> anywhere...?

Hmm good point, there is no code that handles this case. Better leave the IDs to be random (even if they're not used) to avoid this problem.
Comment 7 Gabriel Ivașcu 2016-12-06 21:48:24 UTC
(In reply to Michael Catanzaro from comment #5)
> Hm, actually I guess if the ID is being set randomly in _init, then it's not
> even persistent across runs unless later set explicitly with
> ephy_bookmark_set_id, and we can get rid of it...?

The ID becomes persistent when the bookmark is saved to the .gdvb file (see the _get_id from build_variant() in ephy-bookmarks-export.c and the _set_id from get_bookmarks_from_table() in ephy-bookmarks-import.c). When the bookmark is then loaded from file, it will have its ID set to the same value as before.

What I don't like about generating the ID in _init is that every time we create an EphyBookmark object and call _set_id right after, we end up overwriting the ID generated in _init, therefore we generate it for nothing. That is the case of https://git.gnome.org/browse/epiphany/tree/src/bookmarks/ephy-bookmark.c#n571 and https://git.gnome.org/browse/epiphany/tree/src/bookmarks/ephy-bookmarks-import.c#n80. Not sure how to fix this, so I'm open to suggestions.
Comment 8 Michael Catanzaro 2016-12-07 00:26:16 UTC
Should it become a required construct property? That should avoid the issue, except I'm not sure how to handle json_gobject_from_data. Anyway, I'll push this for now, you can think about it and propose changing it in a different bug if you want.
Comment 9 Michael Catanzaro 2016-12-07 00:26:40 UTC
The following fix has been pushed:
36a40cb Move sync support behind --enable-firefox-sync flag
Comment 10 Michael Catanzaro 2016-12-07 00:26:44 UTC
Created attachment 341522 [details] [review]
Move sync support behind --enable-firefox-sync flag

We'll most likely leave it disabled for 3.24 so it has more time to bake
until 3.26.
Comment 11 Stephen 2016-12-28 14:29:38 UTC
Where's the right place to find discussion of the addition of this feature out of interest?

The Firefox Sync platform seems to have been built with the Firefox+Mozilla server combination in mind (even though you *can* roll your own servers), and there doesn't seem to be much interest in promoting it as a long-term cross-browser sync solution with multiple backend hosting options. Mozilla has already canned and abandoned one version of their sync services; what happens if they do the same again?

It would be nice to see an Epiphany implementation of a sync service with multiple backend implementations and less reliance on continued development by a company with a history of abandoning sub-projects all over the place ;) (if such a service even exists), or while we're dreaming, maybe even something built on top of an existing sync protocol rather than a custom one.