GNOME Bugzilla – Bug 775147
Determine if sync UI should be present in 3.24
Last modified: 2016-12-28 14:29:38 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.
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.
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.
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.
(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...?
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...?
(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.
(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.
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.
The following fix has been pushed: 36a40cb Move sync support behind --enable-firefox-sync flag
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.
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.