GNOME Bugzilla – Bug 780564
Epiphany doesn't import all bookmarks from Firefox
Last modified: 2017-04-01 07:23:04 UTC
Created attachment 348744 [details] bookmarks Epiphany doesn't import all bookmarks from Firefox using the new system. I have attached a sample to test it. To reproduce: 1. In Firefox: Import the html file attached in the bookmarks manager window. 2. In epiphany: Import bookmarks from Firefox. Expected results: All bookmarks imported. Actual results: Only some bookmarks are imported.
Created attachment 349064 [details] [review] bookmarks: Fix problem causing bookmarks to disappear after an import This fixes some bugs mainly related to importing bookmarks from Firefox. In ephy_bookmarks_manager_add_bookmarks(), g_sequence_lookup() was called on an unsorted sequence, which caused the function to fail and not add bookmarks to the manager. Besides that, the compare function used for various GSequence operations did not treat the case where the time_added was equal for both bookmarks.
Created attachment 349065 [details] [review] bookmarks-import: Display new bookmarks immediately after an import This fixes the problem where the browser has to be restarted after importing a set of bookmarks from a gvdb file/firefox profile.
Created attachment 349066 [details] [review] bookmarks-manager: Change *_add_bookmark() to be (transfer none) This should fix the inconsistency between *_add_bookmarks() and *_add_bookmark().
Review of attachment 349064 [details] [review]: Great work, thanks a bunch! ::: src/bookmarks/ephy-bookmark.c @@ +504,3 @@ + if (time2 - time1 == 0) + return g_strcmp0 (ephy_bookmark_get_title (bookmark1), I don't think this is good enough. You can have two bookmarks with the same title and time created, right? Don't you need to check the bookmark ID as well?
Review of attachment 349065 [details] [review]: ::: src/bookmarks/ephy-bookmarks-manager.c @@ +352,3 @@ + position = g_sequence_iter_get_position (new_iter); + + g_list_model_items_changed (G_LIST_MODEL (self), position, 0, 1); Don't you think it would be better without the blank line above?
Review of attachment 349066 [details] [review]: ::: src/bookmarks/ephy-bookmarks-manager.c @@ +286,3 @@ } + Don't add this blank line here.
Created attachment 349078 [details] [review] bookmarks: Fix problem causing bookmarks to disappear after an import This fixes some bugs mainly related to importing bookmarks from Firefox. In ephy_bookmarks_manager_add_bookmarks(), g_sequence_lookup() was called on an unsorted sequence, which caused the function to fail and not add bookmarks to the manager. Besides that, the compare function used for various GSequence operations did not treat the case where the time_added was equal for both bookmarks.
Created attachment 349079 [details] [review] bookmarks-import: Display new bookmarks immediately after an import This fixes the problem where the browser has to be restarted after importing a set of bookmarks from a gvdb file/firefox profile.
Created attachment 349080 [details] [review] bookmarks-manager: Change *_add_bookmark() to be (transfer none) This should fix the inconsistency between *_add_bookmarks() and *_add_bookmark().
Please have a look at attachment 349074 [details] [review] as I also changed around line 300 in ephy-bookmarks-manager.c. I don't want to break anything else.
(In reply to Iulian Radu from comment #10) > Please have a look at attachment 349074 [details] [review] [review] as I also changed > around line 300 in ephy-bookmarks-manager.c. I don't want to break anything > else. This is a GNOME Documents patch; which one did you really mean?
Review of attachment 349079 [details] [review]: OK
Review of attachment 349080 [details] [review]: OK
Review of attachment 349078 [details] [review]: ::: src/bookmarks/ephy-bookmark.c @@ +520,3 @@ + + return g_strcmp0 (id1, id2); + } This looks right now, but I'd write it differently to avoid having two levels of nesting, and to avoid the return values from being separated so far from their corresponding checks. E.g.: if (time2 - time1 != 0) return time2 - time1; if (title_result != 0) return title_result; That way, it will read more naturally/linearly.
I meant 349078, sorry!
The change looks sane to me.
The following fixes have been pushed: 4562e88 bookmarks-manager: Change *_add_bookmark() to be (transfer none) 1d8b2e3 bookmarks-import: Display new bookmarks immediately after an import aa05f49 bookmarks: Fix problem causing bookmarks to disappear after an import
Created attachment 349102 [details] [review] bookmarks-manager: Change *_add_bookmark() to be (transfer none) This should fix the inconsistency between *_add_bookmarks() and *_add_bookmark().
Created attachment 349103 [details] [review] bookmarks-import: Display new bookmarks immediately after an import This fixes the problem where the browser has to be restarted after importing a set of bookmarks from a gvdb file/firefox profile.
Created attachment 349104 [details] [review] bookmarks: Fix problem causing bookmarks to disappear after an import This fixes some bugs mainly related to importing bookmarks from Firefox. In ephy_bookmarks_manager_add_bookmarks(), g_sequence_lookup() was called on an unsorted sequence, which caused the function to fail and not add bookmarks to the manager. Besides that, the compare function used for various GSequence operations did not treat the case where the time_added was equal for both bookmarks.