GNOME Bugzilla – Bug 772423
Restore bookmarks import/export features
Last modified: 2016-11-30 17:51:02 UTC
We want to restore the bookmarks import feature since it would be unfortunate to lose the ability to manually import bookmarks from Firefox. (Did we ever support importing from Chrome?)
Created attachment 340919 [details] [review] bookmarks: Add import/export application menu items
Created attachment 340920 [details] [review] bookmarks: Add import/export application menu items
Created attachment 340921 [details] [review] bookmarks: Add import dialog
Created attachment 340922 [details] [review] bookmarks: Add option to import bookmarks from .gvdb file
Created attachment 340923 [details] [review] bookmarks: Add option to export bookmarks as .gvdb file
These patches should set the base for the bookmarks import/export features. Importing/exporting from/to a .gvdb file (the format epiphany uses) is not particularly useful, but it was trivial to implement as long the menu option and dialogs were created. I can remove those if you'd like so. Next, if the patches above are ok, I will work on the most requested feature, the option to import bookmarks from Firefox. After that, the option to import/export bookmarks as HTML file (Netscape Bookmark File Format) which should allow Epiphany to manually exchange bookmarks with any major browser.
(In reply to Iulian Radu from comment #6) > These patches should set the base for the bookmarks import/export features. > Importing/exporting from/to a .gvdb file (the format epiphany uses) is not > particularly useful, but it was trivial to implement as long the menu option > and dialogs were created. I can remove those if you'd like so. > > Next, if the patches above are ok, I will work on the most requested > feature, the option to import bookmarks from Firefox. After that, the option > to import/export bookmarks as HTML file (Netscape Bookmark File Format) > which should allow Epiphany to manually exchange bookmarks with any major > browser. This plan sounds good to me.
Created attachment 340974 [details] [review] bookmarks-import: Add option to import from Firefox Firefox stores bookmarks for each profile in a SQLite file, in the .mozilla/firefox/<profile>/ directory, in the users home directory. This currently works if the user only has one Firefox profile.
Created attachment 340982 [details] [review] bookmarks-import: Allow selecting a Firefox profile to import from In Firefox, an user can have multiple profiles for storing information such as bookmarks, history, cookies etc. When trying to import, if the user has multiple profiles, display a window from where the user can select the profile he wants to import from.
Review of attachment 340920 [details] [review]: ::: src/ephy-shell.c @@ +169,3 @@ + gpointer *user_data) +{ + window_cmd_import_bookmarks (NULL, NULL, NULL); OK since this is the style used elsewhere in the file, but we should really rethink the pattern of adding window_cmd functions with parameters that are never used....
Review of attachment 340921 [details] [review]: ::: src/window-commands.c @@ +124,3 @@ gpointer user_data) { + EphyWindow *window = EPHY_WINDOW (user_data); You passed NULL as the user_data in the previous patch. I guess you change that in a future patch, but you should do it in this patch.
Review of attachment 340922 [details] [review]: ::: src/bookmarks/ephy-bookmarks-import.c @@ +37,3 @@ + + /* Create a new table to hold data stored in file. */ + root_table = gvdb_table_new (filename, TRUE, NULL); Add a GError parameter to ephy_bookmarks_import, then pass it here for the third parameter... you see it will automatically be set to the right thing for the caller of ephy_bookmarks_import then. @@ +47,3 @@ + if (!table) { + res = FALSE; + goto exit; Here you would need to set the error parameter manually with g_error_new. You'll need to create your own error domain and message. Easiest way is to copy from some library you're familiar with (Gio or libsoup would be good bets). The error message could be something like "File is not a valid Epiphany bookmarks file: missing tags table". @@ +61,3 @@ + if (!table) { + res = FALSE; + goto exit; And here too. @@ +86,3 @@ + + /* Add all stored tags in a GSequence. */ + tags = g_sequence_new (g_free); This is leaked @@ +107,3 @@ + + exit: + g_strfreev (list); You learn well. :p ::: src/window-commands.c @@ +28,3 @@ #include "ephy-add-bookmark-popover.h" +#include "ephy-bookmarks-manager.h" +#include "ephy-bookmarks-import.h" Please alphabetize (reverse) these. @@ +139,3 @@ + GtkFileFilter *filter; + + filter = gtk_file_filter_new (); You need to unref it! @@ +147,3 @@ + "modal", TRUE, + "show-hidden", TRUE, + "transient-for", GTK_WINDOW (dialog), You shouldn't need the cast here. @@ +152,3 @@ + gtk_dialog_add_buttons (GTK_DIALOG (file_chooser_dialog), + _("Cancel"), GTK_RESPONSE_CANCEL, + _("Import"), GTK_RESPONSE_OK, You should add mnemonics here too! @@ +175,3 @@ + GTK_BUTTONS_OK, + imported ? "Bookmarks successfully imported!" : + "There was an error importing bookmarks!"); You forgot to use _(), translators will not be happy. :) Also, since we're writing the interface for this now, we should make ephy_bookmarks_import return a GError and display the message here, since displaying an error dialog with no reason is useless to the user. @@ +214,3 @@ + GTK_RESPONSE_OK); + gtk_style_context_add_class (gtk_widget_get_style_context (suggested), + GTK_STYLE_CLASS_SUGGESTED_ACTION); You shouldn't set the style class manually. It can be annoying and tricky to get it to work, but it should be coming automatically. See https://developer.gnome.org/Dialogs/
Review of attachment 340923 [details] [review]: ::: src/window-commands.c @@ +265,3 @@ + gtk_dialog_add_buttons (GTK_DIALOG (dialog), + _("Cancel"), GTK_RESPONSE_CANCEL, + _("Save"), GTK_RESPONSE_OK, Add mnemonics here too @@ +273,2 @@ + suggested = gtk_dialog_get_widget_for_response (GTK_DIALOG (dialog), GTK_RESPONSE_OK); + gtk_style_context_add_class (gtk_widget_get_style_context (suggested), Again, you should be getting this via gtk_dialog_set_default_response. @@ +290,3 @@ + GTK_BUTTONS_OK, + exported ? "Bookmarks successfully exported!" : + "There was an error exporting bookmarks!"); Again, you forgot to translate these. Again, you should have ephy_bookmarks_export return a GError and display a real error message here.
Review of attachment 340974 [details] [review]: ::: src/bookmarks/ephy-bookmarks-import.c @@ +128,3 @@ + EphyBookmarksManager *manager = ephy_shell_get_bookmarks_manager (ephy_shell_get_default ()); + GError *error = NULL; + const char *statement_str = "SELECT tag.title " Wow nice! @@ +139,3 @@ + &error); + if (error) { + g_warning ("Could not build tags query statement: %s", error->message); For instance, here you would just pass the error back to the caller instead of printing and freeing it. Note that if you add your own text it will then have to be translated. @@ +141,3 @@ + g_warning ("Could not build tags query statement: %s", error->message); + g_error_free (error); + return; You leak statement here. It's really better to use goto to the bottom of the function and free it there to make this harder to forget. @@ +193,3 @@ + NULL); + + connection = ephy_sqlite_connection_new (); Looks like this is leaked when there is no error. @@ +203,3 @@ + } + + statement = ephy_sqlite_connection_create_statement (connection, Looks like this is leaked when there is no error too. Wow, the opposite problem is rather more common. :) Anyway, again I think a goto the bottom of this function would really help to clean up and clarify the code, so that everything is freed in one place. The downside is that when you do that, you need to remember to set variables = NULL when necessary in the declarations and free them only if != NULL when it's possible they would be unset at that point. ::: src/bookmarks/ephy-bookmarks-import.h @@ +25,3 @@ G_BEGIN_DECLS +#define FIREFOX_PROFILES_DIR ".mozilla/firefox" Primitive. Real shame Mozilla hasn't gotten the memo to move to xdg dirs a decade later. :( @@ +27,3 @@ +#define FIREFOX_PROFILES_DIR ".mozilla/firefox" +#define FIREFOX_PROFILES_FILE "profiles.ini" +#define FIREFOX_BOOKMARKS_FILE "places.sqlite" These are not needed except in ephy-bookmarks-import.c, so they should be defined there. No reason to pollute the global namespace with a bunch of unneeded defines. ::: src/window-commands.c @@ +276,3 @@ + GTK_BUTTONS_OK, + imported ? "Bookmarks successfully imported!" : + "There was an error importing bookmarks!"); Again: don't forget to translate it, and add a GError parameter to ephy_bookmarks_import_from_firefox so you can display a real error message here.
Review of attachment 340982 [details] [review]: Nit: write "a user" in the commit message since "user" does not start with a vowel sound. I don't know how to explain when a word starts with a vowel sound or not... English is just kind of stupid sometimes. ::: src/window-commands.c @@ +331,3 @@ imported = ephy_bookmarks_import_from_firefox (manager, profiles->data); + } + else if (num_profiles != 0) { What happens if there are zero profiles? Then Firefox should not appear in the combo box at all, and you should skip straight to the gvdb file chooser. Anyway, there should be a g_assert here regardless to make sure you take one branch or the other, it doesn't make sense for this callback to silently do nothing.
Created attachment 341010 [details] [review] bookmarks: Add import/export application menu items
Created attachment 341011 [details] [review] bookmarks: Add import dialog
Created attachment 341012 [details] [review] bookmarks: Add option to import bookmarks from .gvdb file
Created attachment 341013 [details] [review] bookmarks: Add option to export bookmarks as .gvdb file
Created attachment 341014 [details] [review] bookmarks-import: Add option to import from Firefox Firefox stores bookmarks for each profile in a SQLite file, in the .mozilla/firefox/<profile>/ directory, in the users home directory. If the user has no profiles, don't display the 'Firefox' option in the impport combo box. If he has one profile, automatically import from that one. If he has multiple profiles, display a dialog which allows the user to select the profile he wants to import from.
Review of attachment 341010 [details] [review]: OK
Review of attachment 341011 [details] [review]: ::: src/window-commands.c @@ +141,3 @@ + GTK_RESPONSE_OK, + NULL); + gtk_dialog_set_default_response (GTK_DIALOG (dialog), GTK_RESPONSE_OK); You got suggested-action to work automatically? :)
Review of attachment 341012 [details] [review]: ::: src/bookmarks/ephy-bookmarks-import.c @@ +32,3 @@ +#define BOOKMARKS_IMPORT_ERROR bookmarks_import_error_quark () + + I'd prefer to leave only one blank line here instead of two. @@ +41,3 @@ +ephy_bookmarks_import (EphyBookmarksManager *manager, + const char *filename, + GError **error) Our code style is to leave one blank space between the longest type in the parameter list and the leftmost star. That is: move all the parameter names one space further to the right. @@ +91,3 @@ + /* Iterate over all keys (url's) in the table. */ + list = gvdb_table_get_names (table, &length); + for (i = 0; i < length; i++) { Can the body of this loop be split into a separate function? I think that would help readability. @@ +129,3 @@ + ephy_bookmarks_manager_add_bookmarks (manager, bookmarks); + + exit: The label should be named "out" rather than "exit", that's our convention.
Review of attachment 341013 [details] [review]: ::: src/bookmarks/ephy-bookmarks-export.c @@ +78,3 @@ +ephy_bookmarks_export (EphyBookmarksManager *manager, + const char *filename, + GError **error) Same style nit applies for everywhere you've added a GError parameter. ::: src/window-commands.c @@ +274,3 @@ + gtk_dialog_set_default_response (GTK_DIALOG (dialog), GTK_RESPONSE_OK); + + // Translators: Only translate the part before ".gvdb" (e.g. "bookmarks") Use /* */ comments... I guess it's inevitable that we start using // comments in Epiphany eventually but we haven't started doing so yet.
Review of attachment 341014 [details] [review]: Thanks :)
> @@ +27,3 @@ > +#define FIREFOX_PROFILES_DIR ".mozilla/firefox" > +#define FIREFOX_PROFILES_FILE "profiles.ini" > +#define FIREFOX_BOOKMARKS_FILE "places.sqlite" > > These are not needed except in ephy-bookmarks-import.c, so they should be > defined there. No reason to pollute the global namespace with a bunch of > unneeded defines. These are also used in window-commands.c to retrieve a list of user profiles. > You got suggested-action to work automatically? :) Yep.
Created attachment 341042 [details] [review] bookmarks: Add option to import bookmarks from .gvdb file
Created attachment 341043 [details] [review] bookmarks: Add option to export bookmarks as .gvdb file
Created attachment 341044 [details] [review] bookmarks-import: Add option to import from Firefox Firefox stores bookmarks for each profile in a SQLite file, in the .mozilla/firefox/<profile>/ directory, in the users home directory. If the user has no profiles, don't display the 'Firefox' option in the impport combo box. If he has one profile, automatically import from that one. If he has multiple profiles, display a dialog which allows the user to select the profile he wants to import from.
The following fixes have been pushed: 0de094d bookmarks-import: Add option to import from Firefox dcb0bcc bookmarks: Add option to export bookmarks as .gvdb file d46f782 bookmarks: Add option to import bookmarks from .gvdb file c195a4c bookmarks: Add import dialog 85b7776 bookmarks: Add import/export application menu items
Created attachment 341045 [details] [review] bookmarks-import: Add option to import from Firefox Firefox stores bookmarks for each profile in a SQLite file, in the .mozilla/firefox/<profile>/ directory, in the users home directory. If the user has no profiles, don't display the 'Firefox' option in the impport combo box. If he has one profile, automatically import from that one. If he has multiple profiles, display a dialog which allows the user to select the profile he wants to import from.
Created attachment 341046 [details] [review] bookmarks: Add option to export bookmarks as .gvdb file
Created attachment 341047 [details] [review] bookmarks: Add option to import bookmarks from .gvdb file
Created attachment 341048 [details] [review] bookmarks: Add import dialog
Created attachment 341049 [details] [review] bookmarks: Add import/export application menu items
You will get better with git-bz in time. :p