GNOME Bugzilla – Bug 776738
Add a search engines manager and rework preferences dialog interface.
Last modified: 2017-02-24 22:35:35 UTC
Created attachment 342738 [details] [review] epiphany search engines manager @Michael Catanzaro. Here is a patch that : * creates an object EphySearchEnginesManager, * improves the preferences dialog UI for the Search Engines section. * uses the EphySearchEnginesManager instance Users can select the default search engine in the combo box of the preferences dialog. After that every search (via the adress bar or via the contextual menu ("Search the web for %s") use the default search engine. The EphySearchEnginesManager use the keyfile backend of gsettings.
Nice, thanks! I'll try to review this soon.
Created attachment 342907 [details] [review] Improved path for search engines manager @Michael Catanzaro I have cleaned/improved the previous patch. Please use this one instead.
Review of attachment 342907 [details] [review]: This looks pretty good for one of your first contributions. Thanks again for working on it; I can see you're building towards a huge improvement in how we manage search engines. Looking forward to maybe working with you more in the future. ;) Try not to be overwhelmed at the length of my review; this is pretty normal for new contributors as you're not very used to the code style and idioms in Epiphany yet. Functionality-wise, I'm happy with your approach. The change to the preferences dialog looks good, although I'd like to experiment with placing the Manage Search Engines button one row higher, across from the the bold Search Engines label rather than the search engine combo box, to parallel the placement of the Manage buttons on the Privacy tab. Not sure if that will look better or not. But I think you should split the new button into a follow-up patch. It's not directly used here, after all. So you can have this one patch that adds the EphySearchEngineManager, then a second patch that adds the button and the dialog. I think that's the best way to split it up. ::: data/Makefile.am @@ +25,3 @@ service_in_files = org.gnome.EpiphanySearchProvider.service.in service_DATA = $(service_in_files:.service.in=.service) + Careful to avoid spurious whitespace changes! ::: data/org.gnome.epiphany.search.engines.gschema.xml @@ +2,3 @@ + <schema id="org.gnome.Epiphany.search.engines" gettext-domain=""> + <key type="s" name="default-search-engine"> + <!-- DuckDuckGo is the default search engine. Must exactly match the URL used This translator comment doesn't make sense anymore, since it's no longer above a URL. The comment should be moved down below, to directly above the list of search-engine-urls, and updated to account for the fact that it's a list now. It also needs to be translatable using l10n="messages". See the existing gschema for an example of how to do that. The point is to allow translators to append region query parameters in order to improve country-specific search results. @@ +9,3 @@ + default-bookmarks.rdf. See the comment there for region parameters to + the URL. --> + <default>'DuckDuckGo'</default> Each key should have summary and description elements. ::: embed/ephy-embed-utils.c @@ +29,3 @@ #include "ephy-string.h" #include "ephy-view-source-handler.h" +#include "../src/ephy-search-engines-manager.h" OK, so you noticed that the src/ directory is not in the include path for embed/, and you found a workaround. I'm a little surprised and disappointed that it was able to link successfully, because this is a layering violation. The code is currently structured into layers, where higher-level layers have full access to lower-level layers, but lower-level layers have no access to the higher layers except via delegate objects. (See EphyEmbedContainer for an example of a delegate interface that allows embed/ limited access to EphyWindow, even though EphyWindow is in src/.) The levels are: * src/ * lib/widgets/ <-- confusing * embed/ * lib/ embed/ is for stuff relating to the web view. EphySearchEnginesManager has little to do with the web view, so it doesn't belong in embed/. And you need to be able to access it from embed/, so it doesn't belong in src/ either: that's higher-level. So the right place to put it would be lib/, rather than src/. Right now this is only documented on the wiki; I should really move the documentation into our HACKING file. I thought we were safe from anyone accidentally putting stuff in the wrong place due to the structure of the libtool archives, I thought it wouldn't link if you tried this, but apparently I'm wrong. Anyway, that was a long explanation, but it's fundamentally an easy problem to solve, since all you have to do is move the file to a different directory and list them in lib/Makefile.am instead of src/Makefile.am. @@ +35,3 @@ #include <JavaScriptCore/JavaScript.h> +#include <string.h> Epiphany code style is to have two sections of includes: first Epiphany project headers, alphabetized, then external headers. So please remove the blank line above this include, and add a blank line below. @@ +232,3 @@ ephy_embed_utils_autosearch_address (const char *search_key) { + char *query_param, *url_search, *default_name; I see the original code had two parameters declared on the same line, but the style I prefer is only one declaration per line for pointer types, so please move them to separate lines. @@ +236,3 @@ + EphySearchEnginesManager *manager; + manager = ephy_search_engines_manager_new (); You have to free it with g_object_unref. But it's not great to create and destroy this object each time ephy_embed_utils_autosearch_address is called. Instead, I would make the EphySearchEnginesManager a member of EphyEmbedShell. Add it to the EphyEmbedShellPriv struct in ephy-embed-shell.c, add a getter function ephy_embed_shell_get_search_engine_manager(), create it in ephy_embed_shell_startup(), and be sure to unref it in ephy_embed_shell_dispose() using g_clear_object(). Then it only has to be created/destroyed once for the duration of the program. EphyEmbedShell is a singleton object where we put all our global state, so it's kinda like having a global variable, but more organized. You can access it from anywhere in embed/ or src/ using ephy_embed_shell_get_default(). It inherits from GtkApplication so it's actually the GtkApplication instance for Epiphany. There's also an EphyShell class that lives in src/; it's subclassed, so the EphyShell global instance is the same as the EphyEmbedShell instance, but you only have access to the EphyEmbedShell stuff from here in embed/. @@ +238,3 @@ + manager = ephy_search_engines_manager_new (); + default_name = ephy_search_engines_manager_get_default (manager); + url_search = ephy_search_engines_manager_get_url (manager, default_name); OK, this works fine, I think it would be nice to add a convenience function ephy_search_engines_manager_get_default_url() that performs both of these operations at the same time, to make the simple case easy. But I'm also surprised by the ownership transfer here. _get_default is clearly transfer full, because you free the result, but _get_url is not? Why are they different? Also, since _get_url is transfer none, it should be assigned to a const char *. In GLib applications, const is used to track string ownership. But I see from the implementation that you actually do need to free the result of both of these calls, so you don't need to use const, you just need to free it. ::: src/ephy-search-engines-manager.c @@ +43,3 @@ + gchar *key_file = NULL; + gchar **search_engines_urls; + gchar **search_engines_names; Nit: I prefer singular engine. "search_engine_urls", "search_engine_names". It might seem strange if English isn't your native language, but you can think of it as a collection of search engine names/URLs, where "search engine" is a label describing the names/URLs and does not have an associated quantity. On that note, I'd prefer to change the name of the class (and the files) as well, to EphySearchEngineManager/ephy-search-engine-manager. @@ +48,3 @@ + manager->search_engines = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, g_free); + key_file = g_strdup_printf ("%s" G_DIR_SEPARATOR_S "search_engines.ini", + ephy_dot_dir ()); I know you copied this from EphyHostsManager, but please use g_build_filename instead of g_strdup_printf. I've improved the EphyHostsManager code on the wip/security-origins sidebranch if you want to copy what I did there, but it should be clear enough from the documentation how to use it. Also, I'd prefer to use a hyphen ("search-engines.ini") instead of the underscore. @@ +53,3 @@ + g_free (key_file); + + manager->settings = g_settings_new_with_backend_and_path ("org.gnome.Epiphany.search.engines", How about "org.gnome.Epiphany.search-engines" if hyphens are permitted, otherwise simply "org.gnome.Epiphany.search" @@ +55,3 @@ + manager->settings = g_settings_new_with_backend_and_path ("org.gnome.Epiphany.search.engines", + manager->backend, + "/org/gnome/epiphany/searchengines/"); Ditto. @@ +63,3 @@ + n_engines = g_strv_length (search_engines_names); + + for (i = 0; i < n_engines; ++i) { Declare i here instead of up above. Always nice for variables to have the smallest-possible scope. @@ +79,3 @@ + EphySearchEnginesManager *manager = EPHY_SEARCH_ENGINES_MANAGER (object); + + g_clear_pointer (&manager->search_engines, g_hash_table_destroy); This g_clear_pointer is correct, but... @@ +81,3 @@ + g_clear_pointer (&manager->search_engines, g_hash_table_destroy); + g_object_unref (&manager->backend); + g_object_unref (&manager->settings); ...this will crash. g_object_unref expects a GObject*, not a GObject**. The reason why you didn't notice any crashes is you forgot to unref it, so it's never destroyed currently and dispose is never called! The fix is to get rid of the &. But don't, because you can't use g_object_unref here at all. Or you can, but it would need to be more complicated, because dispose is for breaking reference cycles and it can be called many times in a row, then even fixing your call to g_object_unref it would crash the second time, because these have already been unreffed. As a rule, dispose has to leave your object in a valid state, so you have to do this: if (manager->backend != NULL) { g_object_unref (manager->backend); manager->backend = NULL; } But this used to be a very common pattern, nowadays we have g_clear_object which does the same thing in one line. Since it needs to clear the pointer location, it needs to receive a GObject** instead of a GObject*, so the call would look like g_clear_object (&manager->backend). Then the equivalent to that for non-GObjects is g_clear_pointer, which you've already used successfully up above. Same for manager->settings. P.S. As a corollary to "dispose must leave the object in a valid state", you should assume that any of the other public functions of GObjects can be called after dispose and avoid crashing (return something sane) if so. Not all of our current code does this, but it should. This is the main disadvantage of refcounted objects over the nicer C++-style RAII constructors/destructors. @@ +82,3 @@ + g_object_unref (&manager->backend); + g_object_unref (&manager->settings); + G_OBJECT_CLASS (ephy_search_engines_manager_parent_class)->dispose (object); Please leave one blank line before chaining up. @@ +100,3 @@ + +gchar * +ephy_search_engines_manager_get_url (EphySearchEnginesManager *manager, gchar *name) Please only one parameter per line in the function definition. See the other files for examples of parameter alignment. @@ +106,3 @@ + +gchar * +ephy_search_engines_manager_get_default (EphySearchEnginesManager *manager) OK, I see what you were thinking here in choosing the name for this function, but it's kinda confusing because we use _get_default() as the name for our singleton access functions, and this isn't a singleton. So I would avoid that ambiguity by naming it ephy_search_engine_manager_get_default_engine. @@ +131,3 @@ + gpointer key, value; + guint size, i; + gchar **search_engines_names, **search_engines_urls; One of the reasons I prefer to have one declaration per line when pointers are involved: you can see how it would be pretty confusing if the second variable was missing the stars, or if it only had one star instead of two. @@ +136,3 @@ + + search_engines_names = g_malloc(sizeof(gchar) *(size + 1)); + search_engines_urls = g_malloc(sizeof(gchar) *(size + 1)); sizeof(gchar) is guaranteed to be one. It's not guaranteed to be one byte, although it would be crazy for a modern system to have different-sized chars, but sizeof is defined to return multiples of sizeof(char) so even if you're pedantic you don't need to check this. ;) @@ +149,3 @@ + + g_settings_set_strv (manager->settings, "search-engines-names", (const gchar * const*) search_engines_names); + g_settings_set_strv (manager->settings, "search-engines-urls", (const gchar * const*) search_engines_urls); (const char * const *)search_engines_ @@ +157,3 @@ +void +ephy_search_engines_manager_add_engine (EphySearchEnginesManager *manager, + gchar *name, gchar *url) One parameter per line and align please @@ +165,3 @@ +void +ephy_search_engines_manager_delete_engine (EphySearchEnginesManager *manager, + gchar *name) Ditto @@ +173,3 @@ +void +ephy_search_engines_manager_modify_engine (EphySearchEnginesManager *manager, + gchar *name, gchar *url) Ditto ::: src/ephy-search-engines-manager.h @@ +20,3 @@ + +#ifndef EPHY_SEARCH_ENGINES_MANAGER_H +#define EPHY_SEARCH_ENGINES_MANAGER_H Please use #pragma once, it's harder to get wrong and is supported by all modern compilers. @@ +33,3 @@ +EphySearchEnginesManager *ephy_search_engines_manager_new (void); +gchar *ephy_search_engines_manager_get_url (EphySearchEnginesManager *, gchar *); +gchar *ephy_search_engines_manager_get_default (EphySearchEnginesManager *); Use char * rather than gchar *. @@ +38,3 @@ +void ephy_search_engines_manager_add_engine (EphySearchEnginesManager *, gchar *, gchar *); +void ephy_search_engines_manager_delete_engine (EphySearchEnginesManager *, gchar *); +void ephy_search_engines_manager_modify_engine (EphySearchEnginesManager *, gchar *, gchar*); Hm, I'm surprised it built without names for all the parameters. I thought that was required for C. At least it is still GNOME style to always name the parameters, so please let's do so here. Also, please align the declarations like you see in the other header files. ::: src/prefs-dialog.c @@ +1334,3 @@ static void +search_engine_combo_add_search_engines (GtkListStore *store, + EphySearchEnginesManager *manager) Align the parameters GNOME-style, like this: (GtkListStore *store EphySearchEnginesManager *manager) @@ +1337,2 @@ { + guint i, n_engines; Since we're not stuck on C89 anymore, go ahead and declare i in the for loop. @@ +1337,3 @@ { + guint i, n_engines; + gchar **engines_names ; You've got an extra space before the semicolon here. Also, go ahead and just use plain char **. I think the g types are kinda dumb, except for the unsigned types where they're more shorter and more convenient. Please refer to the HACKING file for code style; this is all covered there. @@ -1322,3 @@ { - guint i; - const char *default_engines[][3] = { /* Search engine option in the preferences dialog */ This is some of the first code I ever wrote for Epiphany that you are deleting. :P Ah well, it's time for it to go. ::: src/search-provider/ephy-search-provider.c @@ +285,1 @@ + manager = ephy_search_engines_manager_new (); You have to unref it. @@ +285,3 @@ + manager = ephy_search_engines_manager_new (); + default_name = ephy_search_engines_manager_get_default (manager); + url_search = ephy_search_engines_manager_get_url (manager, default_name); Free it.
Created attachment 343176 [details] [review] corrected patch for the search engine manager ✔ remove the changes related to the interface (will be done after that) ✔ check spurious whitespaces ✔ improve search.egines gschema ✔ * move translator comment and modify it ✔ * add translatable attribute ✔ * add summary and description element for each key introduced ✔ move the search engine manager source file from src/ to lib/ ✔ modify the related Makefile s ✔ add headers in alphabetical order, separate epiphany and external headers ✔ check variable declarations : one per line ✔ add a SearchEngineManager instance in the EpyEmbedShell class. ✔ add the function ephy_embed_shell_get_search_engine_manager ✔ use only transfert ownership for string or array of string returned by SearchEngineManager class ✔ use the singular forms search_engine_manager / SearchEngineManager/search-engine-manager.(c|h) ✔ use g_build_file_name instead of g_strdup_printf ✔ use search-engines.ini as name for the keyfile ✔ use org.gnome.Epiphany.search-engines, /org/gnome/epiphany/search-engines/ ✔ declare int i in the for loop. ✔ use g_clear_object instead of g_object_unref ✔ use only one parameter per line in function definition and prototype ✔ change function name ephy_search_engine_manager_get_default to ephy_search_engine_manager_get_default_engine ✔ don't check the size of a char/gchar ✔ use #pragma once ✔ don't use gchar, gint ... use char, int ... ✔ name of function parameters are optional in prototype but you are right, it is better for readability. ✔ align parameter name when multiple parameters I have a few questions: Should I always try to limit the scope of the variables or should I, by default, always declare variables at the begining of the function and possibly for counter (like int i in for loop ) ? > P.S. As a corollary to "dispose must leave the object in a valid state", you > should assume that any of the other public functions of GObjects can be called > after dispose and avoid crashing (return something sane) if so. Not all of our > current code does this, but it should. This is the main disadvantage of > refcounted objects over the nicer C++-style RAII constructors/destructors. Does that mean that I should check in each public functions the validity of the data's instance that I need ? If so what is the appropriate behavior if the object has been disposed. for example in the function ephy_search_engine_manager_add_engines , should I check if manager is not NULL, should I check if manager->search_engines is not NULL, if yes should I rebuild a new instance, should I rebuild a new hash in manager->search_engines? In order to format/check the code style, do you use any tool (such as clang-format)?
(In reply to cedlemo from comment #4) > Should I always try to limit the scope of the variables or should I, by > default, always declare variables at the begining of the function and > possibly for counter (like int i in for loop ) ? If this were a new codebase, I would say "yes, always limit scope to as small as possible." If it were C++, where we don't need to use goto for error handling, I would emphasize that the smallest-possible scope means not declaring variables at the top of functions. But this is an old C codebase. It's common for variables to be declared at the top of a function even if they are only needed inside a loop or conditional. So my answer is "it depends." Whatever is cleanest for readability. If the function is small, then declaring everything at the top is usually cleanest. When functions become larger and harder to read, then I think it's usually best to declare in the scope of the conditionals and loops. An exception would be loop control variables like i. I always declare those inside the loops. Much older Epiphany code does not do this. > > P.S. As a corollary to "dispose must leave the object in a valid state", you > > should assume that any of the other public functions of GObjects can be called > > after dispose and avoid crashing (return something sane) if so. Not all of our > > current code does this, but it should. This is the main disadvantage of > > refcounted objects over the nicer C++-style RAII constructors/destructors. > > Does that mean that I should check in each public functions the validity of > the > data's instance that I need ? If so what is the appropriate behavior if the > object has been disposed. for example in the function > ephy_search_engine_manager_add_engines , should I check if manager is not > NULL, > should I check if manager->search_engines is not NULL, if yes should I > rebuild a new instance, should I rebuild a new hash in > manager->search_engines? You can assume that manager is not null. It's best practice to assert that this is true using g_assert() (not in library code), g_return_if_fail(), or g_return_val_if_fail(). The g_return functions print critical warnings and are used in most of Epiphany, but I think it's valuable to just crash instead so we can get bug reports in these cases, so I'm planning to change them all to g_assert() eventually. But you *should* check to ensure that manager->search_engines is not NULL. Definitely do not rebuild a new instance, just return the most-harmless result (usually NULL or FALSE). Since this function has no return value, you can just return early. All that said, EphySearchEngineManager will probably never be used in such a situation. It's just best practice when writing GObjects. > In order to format/check the code style, do you use any tool (such as > clang-format)? I use uncrustify, but not regularly, maybe once a year or so. I'm definitely interested in seeing if clang-format could match Epiphany style, but writing style checker rules is quite hard.
Review of attachment 343176 [details] [review]: OK, looks like you did a good job responding to my feedback. That's great, thanks. I think there's not much value in committing this until we have GUI code that uses it, but it's a good first step. ::: src/prefs-dialog.c @@ +536,2 @@ static void +on_search_engine_dialog_button_clicked (GtkWidget *button, You'll want to move this to a later patch. @@ +597,3 @@ gtk_widget_class_bind_template_child (widget_class, PrefsDialog, download_button_hbox); gtk_widget_class_bind_template_child (widget_class, PrefsDialog, download_button_label); + gtk_widget_class_bind_template_callback (widget_class, on_search_engine_dialog_button_clicked); Ditto.
So the next step : Do I create a new patch with the GUI or should I create a patch with all the search engine manager and the GUI ? Should I had checks in the getter/setters methods in the patch as well ?
(In reply to cedlemo from comment #7) > So the next step : > > Do I create a new patch with the GUI or should I create a patch with all the > search engine manager and the GUI ? I think it's better to have two separate patches. It makes code review easier, and it also makes it easier for anyone looking at the two commits in the future. > Should I had checks in the getter/setters methods in the patch as well ? Oh, I was going to mention that but forgot! Yes please, that's a good idea.
Created attachment 343417 [details] [review] implement search engine manager The previous patch without the code for the search engine dialog button
Created attachment 343418 [details] [review] checks data validity in search engine manager methods add checks for instance data valid in the search engine manager class
Created attachment 343420 [details] [review] add button for search engine management dialog add a button in prefs dialog that will be used to display a search engine management window. I am working on the search engine dialog.
Created attachment 343426 [details] [review] implement search engine manager There was an error in the embed shell function that returns a search engine manager instance.
Created attachment 343507 [details] [review] first implementation of search engine dialog Here is a first implementation of the search engine dialog. It is not finished but I really would like that you review it and say me what you think (it can save us some time instead of waiting I fully finish it).
Review of attachment 343418 [details] [review]: OK. Of course this should be merged into the patch that adds EphySearchEnginesManager.
Review of attachment 343420 [details] [review]: OK. Should be merged into the patch that adds the dialog.
Review of attachment 343426 [details] [review]: Looking good ::: data/Makefile.am @@ +12,3 @@ +gsettings_SCHEMAS = org.gnome.epiphany.gschema.xml \ + org.gnome.epiphany.host.gschema.xml \ I like to use one or more tabs rather than a space before the backslashes, so they align nicely. ::: data/org.gnome.epiphany.search.engines.gschema.xml @@ +9,3 @@ + <default>['DuckDuckGo', 'Google', 'Bing' ]</default> + <summary>Default search engines.</summary> + <description>List of de names of the default search engines.</description> de -> the @@ +12,3 @@ + </key> + <key type="as" name="search-engines-urls"> + <!-- DuckDuckGo is the default search engine. Must exactly match the URL used "Array of default search engines. Must exactly match the URLs used..." @@ +13,3 @@ + <key type="as" name="search-engines-urls"> + <!-- DuckDuckGo is the default search engine. Must exactly match the URL used + in the preferences dialog, except this string is surrounded by single quotes "...these strings are surrounded..." @@ +14,3 @@ + <!-- DuckDuckGo is the default search engine. Must exactly match the URL used + in the preferences dialog, except this string is surrounded by single quotes + and uses & instead of simply &. If the match is not otherwise exact, "...and use &" ::: lib/ephy-search-engine-manager.c @@ +50,3 @@ + + manager->backend = g_keyfile_settings_backend_new (key_file, "/", "SearchEngines"); + g_free(key_file); Missing space before opening parentheses ::: lib/ephy-search-engine-manager.h @@ +31,3 @@ + +EphySearchEngineManager *ephy_search_engine_manager_new (void); +char *ephy_search_engine_manager_get_url (EphySearchEngineManager *manager, Please do align the function declarations in this file as done in other headers; the style is different than we use in source files. ephy-embed-shell.h is a good example of preferred style. ::: src/prefs-dialog.c @@ +1385,3 @@ NULL); + g_free (default_search_engine); + Nit: remove this extra blank line.
Review of attachment 343426 [details] [review]: ::: lib/ephy-search-engine-manager.c @@ +62,3 @@ + n_engines = g_strv_length (search_engine_names); + + for (int i = 0; i < n_engines; ++i) { Ah, careful: /home/mcatanzaro/Projects/GNOME/epiphany/lib/ephy-search-engine-manager.c:64:21: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] for (int i = 0; i < n_engines; ++i) { ^ You should use guint i = 0, that way you don't have a problem if someone adds two billion search engines. :D
Review of attachment 343426 [details] [review]: ::: lib/ephy-search-engine-manager.h @@ +32,3 @@ +EphySearchEngineManager *ephy_search_engine_manager_new (void); +char *ephy_search_engine_manager_get_url (EphySearchEngineManager *manager, + char *name); Remember: char* means transfer full, const char* means transfer none. Transfer full means you have to free it. If it's a return value, that looks like this: char *url; url = ephy_search_engine_manager_get_url (manager, name); g_free (url); If it's a parameter, you have to take ownership inside the function. Like this: char * ephy_search_engine_manager_get_url (EphySearchEngineManager *manager, char *name) { char *result; result = g_strdup (g_hash_table_lookup (manager->search_engines, name)); g_free (name); return result; } Of course, that's silly: there's no reason to transfer ownership of name. In general, strings should usually be passed as const char * unless you have some particular reason not to. @@ +33,3 @@ +char *ephy_search_engine_manager_get_url (EphySearchEngineManager *manager, + char *name); +char * ephy_search_engine_manager_get_default_engine (EphySearchEngineManager *manager); Write: char *ephy_search_engine_manager_... instead of: char * ephy_search_engine_manager_... @@ +38,3 @@ +void ephy_search_engine_manager_add_engine (EphySearchEngineManager *manager, + char *name, + char *url); Use const char * here too. Since you duplicate both name and url before inserting them into the hash table (which is good), you're not taking ownership (which is good), so the parameters are transfer none. In C, you just get some compiler warnings for getting this wrong. If this were an introspectable API, it would result in memory leaks when used from higher-level languages (JavaScript, Python, Vala) as they will never free the strings they pass in, assuming that you're going to do so inside the function. @@ +40,3 @@ + char *url); +void ephy_search_engine_manager_delete_engine (EphySearchEngineManager *manager, + char *name); Ditto @@ +43,3 @@ +void ephy_search_engine_manager_modify_engine (EphySearchEngineManager *manager, + char *name, + char *url); Ditto
Review of attachment 343426 [details] [review]: ::: src/prefs-dialog.c @@ +1331,3 @@ + for (i = 0; i < n_engines; i++) { + const char *name = engines_names[i]; + char *url = ephy_search_engine_manager_get_url (manager, name); /home/mcatanzaro/Projects/GNOME/epiphany/src/prefs-dialog.c: In function ‘search_engine_combo_add_search_engines’: /home/mcatanzaro/Projects/GNOME/epiphany/src/prefs-dialog.c:1349:62: warning: passing argument 2 of ‘ephy_search_engine_manager_get_url’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers] char *url = ephy_search_engine_manager_get_url (manager, name); ^~~~ This will be fixed when you fix the declaration of ephy_search_engine_manager_get_url. @@ +1374,3 @@ NULL); + search_engine_settings = ephy_search_engine_manager_get_settings(search_engine_manager); Missing space before opening parentheses.
Review of attachment 343507 [details] [review]: OK, this looks like a great start on the dialog! We're going to need to do some UI review on the dialog with a designer, but it's better than I was expecting for your first attempt. ::: src/ephy-search-engine-dialog.c @@ +44,3 @@ +ephy_search_engine_dialog_dispose (GObject *object) +{ + EphySearchEngineDialog *dialog = EPHY_SEARCH_ENGINE_DIALOG (object); Are you looking at warnings? I've tried to make sure there are no warnings with our standard build flags, at least with GCC 6, so if you see any fly by when compiling you should be able to assume it's your fault, unless your compiler is printing more warnings than I do. I see: /home/mcatanzaro/Projects/GNOME/epiphany/src/ephy-search-engine-dialog.c: In function ‘ephy_search_engine_dialog_dispose’: /home/mcatanzaro/Projects/GNOME/epiphany/src/ephy-search-engine-dialog.c:46:27: warning: unused variable ‘dialog’ [-Wunused-variable] EphySearchEngineDialog *dialog = EPHY_SEARCH_ENGINE_DIALOG (object); ^~~~~~ Anyway, you don't do anything inside this dispose function, so you should just remove it. @@ +51,3 @@ +static void +remove_list_box_row_cb (GtkWidget *button, + gpointer data) Again, here and everywhere in this file, you need to align the function parameters. Like you did in your other patches: static void remove_list_box_row_cb (GtkWidget *button, gpointer data) @@ +59,3 @@ + dialog = EPHY_SEARCH_ENGINE_DIALOG (data); + list_box_row = gtk_widget_get_parent (gtk_widget_get_parent (button)); + list_box = dialog->search_engine_list_box; Get rid of this local variable; it doesn't save much space on the next line. @@ +89,3 @@ + + list_box_row = gtk_list_box_row_new (); + gtk_container_add(GTK_CONTAINER (list_box_row), hbox); Missing space before opening parentheses. @@ +91,3 @@ + gtk_container_add(GTK_CONTAINER (list_box_row), hbox); + + gtk_widget_show_all(list_box_row); Ditto. @@ +93,3 @@ + gtk_widget_show_all(list_box_row); + + listbox = dialog->search_engine_list_box; /home/mcatanzaro/Projects/GNOME/epiphany/src/ephy-search-engine-dialog.c: In function ‘add_list_box_row’: /home/mcatanzaro/Projects/GNOME/epiphany/src/ephy-search-engine-dialog.c:95:11: warning: assignment from incompatible pointer type [-Wincompatible-pointer-types] listbox = dialog->search_engine_list_box; ^ You need to use a GObject cast here, since listbox is a GtkListBox* but dialog->search_engine_list_box is a GtkWidget*. You know it's a GObject subclass and thus safe to cast, because all of the data members of GtkListBox come after the members of GtkWidget in the instance struct, but the compiler has no way to known that: listbox = GTK_LIST_BOX (dialog->search_engine_list_box); @@ +94,3 @@ + + listbox = dialog->search_engine_list_box; + gtk_list_box_insert (GTK_LIST_BOX (listbox), list_box_row, position); But you don't need to cast listbox here. @@ +95,3 @@ + listbox = dialog->search_engine_list_box; + gtk_list_box_insert (GTK_LIST_BOX (listbox), list_box_row, position); + gtk_list_box_select_row (GTK_LIST_BOX (listbox), list_box_row); Or here. But you do need to cast list_box_row: /home/mcatanzaro/Projects/GNOME/epiphany/src/ephy-search-engine-dialog.c:97:52: warning: passing argument 2 of ‘gtk_list_box_select_row’ from incompatible pointer type [-Wincompatible-pointer-types] gtk_list_box_select_row (GTK_LIST_BOX (listbox), list_box_row); ^~~~~~~~~~~~ @@ +106,3 @@ + return; + + EphySearchEngineDialog *dialog; /home/mcatanzaro/Projects/GNOME/epiphany/src/ephy-search-engine-dialog.c: In function ‘list_box_row_selected_cb’: /home/mcatanzaro/Projects/GNOME/epiphany/src/ephy-search-engine-dialog.c:108:3: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement] EphySearchEngineDialog *dialog; ^~~~~~~~~~~~~~~~~~~~~~ You'll need to move all the declarations up to the top, because we use -Wdeclaration-after-statement in pretty much all of GNOME for historical reasons. @@ +110,3 @@ + GtkWidget *hbox; + GtkLabel *search_engine_label; + GtkLabel *search_engine_url; /home/mcatanzaro/Projects/GNOME/epiphany/src/ephy-search-engine-dialog.c:112:13: warning: unused variable ‘search_engine_url’ [-Wunused-variable] GtkLabel *search_engine_url; ^~~~~~~~~~~~~~~~~ @@ +123,3 @@ + gtk_entry_set_text (GTK_ENTRY (dialog->search_engine_name_entry), name); + + url = ephy_search_engine_manager_get_url (manager, name); /home/mcatanzaro/Projects/GNOME/epiphany/src/ephy-search-engine-dialog.c:125:54: warning: passing argument 2 of ‘ephy_search_engine_manager_get_url’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers] url = ephy_search_engine_manager_get_url (manager, name); ^~~~ This one is happening because name needs to be const in the declaration of ephy_search_engine_manager_get_url. @@ +140,3 @@ + uint n_engines; + + listbox = dialog->search_engine_list_box; /home/mcatanzaro/Projects/GNOME/epiphany/src/ephy-search-engine-dialog.c: In function ‘ephy_search_engine_dialog_fill_list_box’: /home/mcatanzaro/Projects/GNOME/epiphany/src/ephy-search-engine-dialog.c:142:11: warning: assignment from incompatible pointer type [-Wincompatible-pointer-types] listbox = dialog->search_engine_list_box; ^ ::: src/prefs-dialog.c @@ +568,3 @@ + GtkWidget *search_engine_dialog; + + search_engine_dialog = ephy_search_engine_dialog_new (); Hm, I wonder if it's leaked. I think so, but I do see that this is how all the other dialogs are created, so it's a preexisting problem if so. Also: /home/mcatanzaro/Projects/GNOME/epiphany/src/prefs-dialog.c: In function ‘on_search_engine_dialog_button_clicked’: /home/mcatanzaro/Projects/GNOME/epiphany/src/prefs-dialog.c:570:24: warning: assignment from incompatible pointer type [-Wincompatible-pointer-types] search_engine_dialog = ephy_search_engine_dialog_new (); ^ Since you cast it to GtkWindow everywhere below anyway, you might as well just declare it as EphySearchEngineDialog to avoid this. Or make ephy_search_engine_dialog_new() return a GtkWidget*, which is idiomatic in GTK+, although I'm not super fond of it. ::: src/resources/search-engine-dialog.ui @@ +12,3 @@ + <property name="show-close-button">True</property> + <child> + <object class="GtkButton" id="new_engine_button"> Please add a (translatable) tooltip using the tooltip_text property: "Add search engine" @@ +30,3 @@ + <child> + <object class="GtkButton" id="save_and_quit_button"> + <property name="label" translatable="yes">S_ave</property> Why can't you use _S as the accelerator here? What clashes with it? It's very common to use S as the accelerator for Save buttons so I think you should use it here, and pick a different accelerator for whatever is clashing with it. Now, here is the tricky part: the Save button needs to get the suggested-action style class, so that it turns blue (at least in Adwaita; I know you use a different theme). The best practice to ensure this is to adjust the layout of the UI file instead. See https://developer.gnome.org/Dialogs/ for example. Instead of manually accessing the internal children of the header bar using <child internal-child="headerbar">, you instead want to use <child type="action"> for the buttons, then add an <action-widgets> section at the very bottom. It's tricky if you haven't done it before, but the end result is a simpler UI file. GtkDialog is aggressive at setting and unsetting the suggested-action style class from the buttons in its header bar as it thinks is appropriate, so this is definitely the best way to make it happen. @@ +45,3 @@ + <child> + <object class="GtkBox"> + <property name="orientation">horizontal</property> The UI files get really hard to read if you don't indent consistently. You should indent two spaces for each level of XML. E.g.: <object class="GtkBox"> <child> <object class="GtkBox"> <property name="orientation">horizontal</property> <property name="visible">True</property> You'll want to reindent the whole file. :( @@ +49,3 @@ + <property name="can_focus">False</property> + <property name="margin_left">15</property> + <property name="margin_right">15</property> Try setting just the "margin" property, which should set all four margin properties at the same time. Note: if you did want to have different margins, you should still avoid using margin_left and margin_right, since they don't do what you want in right-to-left locales. Instead, use margin_start and margin_end. margin_start is equivalent to margin_left in LTR locales and margin_right in RTL locales. margin_end is of course the opposite. @@ +134,3 @@ + <property name="margin_top">6</property> + <property name="margin_bottom">6</property> + <property name="label" translatable="yes">Adress</property> Address @@ +179,3 @@ + <property name="margin_top">6</property> + <property name="margin_bottom">6</property> + <property name="label" translatable="yes">"To determine the search URL, perform a search using the search engine that you want to add and check the resulting URL. Remove the search term from the resulting URL and replace it with “%s” (without the quotes)."</property> Get rid of the quotes around the text, it's showing up literally in the dialog.
Review of attachment 343507 [details] [review]: A couple design thoughts on the dialog. First, on the spacing: * The search engine list is too narrow. Try increasing its horizontal allocation by about 40%-50%. * Not enough padding between the list of search engines on the left and the search engine information section on the right. Try increasing multiples of 6 pixels until it looks good. (For whatever reason, we usually work in multiples of 6 pixels.) * Too much whitespace at the bottom of the dialog. Try reducing it. I think we also need to consider the design guidance from https://developer.gnome.org/hig/stable/dialogs.html.en. It gives guidance on creating message dialogs, action dialogs, and presentation dialogs. I think this dialog qualifies as a presentation dialog. The guidelines further classify presentation dialogs into instant-apply and explicit-apply dialogs, and recommend using instant-apply dialogs when possible. Note that the comparable preferences dialogs (cookies, passwords) are both instant-apply. Currently your dialog is explicit-apply: we should change that. So ignore my advice above to tweak the accelerator on the Save button, and just delete the Save button: we don't need it. That also fixes the problem of having both a close button and a Save/Apply button right next to each other in the header bar, which is not a pattern we use in GNOME. (Look though the screenshots on the page and notice that none of the other dialogs use that pattern). ::: src/ephy-search-engine-dialog.c @@ +79,3 @@ + gtk_box_pack_start (GTK_BOX (hbox), label, FALSE, FALSE, 0); + + delete_button = gtk_button_new_from_icon_name ("edit-delete-symbolic", GTK_ICON_SIZE_MENU); Use list-remove-symbolic, it's more appropriate and looks much better!
(In reply to Michael Catanzaro from comment #20) > @@ +30,3 @@ > + <child> > + <object class="GtkButton" id="save_and_quit_button"> > + <property name="label" translatable="yes">S_ave</property> > > Why can't you use _S as the accelerator here? What clashes with it? It's > very common to use S as the accelerator for Save buttons so I think you > should use it here, and pick a different accelerator for whatever is > clashing with it. > > Now, here is the tricky part: the Save button needs to get the > suggested-action style class, so that it turns blue (at least in Adwaita; I > know you use a different theme). The best practice to ensure this is to > adjust the layout of the UI file instead. See > https://developer.gnome.org/Dialogs/ for example. Instead of manually > accessing the internal children of the header bar using <child > internal-child="headerbar">, you instead want to use <child type="action"> > for the buttons, then add an <action-widgets> section at the very bottom. > It's tricky if you haven't done it before, but the end result is a simpler > UI file. GtkDialog is aggressive at setting and unsetting the > suggested-action style class from the buttons in its header bar as it thinks > is appropriate, so this is definitely the best way to make it happen. Note: in my comment above I ask you to remove the Save button entirely, so don't waste time trying to figure this out. :)
Created attachment 343560 [details] [review] implement search engine manager The final (I hope) version of the search engine manager implementation with the checks in the methods.
Review of attachment 343560 [details] [review]: Almost there! ::: lib/ephy-search-engine-manager.c @@ +102,3 @@ +char * +ephy_search_engine_manager_get_url (EphySearchEngineManager *manager, + const char *name) The alignment is still off in your parameter lists, it should look like this: (EphySearchEngineManager *manager, const char * name) @@ +107,3 @@ + return NULL; + + return g_strdup (g_hash_table_lookup (manager->search_engines, name)); You can actually return a const char * from this function, just get rid of the g_strdup, adjust the declaration accordingly, and stop freeing the return value. It should work fine as long as the hash table entry doesn't get removed, in which case the caller would need to dup the string, but that's expected. @@ +181,3 @@ +ephy_search_engine_manager_add_engine (EphySearchEngineManager *manager, + const char *name, + const char *url) Ditto (alignment) @@ +192,3 @@ +void +ephy_search_engine_manager_delete_engine (EphySearchEngineManager *manager, + const char *name) Ditto @@ +204,3 @@ +ephy_search_engine_manager_modify_engine (EphySearchEngineManager *manager, + const char *name, + const char *url) Ditto ::: lib/ephy-search-engine-manager.h @@ +33,3 @@ +char *ephy_search_engine_manager_get_url (EphySearchEngineManager *manager, + const char *name); +char * ephy_search_engine_manager_get_default_engine (EphySearchEngineManager *manager); char *ephy_search_engine_manager_get_default_engine (EphySearchEngineManager *manager); ::: src/prefs-dialog.c @@ +1322,3 @@ static void +search_engine_combo_add_search_engines (GtkListStore *store, + EphySearchEngineManager *manager) This is the right way to align the parameters. :)
Created attachment 343721 [details] [review] implement search engine manager implement search engine manager. (thanks for you patience)
Review of attachment 343721 [details] [review]:
(In reply to Michael Catanzaro from comment #26) > Review of attachment 343721 [details] [review] [review]: Seems Splinter stripped out my comment. Let's try that again:
Seems Bugzilla is stripping it out. My comment was U+1F44D THUMBS UP SIGN. ;)
Something to keep in mind when working on this: we'd like to add a "keyword" feature for search engines. It would function the same as DuckDuckGo bangs. Basically, you would associate a letter or a couple letters with each search engine, then when typing in the address bar you could do this: !xzy whatever To search for "whatever" using the search engine associated with the string "xyz". If it doesn't exist it would use the default search engine instead, so normal DDG bangs would continue to function. We could use that to have native support for !g and !bing bangs, to make them faster by bypassing DDG. You don't have to implement that as part of this project... but you could. Just please keep it in mind when designing the UI, as we need room for another little text entry to hold the keyword for each engine.
Created attachment 343912 [details] [review] search engine dialog I tried to follow your recommandations, I removed the save button, modified the padding, removed the space at the bottom of the text and fix a minimum width for the listbox on the left. * displaying the search engines list ✔ done It remains some questions, assuming that the dialog is an instant apply dialog: * add a new search engine : when the user hits the add button, a new entry is added in the listbox and the related name GtkEntry is filled with "__new__" and the related url GtkEntry is filled with an empty string. When do I save the data ? - at the GtkEntry's activate signal ? (https://developer.gnome.org/gtk3/stable/GtkEntry.html#GtkEntry-activate) - at the GtkEntryBuffer inserted text ? (not my favorite) (https://developer.gnome.org/gtk3/stable/GtkEntryBuffer.html#GtkEntryBuffer-inserted-text) - at the GtkWidget focus out event ? (I prefer this one) (https://developer.gnome.org/gtk3/unstable/GtkWidget.html#GtkWidget-focus-out-event) > You don't have to implement that as part of this project... but you could. Just please keep it in mind when designing the UI, as we need room for another little text entry to hold the keyword for each engine. I would like to keep this for another patch. Give me an answer for the method to modify/save the search engine data, I implement it, you review it until you approve it then after that I that I add the search engine keyword functionality. Is it ok ?
(In reply to cedlemo from comment #30) > - at the GtkWidget focus out event ? (I prefer this one) > (https://developer.gnome.org/gtk3/unstable/GtkWidget.html#GtkWidget-focus- > out-event) Yeah, that seems good. > > You don't have to implement that as part of this project... but you could. Just please keep it in mind when designing the UI, as we need room for another little text entry to hold the keyword for each engine. > > I would like to keep this for another patch. Give me an answer for the > method to modify/save the search engine data, I implement it, you review it > until you approve it then after that I that I add the search engine keyword > functionality. > > Is it ok ? Yup.
Review of attachment 343912 [details] [review]: This is looking good. I'm going to request three more UI changes before we commit this: (1) I'd like the add and remove buttons to be underneath the list box on the left. Could you please open up System Settings, go to the Region & Language panel, and look at the +- buttons there as an example? I think that's a better pattern. You could also try to match the cookies dialog or passwords dialog, though I appear to have broken the passwords dialog by mistake in master, and neither one has an add button there. I know I told you to change the remove icons to list-remove-symbolic, but it doesn't really work like I thought it would: just looks like a bunch of hyphens. I forgot that icon only makes sense when placed next to an add button. So you then would not need the add button in the header bar, either. (2) The explanatory paragraph in the dialog is unfortunate, but required IMO. To make it look a bit nicer, try adding a dialog-info-symbolic icon of size GTK_ICON_SIZE_DIALOG to the left of the paragraph. (3) Please move selection of the default search engine into the new dialog. Replace the old combo box with a boring label: "You can add or remove search engines." Look at the label next to the clear data dialog for an example. As for where to put the setting in the new dialog... I'm not sure! I'd like to see what you come up with. ;) ::: src/ephy-search-engine-dialog.c @@ +151,3 @@ + EphySearchEngineDialog *dialog) +{ + add_list_box_row (dialog, "_new_", -1); Instead of "_new_", how about: _("New search engine") Since you're adding a new translatable string, you'll have to add this file to POTFILES.in. That reminds me, you have to add search-engine-dialog.ui to POTFILES.in as well! I always forget to do this. If it's missing, translations won't work. ::: src/resources/gtk/prefs-dialog.ui @@ +189,3 @@ + <child> + <object class="GtkButton" id="search_engine_dialog_button"> + <property name="label" translatable="yes">_Manage Search Engines</property> We forgot to use an ellipses here! @@ +226,3 @@ </child> + <child> + <!-- Time to remove this commented earlier placement of the button. ::: src/resources/gtk/search-engine-dialog.ui @@ +112,3 @@ + <property name="can_focus">False</property> + <property name="margin">6</property> + <property name="label" translatable="yes">Adress</property> Adress -> Address @@ +156,3 @@ + <property name="margin_bottom">0</property> + <property name="valign">end</property> + <property name="label" translatable="yes">To determine the search URL, perform a search using the search engine that you want to add and check the resulting URL. Remove the search term from the resulting URL and replace it with “%s” (without the quotes).</property> Let's replace "URL" here with "address" in order to match the terminology of the Address entry above. And, thinking about the text a bit more, let's clean up the end of the string a bit: "To determine the search address, perform a search using the search engine that you want to add and check the resulting address. Remove the search term from the resulting address and replace it with %s." Since you show the address of the existing search engines, that basically works as an example and should help people figure out how to use the dialog. Designers can ask us to improve it later. ;)
(In reply to Michael Catanzaro from comment #32) > We forgot to use an ellipses here! Use the actual ellispses Unicode character … rather than tying out three dots like ... (see the Manage Passwords, Manage Cookies buttons for examples)
Created attachment 344219 [details] [review] search engine dialog Here is another patch for a non finished dialog. * Firstly, what do you think of this new dialog ? * Secondly I want to remove the "name" entry in the dialog. The reason is to avoid the redundancy (search engine name in listbox and in the dialog). But a new question appears : how to modify the name of the search engine ? For this I have some possibilities : * use a GtkLabel with a button on its right in a GtkListBoxRow. The button "clicked" event could display a popover with a GtkEntry. * use a GtkEventBox with a GtkLabel inside and catch double click which could display a popover GtkEntry. * use a GtkEntry directly (which is not a good solution according comments I read) Thirdly, I am struggling with a bug and I need some help in order to fix it (I really wanted to fix it myself but unfortunatly I just lost time). If you try the patch, you will see that when you click on the "+" button (add new search engine), epiphany crash. After some tests, I think that the function that crash is in the file : lib/ephy-search-engine-manager.c:138 static void ephy_search_engine_manager_apply_settings (EphySearchEngineManager *manager) Finally, could you give me some advices on debugging with jhbuild, do you use gdb and breaks points ? PS : is there a dealine for this bug ?
(In reply to cedlemo from comment #34) > * Secondly I want to remove the "name" entry in the dialog. The reason is to > avoid the redundancy (search engine name in listbox and in the dialog). But > a new question appears : how to modify the name of the search engine ? > For this I have some possibilities : > * use a GtkLabel with a button on its right in a GtkListBoxRow. The button > "clicked" event could display a popover with a GtkEntry. > * use a GtkEventBox with a GtkLabel inside and catch double click which > could display a popover GtkEntry. > * use a GtkEntry directly (which is not a good solution according comments > I read) I wouldn't try to remove it. Users aren't going to realize they can change the name from the list box. > Finally, could you give me some advices on debugging with jhbuild, do you > use gdb and breaks points ? 'jhbuild run gdb epiphany' (or just 'gdb epiphany' if you're inside a 'jhbuild shell'). > PS : is there a dealine for this bug ? Feburary 13 is the deadline to land in Epiphany 3.24 (release: March 20, just before GNOME 3.24 release on March 22); after that it will get pushed back six months to 3.26 (release: late September). So if you want it to land in 3.24 (which would be nice) we should aim to get it in a few days before Feb. 13 so that we have extra time in case something goes wrong. It's not a big deal if it slips to 3.26, but I think we're pretty close. (In reply to cedlemo from comment #34) > * Firstly, what do you think of this new dialog ? Will check it soon. > Thirdly, I am struggling with a bug and I need some help in order to fix it > (I really wanted to fix it myself but unfortunatly I just lost time). Ditto.
Could you please rebase your patch on latest master? There are some merge conflicts in ephy-embed-shell.c.
Created attachment 344262 [details] [review] implement search engine manager after rebase This is the patch after the rebase.I don't set to obsolete the patch previous patch in case my rebase is bad
Created attachment 344263 [details] [review] search engine dialog after rebase search engine dialog after rebase.
Review of attachment 344262 [details] [review]: ::: embed/ephy-embed-utils.c @@ +244,3 @@ + + if (url_search == NULL || url_search[0] == '\0') + url_search = _("https://duckduckgo.com/?q=%s&t=epiphany"); You need to add a translator comment to indicate that translators should add a country code parameter, otherwise translators are going to have no clue how to handle this string. I'm surprised that nobody has complained about it yet. ::: src/search-provider/ephy-search-provider.c @@ +294,2 @@ + if (url_search == NULL || url_search[0] == '\0') + url_search = _("https://duckduckgo.com/?q=%s&t=epiphany"); OK, this needs to not be hardcoded in multiple places. Please move it to some appropriate header file in lib/ (you might have to make a new header if there is none that fits).
Review of attachment 344263 [details] [review]: Looks better, we're getting there! Someone on IRC noticed that it also crashes if you attempt to delete one of the search engines in addition to when you attempt to add one. I'll look at that next. It should only be possible for one search engine to be the default at any given time, so when the user toggles one search engine's toggle on, the rest need to be toggled off. And there must always be exactly one default search engine. Accordingly, it should also not be possible to remove the last search engine. (I can't test since removing an engine causes a crash.) Let's change our terminology for keywords; let's call them "bangs" instead of "keywords", to match DuckDuckGo. So please change the label next to the entry accordingly. Also, let's try to improve the layout a bit: the entry for the bang can be way smaller than the entry for the address, so how about moving the bang entry to the right of the default toggle in the grid, so that the address entry is on top and consumes two columns of the grid, with the default toggle in the bottom left and the bang entry in the bottom right? GtkGrid makes this easy, so it's good you're already using it. If a search engine is the default, then it doesn't make sense for it to have a bang, so please make that entry insensitive when a search engine is the default. (Make sure toggling the toggle button changes the sensitivity of the bang entry accordingly.) Please add default bangs for the default engines (!ddg, !g, !b). Also, since we have a full dialog now, let's add Baidu (!bd) and Yandex (!ya) as default options. ::: src/ephy-search-engine-dialog.c @@ +43,3 @@ + +static void +remove_list_box_row_cb (GtkWidget *button, /home/mcatanzaro/Projects/GNOME/epiphany/src/ephy-search-engine-dialog.c:45:1: warning: ‘remove_list_box_row_cb’ defined but not used [-Wunused-function] remove_list_box_row_cb (GtkWidget *button, ^~~~~~~~~~~~~~~~~~~~~~ @@ +55,3 @@ + list_box_row = gtk_list_box_get_selected_row (GTK_LIST_BOX (dialog->search_engine_list_box)); + + gtk_container_remove (GTK_CONTAINER (dialog->search_engine_list_box), /home/mcatanzaro/Projects/GNOME/epiphany/src/ephy-search-engine-dialog.c:58:25: warning: passing argument 2 of ‘gtk_container_remove’ from incompatible pointer type [-Wincompatible-pointer-types] list_box_row); ^~~~~~~~~~~~ (Cast it with GTK_WIDGET().) @@ +173,3 @@ + + gtk_container_remove (GTK_CONTAINER (dialog->search_engine_list_box), + list_box_row); /home/mcatanzaro/Projects/GNOME/epiphany/src/ephy-search-engine-dialog.c:175:25: warning: passing argument 2 of ‘gtk_container_remove’ from incompatible pointer type [-Wincompatible-pointer-types] list_box_row); ^~~~~~~~~~~~ (Ditto.) ::: src/resources/gtk/search-engine-dialog.ui @@ +11,3 @@ + <property name="title" translatable="yes">Manage Search Engines</property> + <property name="show-close-button">True</property> + <!-- <child> Yeah, I like it better now with the add button on the bottom-left, so you can remove this comment. @@ +151,3 @@ + <property name="can_focus">False</property> + <property name="halign">start</property> + <property name="label" translatable="yes">Search Engine Information</property> I don't think this label is helpful; let's remove it to make the dialog less cluttered. @@ +187,3 @@ + <property name="visible">True</property> + <property name="can_focus">False</property> + <property name="label" translatable="yes">Adress</property> Seriously, it's "Address" with two 'd's! I've noted this twice already up above. :( @@ +208,3 @@ + </child> + <child> + <object class="GtkEntry" id="search_engine_adress_entry"> search_engine_address_entry @@ +257,3 @@ + <property name="visible">True</property> + <property name="can_focus">False</property> + <property name="icon_name">dialog-information-symbolic</property> OK good... can you give it a bit more margin on the left and right? Another 6 pixels would probably do. @@ +271,3 @@ + <property name="can_focus">False</property> + <property name="valign">end</property> + <property name="label" translatable="yes">To determine the search URL, perform a search using the search engine that you want to add and check the resulting URL. Remove the search term from the resulting URL and replace it with “%s” (without the quotes).</property> Again, replace "URL" with "address" so that we use consistent terminology in this dialog.
(In reply to cedlemo from comment #34) > Finally, could you give me some advices on debugging with jhbuild, do you > use gdb and breaks points ? To get a backtrace for the bug: $ jhbuild run gdb epiphany (gdb) r -p (gdb is broken right now and will probably give a warning about being unable to demangle a symbol. Ignore it; press 'n' to *n*ot stop debugging.) Then run epiphany and reproduce the crash. I decided to test the crash when clicking on the delete button; you'll have to do this to figure out what's going wrong with the add button as well. (gdb) bt full That tells you where it's crashing:
+ Trace 237086
So something's going wrong on this line: search_engine_label = (gtk_container_get_children (GTK_CONTAINER (list_box_row)))->data; Since the cast is failing, I'd guess that list_box_row is not a GtkContainer anymore. My next guess is that when you removed it from the list box on the line above, that destroyed it; does that sound plausible? So try getting the text from the label *before* removing it from the list box, see if that works. Note: it's also often helpful to add printfs all over the place to figure out what's going on. I do that a lot. But with crashes it's usually easier to just get a backtrace. One more thing: you'll probably have no debuginfo, making backtraces useless, unless you edit ~/.config/jhbuildrc to set appropriate compiler flags. Maybe this was what tripped you up? I recommend: os.environ['CFLAGS'] = '-g -O0' os.environ['CXXFLAGS'] = '-g -O0' os.environ['VALAFLAGS'] = '-g'
Created attachment 344559 [details] [review] implement search engine manager all "url" references have been replaced with "address" or "addresses". all "keyword" references have been replaced with "bang". The search engine-manager now save the bang parameter for each search engine.
Created attachment 344560 [details] [review] implement search engine dialog I reworked the dialog as You had asked. But I was not sur if I should keep the labels for the default selector and the bang entry. See if you like this way or tell me how to adapt it. I have removed the combo in the pref dialog too.
Review of attachment 344559 [details] [review]: Great job! I like this approach to storing the search engine info. Thanks for being responsive and persistent through so many rounds of code review. ::: data/org.gnome.epiphany.search.engines.gschema.xml @@ +9,3 @@ + <default>['DuckDuckGo', 'Google', 'Bing', 'Baidu', 'Yandex', 'Qwant']</default> + <summary>Default search engines.</summary> + <description>List of the names of the default search engines.</description> Mention in the description that it should be kept in sync with search-engine-bangs. @@ +13,3 @@ + <key type="as" name="search-engines-addresses"> + <!-- Array of default search engines. Must exactly match the URLs used + in the preferences dialog, except this stringes are surrounded by single quotes stringes -> strings @@ +15,3 @@ + in the preferences dialog, except this stringes are surrounded by single quotes + and use & instead of simply &. If the match is not otherwise exact, + there will be a spurious, ugly entry in the preferences combo, so please We should remove the combo box for selecting the default search engine from the preferences dialog, since it's obsoleted by your dialog, so you should remove this portion of the comment. @@ +24,3 @@ + <default>['!ddg', '!g', '!b', '!bd', '!ya', '!qw' ]</default> + <summary>Default search engine keywords.</summary> + <description>List of the keywords of the default search engines.</description> Mention in the description that it should be kept in sync with search-engine-names. ::: lib/ephy-search-engine-manager.c @@ +40,3 @@ + char *address; + char *bang; +}_EphySearchEngineInfos; Let's call it "EphySearchEngineInfo" without the leading underscore or trailing 's'. Also, use two spaces for indentation, not four spaces. @@ +45,3 @@ + +static void +ephy_search_engine_infos_free (gpointer data) Then this would be named ephy_search_engine_info_free. Now let's see how we can simplify it. First, you should declare it to take an EphySearchEngineInfo * as a parameter instead of a gpointer (void *), for better type safety and to avoid the need for that first cast. Then the implementation would become: static void ephy_search_engine_info_free (EphySearchEngineInfo *info) { if (!info) return; if (info->address) g_free (info->address); if (info->bang) g_free (info->bang); g_free (info); } Next, note that g_free() (and actually the C free() as well) is NULL-safe, so the check for NULL is already performed inside the function. So you can simplify it further by removing those extra checks: static void ephy_search_engine_info_free (EphySearchEngineInfo *info) { if (!info) return; g_free (info->address); g_free (info->bang); g_free (info); } Lastly, you can simplify it even further by making ephy_search_engine_info_free() itself not NULL-safe. You don't really want NULL-safety here, because it's only ever used for freeing hash table values which you expect to never be NULL. So then it simplifies to: static void ephy_search_engine_info_free (EphySearchEngineInfo *info) { g_free (info->address); g_free (info->bang); g_free (info); } @@ +50,3 @@ + infos = (_EphySearchEngineInfos *) data; + + if(!infos) (Note: missing space before opening parentheses) @@ +64,3 @@ + +static _EphySearchEngineInfos * +ephy_search_engine_infos_new (const char *address, ephy_search_engine_info_new @@ +108,3 @@ + const char *name = search_engine_names[i]; + const char *address = search_engine_addresses[i]; + const char *bang = search_engine_bangs[i]; I think this loop would be simpler if you got rid of the three name/address/bang variable declarations and just wrote out search_engine_names/addresses/bangs[i] directly. @@ +237,3 @@ + + size = g_hash_table_size (manager->search_engines); + const char *search_engine_names[size + 1]; /home/mcatanzaro/Projects/GNOME/epiphany/lib/ephy-search-engine-manager.c:239:3: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement] const char *search_engine_names[size + 1]; ^~~~~ We shouldn't be making a variable-length stack array anyway. In general, it could blow the stack. (I understand that it's unlikely the user will configure nearly enough search engines to cause that, but we still shouldn't do it.) Please heap allocate the memory you need for these arrays with g_malloc() or g_new(), and then free them with g_strfreev(). ::: lib/ephy-search-engine-manager.h @@ +26,3 @@ +G_BEGIN_DECLS + +/// TRANSLATORS: Please modify the main address of duckduckgo in order to match Please use /* */ for comments. (It's historical.)
Review of attachment 344560 [details] [review]: Yeah, we need a label for the Bang entry, and also for the Default toggle. Users will have no clue what these do otherwise. I suppose that means the GtkGrid will have to have four columns, not two like I wrongly suggested. (The Name and Address entries will need to occupy three columns each.) Please also add a label underneath the Search Engines header in the General tab of the preferences dialog (below and to the left of the Manage Search Engines button): "You can select different search engines to use." It just looks kind of bad for that space to be empty. Lastly, it looks like we are very nearly ready to merge this. There is just one more major problem that I see, but it's a big one: we need input validation for the entries! For example: * It should not be allowed for two search engines to have the same name. * It should not be allowed for two search engines to have the same bang. * A search engine URI must be a valid URI (soup_uri_new() returns non-null). I asked about best practices for this in #gtk+ mcatanzaro: Do we have best practices for validating the contents of a GtkEntry? (How to show to the user that the contents are not right.) ebassi: Nope ebassi: Well, just to *show* we have the error CSS class mcatanzaro: Plus a tooltip mcatanzaro: That seems sufficient? ebassi: These days, we use an icon mcatanzaro: What icon? ebassi: And a label packed with the GtkEntry ebassi: An icon in the entry ebassi: Like /!\ ebassi: Tooltips do not work with touch screens, for instance mcatanzaro: And to set the error class, you just gtk_widget_get_style_context (entry) and then gtk_style_context_set_class (context, "error")? ebassi: Yes (Note: the /!\ warning icon is dialog-warning-symbolic). I'm sure you can handle it, having come this far, but be aware it might be harder than you expect! Once you've figured out how to handle that, then I think this is ready to go in (although we will need to hide the bang setting from the UI until support for that is actually implemented). ::: src/ephy-search-engine-dialog.c @@ +50,3 @@ + + children = gtk_container_get_children (container); + child = (GtkWidget *) children->data; No space after the cast: (GtkWidget *)children->data; @@ +159,3 @@ + EphySearchEngineManager *manager; + GtkWidget *list_box_row; + const char *new_engine_name = _("New search engine"); Try to find a way to do this using gtk_entry_set_placeholder_text() instead of prepopulating the actual contents of the entry with this bogus data. @@ +187,3 @@ + uint index; + + /* It should remains at least one search engine*/ /* There should remain at least one search engine */ (Note: I leave a space before the closing *) @@ +201,3 @@ + ephy_search_engine_manager_delete_engine (manager, search_engine_name); + + /* Select the previous row before removing the current one*/ Ditto (missing space before closing *) @@ +204,3 @@ + index = gtk_list_box_row_get_index (list_box_row); + if (index == 0) + index = 2; // Trick in order to select the futur first row /* Trick in order to select the future first row */
@Michael > * It should not be allowed for two search engines to have the same name. > * It should not be allowed for two search engines to have the same bang. > * A search engine URI must be a valid URI (soup_uri_new() returns non-null). I don't feel like it will be hard (maybe it is because I don't get the full picture) For example for the check of the name, I will have to check if use g_hash_table_lookup in : static gboolean name_entry_on_focus_out_cb (GtkWidget *entry, GdkEvent *event, gpointer data) and static void name_entry_on_activate_cb (GtkWidget *entry, gpointer data) if the lookup is not null, I display an icon in the related entry with gtk_entry_set_icon_from_icon_name (https://developer.gnome.org/gtk3/unstable/GtkEntry.html#gtk-entry-set-icon-from-icon-name). I set this icon to NULL if g_hash_table_lookup return NULL. Where do you want the icon ? left or right ? After that I will do the same with the bang and the address. What do you think?
Try putting the icon on the right. I took a look at https://developer.gnome.org/hig/stable/text-fields.html.en and want to point out one thing in particular: "In an instant-apply dialog, validate the contents of the entry field when it loses focus or when the window is closed, not after each keypress." Also think about how to handle what happens when the user closes the dialog while some entries are still invalid.
By the way: if you're currently a college student, you might be interested in paid work on Epiphany as part of the Google Summer of Code program. If so, either mail me or find me on IRC to ask about that.
> I took a look at https://developer.gnome.org/hig/stable/text-fields.html.en and want to point out one thing in particular: "In an instant-apply dialog, validate the contents of the entry field when it loses focus or when the window is closed, not after each keypress." I validate the process when the entry loses focus or when the user hit "Enter" not after each keypress so it should be ok. I will test the "close dialog with invalid entry" case. I am not a college student since a long time ago ;-), I am just a hobbyist.
Created attachment 344927 [details] [review] implement search engine manager Reworked search engine manager (one new method and on old method deleted)
Created attachment 344928 [details] [review] implement search engine dialog Here is a working dialog. I followed you specification. When a user edit a search engine : the name can not be an already used name the address must be validated the bang must not be used the three previous conditions must be ok in order to save the data. There is always the check before saving the data. You can close the window, if the conditions are not fulfilled, no changes are saved. When the user click on the "add" button, a new entry is created in the listbox and temporary labels are set in the entries on the right. The error state is set in order to show to the user that he must validate the data (name, address, bang). The user can not set the default-search engine if the current selected is not already registred in the search engine list. I am not sure that I have covered all the use cases. I look forward to your review.
Created attachment 344946 [details] [review] Implement search engine manager
Created attachment 344947 [details] [review] Implement search engine dialog
Created attachment 344948 [details] [review] Implement search engine manager
Created attachment 344949 [details] [review] Implement search engine dialog
(I modified the first patch to merge the search gschema into the main gschema, since I just did the same with the permissions gschema.)
Comment on attachment 344948 [details] [review] Implement search engine manager Looks good.
Review of attachment 344949 [details] [review]: Great work. Remaining problems: (1) We need to come up with something better for selecting the default engine. Having a toggle button that can't be toggled is not very good. But I don't want to block your patch on this; I think it's good enough for landing. (2) Is it possible to use the placeholder-text properly of the entries for the placeholder text? It might make your code a bit more complicated, but it would be better if the placeholder text disappeared when the user edits the entry; that's what this is intended to accomplish. Again, I don't want to block your patch on this, though. (3) I would probably leave the Bang entry empty by default, since it's optional. This is trivial to change. (4) Here is the only remaining blocker I see: I can make multiple search engines named "New search engine", and after ensuring one has a valid address and bang, then all the new ones get created with the same address and bang as the first, and changing one changes them all. This is bad. Easiest fix would be to disallow use of the name "New search engine" (make sure this works in other locales, i.e. make sure you're comparing against the translated version of the text) as that's not really a sensible name to use anyway.
We just exposed the preferences dialog in app mode, but individual settings are hidden. The entire search section will need to be hidden in app mode!
Created attachment 345054 [details] [review] implement search engine dialog When an user use the add button, a default name is created (New Search Engine 1) and incremented (I have fixed a limit UIN_MAX). I added more controls on the validation so that you can not create two same engines and modify them (I guess ).An engine name can not be * the empty string * another already used search engine name When an user delete a search engine which is the default search engine, another search engine is set as the default (the one which is selected after the deletion) I have put the default label in the placeholder of the entries like you asked. I have hidden the bang label and entry (I set the visible and focus properties to false in the src/resources/gtk/ephy-search-engine-dialog.ui) I tried to keep the code as simple as possible and I added some comments when I felt it was not clear.
> We just exposed the preferences dialog in app mode, but individual settings are hidden. The entire search section will need to be hidden in app mode! I don't get what you are trying to tell me.
(In reply to cedlemo from comment #61) > > We just exposed the preferences dialog in app mode, but individual settings are hidden. The entire search section will need to be hidden in app mode! > > I don't get what you are trying to tell me. I mean make sure the search settings are hidden in app mode and only shown in browser mode. But actually they already are. The patch that went in today hides the entire search box. So I think it's fine.
Review of attachment 345054 [details] [review]: All right! This patch looks good. A few remaining nits: * You accidentally committed the patch file search_engine_dialog.patch. You'll need to remove that. * The address entry does not get the red warning style if the user failed to put a %s in the address. It should! Looks like I forgot to mention that in my last review. Remaining work, for future patches (not this patch): (1) Rip out all the smart bookmarks code. Do you want me to do this? It might be easier, since I added it. (2) Write a profile migrator to convert smart bookmarks into search engines. I might not have time before the fast-approaching feature freeze, so do you want to try doing that? The main trick with the profile migrator would be to note that it should only ever run on the main profile, not in secondary profiles, because search engines are now a global setting. See https://git.gnome.org/browse/epiphany/commit/?id=fac43af4622b46532b298ea5611c6880844c5f0d for some detail on that, though note my approach in that commit was slightly different as in that case we did need to sometimes run the migrator in secondary profiles. For search engines, we don't have to ever run it in secondary profiles, because bookmarks and search engines are not available at all in app mode. ::: src/ephy-search-engine-dialog.c @@ +54,3 @@ + + children = gtk_container_get_children (container); + child = (GtkWidget *) children->data; No space after the cast: (GtkWidget *)children->data; @@ +72,3 @@ + + for (c = children; c != NULL; c = c->next) + { The brace goes on the line above except for loops and conditionals. Like this: for (c = children; c != NULL; c = c->next) { Please fix that everywhere. I must not have checked the code closely enough, because you made this mistake many places! @@ +153,3 @@ + { + + if (g_strcmp0 (name, search_engine_name) != 0) Here it would be better to merge this into the above condition, so you only need one conditional instead of two.
> Write a profile migrator to convert smart bookmarks into search engines. I might not have time before the fast-approaching feature freeze, so do you want to try doing that? Of course, I will look at it and try to figure it out.
(In reply to Michael Catanzaro from comment #63) > (1) Rip out all the smart bookmarks code. Do you want me to do this? It > might be easier, since I added it. OK, I did this. When I was working on it, I noticed one more problem: all the search engines except the default search engine are useless without bangs, because they do not appear in the location entry completion. So besides adding a migrator, we also need to add the search engines to the location entry completion. This patch which removes the support for doing that with smart bookmarks may serve as an example of how to add support for that using the search-engines GSettings.
Created attachment 345058 [details] [review] Remove all the smart bookmarks code It's a bit of a shame that I went to the effort of restoring all the smart bookmarks code, only to remove it now before it ever made it into a stable release, but the recent work on a real proper search engines manager is much better. Abusing the bookmarks system to configure search engines was never a good idea anyway.
Created attachment 345096 [details] [review] implement search engine manager Fixes styles issues : * "{" on the same line of if and for * remove space after cast.
Created attachment 345097 [details] [review] implement search engine dialog Fix styles issues Add regex check for "%s" in search engine address.
(In reply to cedlemo from comment #68) > Created attachment 345097 [details] [review] [review] > implement search engine dialog > > Fix styles issues > Add regex check for "%s" in search engine address. regex is overkill for this. A simple strstr (str, "%s") == NULL check would be sufficient.
Created attachment 345127 [details] [review] implement search engine dialog > regex is overkill for this. A simple strstr (str, "%s") == NULL check would be sufficient. My bad, I didn't know about it.
Michael, before I do something wrong : > So besides adding a migrator, we also need to add the search engines to the location entry completion. That means that in the src/ephy-completion-model.c in the function (l. 404) "query_completed_cb" you want me to add (at line 436 for example) this: * get the search engine manager instance in the shell * get all the search engine names and loop over them * for each name, * get the related url, * use g_regexp in order to replace "%s" with user_data->search_string * add to the list to display with : list = add_to_potential_rows (list, title, url, NULL, 0, TRUE, FALSE); Is it OK for you ?
For the profile migrator here is what I plan to do: in the src/profile-migrator/ephy-profile-migrator.c I create a new function migrate_search_engines (void) that I had in the migrators array. In this function I use the ephy_bookmark_manager_new () in order to get a list of all the bookmarks. For all the smart bookmarks, I had them as search engines via the engine manager instance that I create with and the ephy_search_engine_manager_new (). It remains one problem : How can I identify the smartbookmarks since you have removed the code of the smartbookmarks ?
(In reply to cedlemo from comment #71) > Michael, before I do something wrong : > > > So besides adding a migrator, we also need to add the search engines to the location entry completion. > > That means that in the src/ephy-completion-model.c in the function (l. 404) > "query_completed_cb" you want me to add (at line 436 for example) this: > > * get the search engine manager instance in the shell > * get all the search engine names and loop over them > * for each name, > * get the related url, > * use g_regexp in order to replace "%s" with user_data->search_string > * add to the list to display with : > > list = add_to_potential_rows (list, title, url, NULL, 0, TRUE, FALSE); > > > Is it OK for you ? Yeah that sounds good, except again you should not need GRegex here. strstr() would be faster and easier. Use strstr() to find the %s, and GString to remove it and replace it with the contents of the search. You don't need to use regular expressions to find two adjacent characters. That's about the same amount of work required to implement bangs, by the way, so we could try to get those in now too. To implement bangs you would just use strstr() and check if the returned pointer is same as the pointer to the start of the string you passed in; that will tell you if the string starts with the bang or not. And then start a search for it as you do suggest above. That would need to happen in normalize_or_autosearch_address(). Since most of the logic would be the same for both places, it suggests that most of the logic probably does not belong in EphyLocationEntryCompletion but should go somewhere else (probably EphySearchEngineManager). (In reply to cedlemo from comment #72) > For the profile migrator here is what I plan to do: > > in the src/profile-migrator/ephy-profile-migrator.c I create a new function > migrate_search_engines (void) that I had in the migrators array. > > In this function I use the ephy_bookmark_manager_new () in order to get a > list of all the bookmarks. > For all the smart bookmarks, I had them as search engines via the engine > manager instance that I create with and the ephy_search_engine_manager_new > (). Sounds good. > It remains one problem : > > How can I identify the smartbookmarks since you have removed the code of the > smartbookmarks ? Again, use strstr! A smart bookmark is any bookmark with %s in its URL, so that would look something like: url = ephy_bookmark_get_url (bookmark); is_smart = strstr (url, "%s") != NULL; Be sure to delete the smart bookmark once you've added it to the new GSetting, since the smart bookmarks will not function properly anymore with my backing code removed.
Review of attachment 345096 [details] [review]: ::: data/org.gnome.epiphany.gschema.xml @@ +323,3 @@ + <summary>Default search engine keywords.</summary> + <description>List of the bangs for the default search engines. This list should be kept in sync with search-engine-names.</description> + </key> Too much things to keep in sync. GSettings allows to store any GVariant type in values, so we don't need 4 keys that need to be in sync just to store a single list and a default value. You could use a(sss) for the list of engines, and s for the default one. So, you would have something like: <key type="s" name="default-search-engine"> <default>'DuckDuckGo'</default> <key type="a(sss)" name="search-engines"> <default>[('DuckDuckGo', 'https://duckduckgo.com/?q=%s&t=epiphany', '!ddg'),('Google', 'https://google.com/search?q=%s', '!g'),...]</default> So, we only need to make sure default-search-engine names an engine present in search-engines ::: embed/ephy-embed-shell.c @@ +1474,3 @@ + + if (!priv->search_engine_manager) + priv->search_engine_manager = ephy_search_engine_manager_new (); You are creating this unconditionally in the startup method and on demand here. Either return the value here directly, or do not create it in the startup method. ::: lib/ephy-search-engine-manager.c @@ +31,3 @@ +{ + GObject parent_instance; + GSettingsBackend *backend; Why are we using a key file backend for this? @@ +142,3 @@ + EphySearchEngineInfo *info; + + if (!manager->search_engines) How is this possible? It's created in init and destroyed in dispose. Either remove the early return or use an assert. @@ +149,3 @@ + if (info) + return info->address; + else Do not use else after a return @@ +159,3 @@ + EphySearchEngineInfo *info; + + if (!manager->search_engines) Same here @@ +166,3 @@ + if (info) + return info->bang; + else And here. @@ +173,3 @@ +ephy_search_engine_manager_get_default_engine (EphySearchEngineManager *manager) +{ + if (!manager->settings) And here. @@ +189,3 @@ + return FALSE; + + if (!g_hash_table_lookup (manager->search_engines, name)) g_hash_table_contains since we are not really interested in the key itself. Is this really expected to happen? Maybe we should show a g_warning when this happens @@ +221,3 @@ + + size = g_hash_table_size (manager->search_engines); + search_engine_names = g_malloc (sizeof (char *) * (size + 1)); g_new0 (char *, size + 1); @@ +236,3 @@ + } + + search_engine_names[size] = NULL; using g_new0 this is already NULL ::: src/search-provider/ephy-search-provider.c @@ +294,2 @@ + if (address_search == NULL || address_search[0] == '\0') + address_search = EPHY_SEARCH_ENGINE_DEFAULT_ADDRESS; So all this code is duplicated, would it be possible to move it to a common place?
@Carlos Garcia Campos any hints on how I could save/set the g_settings with an "a(sss)" format ? I have found a way to get the data from the settings : GVariantIter *iter = NULL; char *name; char *address; char *bg; g_settings_get (manager->settings, "search-engines", "a(sss)", &iter); while (g_variant_iter_next (iter, "(sss)", &name, &address, &bg)) { printf ("%s %s %s \n", name, address, bg); // g_free x 3. } but I have some difficulties to find out how to save the settings. > Why are we using a key file backend for this? It was decided at the very begining of this issue by Michael : > Yes, though since the list of search engines is profile data it should > use GSettings's keyfile backend rather than the dconf backend, and be > saved in Epiphany's profile directory (ephy_dot_dir()). I think > EphyHostManager shows how to do that. please see with him.
(In reply to Carlos Garcia Campos from comment #74) > Why are we using a key file backend for this? Looks like I wanted search engines to be profile-specific. But after last week, we now accomplish that by using a relocatable schema, which would be much better. Note: that means my above advice to run the profile migrator only for the default profile was totally wrong! We don't need to worry about limiting the migrator to run only for the default profile if search engines are kept on a per-profile basis. That actually raises questions about whether we really need to be using a key file for our permissions store as well. I guess not, except for the fact that I wound up adding some code that parses the keyfile manually to work around deficiencies in the GSettings API. :(
(In reply to cedlemo from comment #75) > @Carlos Garcia Campos any hints on how I could save/set the g_settings with > an "a(sss)" format ? You'll want to skim the documentation for GVariant: https://developer.gnome.org/glib/stable/glib-GVariantType.html https://developer.gnome.org/glib/stable/glib-GVariant.html https://developer.gnome.org/glib/stable/gvariant-format-strings.html https://developer.gnome.org/glib/stable/gvariant-text.html There's a lot, so don't get too hung up in the details; just try to get a general idea for how it works.
I want my smart bookmarks also when I open epiphany with a private profile with epiphany -p. Only web apps don't need them at all.
Then it can just be a normal non-relocatable, non-keyfile-backed GSettings schema!
I think they should be two keys in the global default schema. That's shared by all ephy instances. It'll make the code a lot simpler.
Created attachment 345512 [details] [review] implement search engine manager sorry for the delay. * the search engines are stored as an array of 3 strings (name, url, bang) * the style issues have been fixed (I guess) * I removed the following part : if (address_search == NULL || address_search[0] == '\0') address_search = EPHY_SEARCH_ENGINE_DEFAULT_ADDRESS; There is always one search engine with a valid url (there are checks in the search engine dialog) * I have added the search engines address + user search in the completion model * I have added the bang check and address + user search in the completion model
Created attachment 345513 [details] [review] implement search engine dialog Since we use the bangs in the completion model, I have set to visible the bang label and the bang entry (focus = true for this one). I have improved the check for the name entry value too.
Created attachment 345514 [details] [review] remove smart bookmarks This patch works with the previous I have created.
Created attachment 345524 [details] [review] profile migrator for search engines I extended the profile migrator in order to transform the smart bookmarks into search engines. It runs without crashing but I have not been able to test it more thant that (I don't know how to create old profile in order to test the migration).
Thanks, will review soon. (In reply to cedlemo from comment #84) > I extended the profile migrator in order to transform the smart bookmarks > into search engines. It runs without crashing but I have not been able to > test it more thant that (I don't know how to create old profile in order to > test the migration). The easiest way is to either use 'git worktree' to maintain two working directories, or just 'git checkout' back and forth between the latest commit with your profile migrator and the code two commits earlier. Here "old profile" would be any profile from after the 3.22 -> 3.23 bookmarks migration, but before your new migrator was added. Additionally, you want to be able to create smart bookmarks for testing it, so it should be any commit before "remove smart bookmarks". "implement search engine dialog" would do nicely. Just run Epiphany built on that "implement search engine dialog" commit, then the profile migration version will automatically be lowered down one. Add a couple smart bookmarks manually (bookmark pages, then modify their URLs to add %s somewhere). Then run Epiphany built on the "profile migrator for search engines" commit, and check to make sure the smart bookmarks you added get migrated correctly to search engines.
@Michael, I tested it as you told me. My code filters all the bookmarks and is able to find the smart bookmarks. The problem is that even if I add/create the new search engines in the migrator, they are not listed in the epiphany search engine dialog. I continue to search why.
(In reply to cedlemo from comment #86) > @Michael, I tested it as you told me. My code filters all the bookmarks and > is able to find the smart bookmarks. The problem is that even if I > add/create the new search engines in the migrator, they are not listed in > the epiphany search engine dialog. I continue to search why. The migrator half worked for me. It migrated the names of the search engines, but it lost the addresses. The address fields were left blank when I opened the search engine dialog. That's bad!
The migrator also failed to delete the original smart bookmarks. It should do that too.
Review of attachment 345512 [details] [review]: Hm, this isn't working quite as I'd envisioned. First problem I see is that the results for all the search engines appear at the top of the location entry completion, above all results from real bookmarks and history, which are usually more interesting. The search engines should appear at the bottom of the completion list, not at the top. I think we should present them differently in the list as well. They should not be starred, because they are not bookmarks. And there is no value to displaying the URL that will be searched, because that is almost an implementation detail. So I think the search engines should not be added in EphyCompletionModel at all. The best place to do it is probably EphyLocationController. Look at the code there that I removed in my "remove smart bookmarks" patch; notice how it adds the smart bookmarks to the end of the location entry completion, does not include the bookmarks star, and does not show the URI in the subtitle. You should accordingly be able to delete EphySearchEngineBangSearch* and ephy_search_engine_manager_build_bang_search_address(). The other problem I notice is that the bangs don't actually work. If I type, say, "!b search term" in the address bar and hit enter, then my search should be performed using the Bing search engine, not the default search engine. But right now, it's performed using the default search engine. It only seems to work if the default search engine is DuckDuckGo, because DuckDuckGo implements the bangs for us. We should handle the bangs ourselves, to avoid the redirection through DuckDuckGo. That should be done in ephy_embed_utils_normalize_or_autosearch_address(). It should be easy, but don't get hung up on it since we're close to the freeze deadline and bangs aren't necessary for the rest of this to land. All of the above would probably be better done in a separate patch, rather than further-complicating this already-large patch with changes to the completion model, location controller, or autosearch function. But if it's easier for you to add it to the current patch, that's fine. ::: data/org.gnome.epiphany.gschema.xml @@ +7,3 @@ <child schema="org.gnome.Epiphany.lockdown" name="lockdown"/> <child schema="org.gnome.Epiphany.permissions" name="permissions"/> + <child schema="org.gnome.Epiphany.search" name="search"/> This is unused now and should be removed! @@ +38,3 @@ + ('Google', 'https://google.com/search?q=%s', '!g'), + ('Bing', 'https://www.bing.com/search?q=%s', '!b'), + ('Baidu', 'http://www.baidu.com/s?wd=%s', '!db'), This should be !bd ::: src/ephy-completion-model.c @@ +455,3 @@ + length = g_strv_length (search_engines); + + for(uint i = 0; i < length; i++) { Leave a space before the opening parentheses. @@ +458,3 @@ + search_address = ephy_search_engine_manager_build_search_address (manager, + search_engines[i], + user_data->search_string); This is misaligned.
Review of attachment 345513 [details] [review]: OK
Review of attachment 345514 [details] [review]: OK
Review of attachment 345524 [details] [review]: Ah, this migrator was much easier than I expected! There's not much room for error here; it looks good. Since it's not working quite right, I'd suspect a problem inside ephy_search_engine_manager_add_engine(). ::: src/profile-migrator/ephy-profile-migrator.c @@ +930,3 @@ + address = ephy_bookmark_get_url (bookmark); + + if (strstr (address, "%s") != NULL) { One more thing: delete the bookmark! Call ephy_bookmarks_manager_remove_bookmark() inside this condition, after the call to ephy_search_engine_manager_add_engine(). @@ +941,3 @@ + + g_clear_object (&bookmarks_manager); + g_clear_object (&search_engine_manager); Just use g_object_unref(). The only reason to use g_clear_object() is if you need to NULL out the pointers, but these are local variables and it's the bottom of the function, so no reason to do that.
Review of attachment 345512 [details] [review]: ::: lib/ephy-search-engine-manager.c @@ +27,3 @@ +#include "ephy-prefs.h" +#define G_SETTINGS_ENABLE_BACKEND 1 +#include <gio/gsettingsbackend.h> You no longer need to define G_SETTINGS_ENABLE_BACKEND or include gsettingsbackend.h. @@ +32,3 @@ +{ + GObject parent_instance; + GSettingsBackend *backend; backend is now unused. Remove it! @@ +33,3 @@ + GObject parent_instance; + GSettingsBackend *backend; + GSettings *settings; You can get rid of this too... @@ +77,3 @@ + (GDestroyNotify)ephy_search_engine_info_free); + + manager->settings = g_settings_new (EPHY_PREFS_SCHEMA); ...no need to create your own GSettings object here or worry about clearing it in dispose. Just #include "ephy-settings.h" and use EPHY_SETTINGS_MAIN instead of manager->settings. @@ +80,3 @@ + g_settings_get (manager->settings, "search-engines", "a(sss)", &iter); + + while (g_variant_iter_next (iter, "(sss)", &name, &address, &bang)) { Good!
Created attachment 345581 [details] [review] implement search engine manager I have used EPHY_PREFS_SCHEMA and cleaned the ephy-search-engine-manager.c file. I have removed all the code related to ephy-completion-model.c. I will add the code in ephy-location-controller.c in an another patch (I hope I will be able to create it before you release the new version)
Review of attachment 345581 [details] [review]: OK!
About the migrator : > Since it's not working quite right, I'd suspect a problem inside ephy_search_engine_manager_add_engine(). The weird thing is in an epiphany session, I am able to add delete search engines. When I close the search engine dialog and reopen it, the search engines remains. But if I close the session of epiphany and reopen it, the modification I added are gone. When I launch epiphany, I have this output: > ./jhbuild run epiphany > Gtk-Message: Failed to load module "canberra-gtk-module" > Gtk-Message: Failed to load module "canberra-gtk-module" > GLib-GIO-Message: Using the 'memory' GSettings backend. Your settings will not be saved or shared with other applications. > > ** (epiphany:5532): WARNING **: Error retrieving filter https://easylist.to/easylist/easylist.txt: Opération non prise en charge > > ** (epiphany:5532): WARNING **: Error retrieving filter https://easylist.to/easylist/easyprivacy.txt: Opération non prise en charge > > GLib-GIO-Message: Using the 'memory' GSettings backend. Your settings will not be saved or shared with other applications. > Gtk-Message: Failed to load module "canberra-gtk-module" > Gtk-Message: Failed to load module "canberra-gtk-module" > GLib-GIO-Message: Using the 'memory' GSettings backend. Your settings will not be saved or shared with other applications. > Gtk-Message: Failed to load module "canberra-gtk-module" > Gtk-Message: Failed to load module "canberra-gtk-module" > GLib-GIO-Message: Using the 'memory' GSettings backend. Your settings will not be saved or shared with other applications. Maybe it is related to the jhbuild environment ?
Created attachment 345583 [details] [review] remove smart bookmarks patch compatible with the modifications of the last search engine manager patch
Ahaha, yeah! You need to 'jhbuild build dconf' if you want your settings to persist; otherwise they're just stored in the program's memory. Well that explains why your migrator is "broken." ;) Once you've fixed that, test it again to be sure it works. Remember that when I tried it the address did not get migrated for some reason. Unfortunately feature freeze for 3.24 is tomorrow. :( If you want to try to sneak this in before freeze, please join the #epiphany IRC channel tomorrow so I know not to release if you're actively working on it. It looks like the only remaining issues here are (a) adding the search engines to the entry completion without touching the completion model, using the ephy-location-controller.c code for smart bookmarks that is removed in attachment #345583 [details], and (b) optionally implementing the bang autosearch, which would be very nice to sneak in since it seems quick and easy.
Review of attachment 345581 [details] [review]: ::: embed/ephy-embed-shell.c @@ +967,3 @@ g_free (cookie_policy); + priv->search_engine_manager = ephy_search_engine_manager_new (); I know I suggested you to do this or on demand. Now that I think about it again, I really prefer to do this on demand, because that way this won't be created for web apps. ::: lib/ephy-search-engine-manager.c @@ +74,3 @@ + (GDestroyNotify)ephy_search_engine_info_free); + + g_settings_get (EPHY_SETTINGS_MAIN, "search-engines", "a(sss)", &iter); Please, add a macro for this new key to ephy-prefs.h and use it here instead. @@ +76,3 @@ + g_settings_get (EPHY_SETTINGS_MAIN, "search-engines", "a(sss)", &iter); + + while (g_variant_iter_next (iter, "(sss)", &name, &address, &bang)) { You are leaking the parameters name, address and bang. You could use (&s&s&s) and const char * to avoid the leaks. Or (s&s&s) and you pass the name idrectly to the hash table without g_strdup(). @@ +82,3 @@ + g_hash_table_insert (manager->search_engines, + g_strdup (name), + info); All these could be just one line if you pass ephy_search_engine_info_new directly to the hash table. @@ +141,3 @@ +ephy_search_engine_manager_get_default_engine (EphySearchEngineManager *manager) +{ + return g_settings_get_string (EPHY_SETTINGS_MAIN, "default-search-engine"); I wonder if we really need to wrap this. @@ +151,3 @@ + return FALSE; + + return g_settings_set_string (EPHY_SETTINGS_MAIN, "default-search-engine", name); And this. @@ +169,3 @@ + + while (g_hash_table_iter_next (&iter, &key, NULL)) { + search_engine_names[i] = g_strdup((char *) key); search_engine_names[i++] = g_strdup((char *) key); ? @@ +186,3 @@ + GVariant *variant; + + builder = g_variant_builder_new (G_VARIANT_TYPE ("a(sss)")); Use a stack allocated builder here. This would be g_variant_builder_init @@ +195,3 @@ + } + variant = g_variant_new ("a(sss)", builder); + g_variant_builder_unref (builder); And here g_variant_builder_end returns the variant @@ +236,3 @@ +} + +char * I think this could return a const char* @@ +266,3 @@ + + strings = g_strsplit (string, pattern, -1); + length = g_strv_length (strings); This is iterating the array to get the length, you don't really need to know the length in advance to iterate below, that's why it's null terminated. @@ +270,3 @@ + buffer = g_string_new (NULL); + + for (uint i = 0; i < length; i++) { for (guint i = 0; string[i]; i++) {
> You are leaking the parameters name, address and bang. You could use (&s&s&s) and const char * to avoid the leaks. You mean that those allocated memory are not free ? I thougth that `g_variant_iter_next (iter, "(sss)` would create an array of pointer of strings. Now that you tell me to use (&s&s&s), I am not sure anymore. Could you explain me the differences ? > I wonder if we really need to wrap this. I thought (maybe it is not very smart), it could help with the readability/maintenance to offer an extensive API for the manipulations of search engines throught the search engine manager. For example, when I started to read the code, I read the header files in order to see the functions to use but sometimes, I had to use ephy_an_object_(get/set)_data and sometimes, I had to search in the c files how to use g_settings_* with a macro used in the ephy-prefs.c. If you really want, I can get ride of those two functions and use the g_settings_ form.
@carlos : > I know I suggested you to do this or on demand. Now that I think about it again, I really prefer to do this on demand, because that way this won't be created for web apps. Should I keep it in the embed shell ?
(In reply to cedlemo from comment #100) > > You are leaking the parameters name, address and bang. You could use (&s&s&s) and const char * to avoid the leaks. > > You mean that those allocated memory are not free ? > > I thougth that `g_variant_iter_next (iter, "(sss)` would create an array of > pointer of strings. Now that you tell me to use (&s&s&s), I am not sure > anymore. When creating a GVariant, there is no difference between 's' and '&s'. When getting values from an existing GVariant, 's' means a pointer to a copy of a string that you have to free yourself; store it in a 'char *' to indicate this. '&s' means a pointer to the GVariant's internal string, which you must not free; store it in a 'const char *' to indicate this. So you need to use 's' when you need ownership and '&s' otherwise. > > I wonder if we really need to wrap this. > > I thought (maybe it is not very smart), it could help with the > readability/maintenance to offer an extensive API for the manipulations of > search engines throught the search engine manager. I agree; it's fine to keep these functions. Please fix the other things Carlos mentioned, though.
(In reply to cedlemo from comment #101) > @carlos : > > > I know I suggested you to do this or on demand. Now that I think about it again, I really prefer to do this on demand, because that way this won't be created for web apps. > > Should I keep it in the embed shell ? Yes. See ephy_embed_shell_get_downloads_manager() for an example of creating the object on demand.
Created attachment 345836 [details] [review] implement search engine manager I really want to apologize for not having been able to do all of the patches in time. In this search engine manager, you will see that I have added the bang support in the location-controller. I have done the rebase too.
Created attachment 345837 [details] [review] implement search engine dialog some minor fixes.
Created attachment 345838 [details] [review] remove smart bookmarks minor fixes + rebase
Created attachment 345842 [details] [review] add search engines in the popover of the title bar This patch add the search engines in the popover and allow the user to use them with the search pattern they typed.
Created attachment 345869 [details] [review] implement search engine manager Fix an little error.
(In reply to cedlemo from comment #104) > Created attachment 345836 [details] [review] [review] > implement search engine manager > > I really want to apologize for not having been able to do all of the patches > in time. That's OK. It will be the first big feature for our next release cycle. And we'll have six months to test it, instead of throwing it in at the last minute and hoping that nothing breaks.
Review of attachment 345869 [details] [review]: ::: lib/ephy-search-engine-manager.c @@ +159,3 @@ + char **search_engine_names; + uint size; + uint i = 0; guint @@ +180,3 @@ + char **search_engine_bangs; + uint size; + uint i = 0; guint @@ +278,3 @@ + gchar **strings; + GString *buffer; + uint length; /home/mcatanzaro/Projects/GNOME/epiphany/lib/ephy-search-engine-manager.c: In function ‘ephy_search_engine_manager_replace_pattern’: /home/mcatanzaro/Projects/GNOME/epiphany/lib/ephy-search-engine-manager.c:280:8: warning: unused variable ‘length’ [-Wunused-variable] uint length; ^~~~~~ By the way, I don't know what header uint comes from, but you should use guint instead. (Did you just recently start using uint, or did I really not notice this before now?) @@ +284,3 @@ + buffer = g_string_new (NULL); + + for (uint i = 0; strings[i] != NULL; i++) { Ditto.
Review of attachment 345842 [details] [review]: Please try to be more careful about compiler warnings! We want to maintain a warning-free build. ::: src/ephy-location-controller.c @@ +300,3 @@ + return; + + char **engines_names = ephy_search_engine_manager_get_names (controller->search_engine_manager); /home/mcatanzaro/Projects/GNOME/epiphany/src/ephy-location-controller.c: In function ‘action_activated_cb’: /home/mcatanzaro/Projects/GNOME/epiphany/src/ephy-location-controller.c:302:3: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement] char **engines_names = ephy_search_engine_manager_get_names (controller->search_engine_manager); ^~~~ @@ +318,3 @@ + GtkEntryCompletion *completion = gtk_entry_get_completion (GTK_ENTRY (lentry)); + EphyEmbedShell *shell; + EphySearchEngineManager *manager; /home/mcatanzaro/Projects/GNOME/epiphany/src/ephy-location-controller.c:320:28: warning: variable ‘manager’ set but not used [-Wunused-but-set-variable] EphySearchEngineManager *manager; ^~~~~~~ @@ +320,3 @@ + EphySearchEngineManager *manager; + shell = ephy_embed_shell_get_default (); + manager = ephy_embed_shell_get_search_engine_manager (shell);// GSequenceIter *iter; Remove the commented out code! @@ +321,3 @@ + shell = ephy_embed_shell_get_default (); + manager = ephy_embed_shell_get_search_engine_manager (shell);// GSequenceIter *iter; + char **engines_names = ephy_search_engine_manager_get_names (controller->search_engine_manager); /home/mcatanzaro/Projects/GNOME/epiphany/src/ephy-location-controller.c: In function ‘add_completion_actions’: /home/mcatanzaro/Projects/GNOME/epiphany/src/ephy-location-controller.c:323:3: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement] char **engines_names = ephy_search_engine_manager_get_names (controller->search_engine_manager); ^~~~
Review of attachment 345842 [details] [review]: OK, this seems to work almost properly. There is one problem: it doesn't listen to settings changed signals, so you have to restart the browser for search engine changes to take effect. To fix that, add a search-engines-changed signal to EphySearchEngineManager so that EphyLocationController can know when it needs to update its list of completion actions. You can implement that signal using the GSettings::changed signal. But beware, there is a trick: the order of connection matters. According to the documentation for GSettings::changed: "Note that settings only emits this signal if you have read key at least once while a signal handler was already connected for key ." which is dumb, and which causes all sorts of sad bugs. Hence my warning to watch out for it.
The bangs seem to work great. Thanks. :)
(In reply to Michael Catanzaro from comment #109) > That's OK. It will be the first big feature for our next release cycle. And > we'll have six months to test it, instead of throwing it in at the last > minute and hoping that nothing breaks. We might request freeze break approval for this if you're able to finish soon. It feels like we're very nearly done, but the settings dialog not working until browser restart is a blocker.
Created attachment 345982 [details] [review] implement search engine manager * use guint instead of uint. * remove unused variable declaration It seems that uint is a non standard typedef for unsigned int (sys/types.h) I used it because you told me that I should not use the g* version of types (char instead of gchar, int instead of gint). There is a line in the coding style of the HACKING file that says the same.
(In reply to cedlemo from comment #115) > It seems that uint is a non standard typedef for unsigned int (sys/types.h) > I used it because you told me that I should not use the g* version of types > (char instead of gchar, int instead of gint). There is a line in the coding > style of the HACKING file that says the same. Ah, that applies to the standard types where the g is just stupid... I had no clue uint was defined in sys/types.h, that's just being pulled in by luck. :)
Created attachment 345986 [details] [review] implement search engine manager Fix an error in the arguments order for ephy_search_engine_modify_engine
Created attachment 345987 [details] [review] implement search engine dialog replace all the uint with guint improve for loop with a check on not NULL pointer instead of check of equal length. Fix issue with bad arguments order for ephy_search_engine_manager_add_engine and ephy_search_engine_manage_modify_engine.
Created attachment 346144 [details] [review] problem with callback when catch search engine modification @Michael, I think I am close but I am stuck . I created a function in the search engine manager : >+void >+ephy_search_engine_manager_connect_to_changes (EphySearchEngineManager *manager, >+ GCallback handler, >+ gpointer user_data) That I use in ephy-location-controller.c. I tested this function with a simple callback that print output whenever I add or remove a search engine. The problem come when I pass data throught the user_data parameter. In the patch, the callback called search_engines_completion_cb crash when I check the user_data : >+static void >+search_engines_completion_cb (GSettings settings, >+ char *key, >+ gpointer data) >+{ >+ EphyLocationController *controller; >+ GtkEntryCompletion *completion; >+ GtkEntry *entry; >+ char **engines_names; >+ >+ controller = EPHY_LOCATION_CONTROLLER (data); I don't understand why this is not working.
I don't see why it's crashing, but this isn't the right approach. You should just define a changed signal in class_init instead (examples of doing this in many files), and have the location controller connect to that. Then have EphySearchEngineManager connect to the GSettings changed signal, and emit its own changed signal when the GSettings changed signal occurs.
Review of attachment 346144 [details] [review]: ::: src/ephy-location-controller.c @@ +333,3 @@ + + if (controller->search_engine_actions_number != 0) + controller->search_engine_actions_number++; // increment in order to transform an index to a nomber of actions number
Created attachment 346200 [details] [review] add search engines in the popover of the title bar I have created a signal for the EphySearchEngineManager like you asked. I didn't know about it, it is very interesting.
Review of attachment 346200 [details] [review]: Great, it seems to be almost perfectly now. My only remaining concern is that the search engines appear in the completion in a strange order: the arbitrary order that they exist in the GSettings key, rather than the order they are displayed in the search engines dialog (alphabetical). They should be alphabetical IMO. That should be handled in EphySearchEngineManager IMO, not EphyLocationController. But that's easy to clean up later. This is good enough now that I'm going to go ahead and request a freeze exception. To save time, I'm going to make the trivial changes that I suggest in this review, and upload a rebased patch series. Note that I'm going to obsolete all previous patches and upload the new set in order, to make it easier for people to apply the patches all at once using 'git bz' without having to drop to interactive mode to figure out what needs to be reordered and obsoleted. One thing to note is that a "popover" is a specific user interface element with different meaning. You're modifying a GtkEntryCompletion, not a GtkPopover. For an example of a GtkPopover, look at the menus on the right-hand side of the header bar. So I'll change the commit message accordingly. One last general note: it's awkward to have such a large list of search engines in the completion entry by default, so I think we should trim down the default list to just DuckDuckGo, Google, and Bing, which is the same list we used before. ::: lib/ephy-search-engine-manager.c @@ +28,3 @@ #include "ephy-prefs.h" +static guint signal_search_engines_changed; Please look at one of our existing files (e.g. ephy-bookmark.c) to see the code style we use for this. It should be an array of signal IDs. Like this: enum { SEARCH_ENGINES_CHANGED, LAST_SIGNAL }; static guint signals[LAST_SIGNAL]; That's just the convention we use. @@ +65,2 @@ static void +emit_signal_search_engines_changed_cb (GSettings *settings, Name this function search_engines_changed_cb. Callbacks should be named after the function that triggers the callback, *not* what the function itself does. Otherwise it'd get really confusing in files with lots of callback functions! @@ +71,3 @@ + manager = EPHY_SEARCH_ENGINE_MANAGER (user_data); + + g_signal_emit (manager, signal_search_engines_changed, 0); Then here you'd write signals[SEARCH_ENGINES_CHANGED] instead of signal_search_engines_changed. @@ +118,3 @@ object_class->dispose = ephy_search_engine_manager_dispose; + + signal_search_engines_changed = g_signal_new ("changed", And the left-hand-side would also be signals[SEARCH_ENGINES_CHANGED] ::: src/ephy-location-controller.c @@ +54,3 @@ gboolean sync_address_is_blocked; + EphySearchEngineManager *search_engine_manager; + guint search_engine_actions_number; num_search_engine_actions @@ +296,3 @@ + char *content; + char *url; + char **engines_names; Should be "engine_names". I guess French handles plurals differently than English? @@ +310,3 @@ + content); + g_strfreev (engines_names); + ephy_link_open (EPHY_LINK (controller), url, NULL, There should be a blank line here. I don't like freeing stuff in the middle of a code block; that hurts readability. @@ +311,3 @@ + g_strfreev (engines_names); + ephy_link_open (EPHY_LINK (controller), url, NULL, + ephy_link_flags_from_current_event () | EPHY_LINK_TYPED); It's not right to use EPHY_LINK_TYPED here, because the user did not type the address. I'm going to remove it. I guess you just copy/pasted it, and it's not here to fix anything specific? @@ +327,3 @@ + + for (guint i = 0; engines_names[i] != NULL; i++) { + const char *name = engines_names[i]; It's not needed, you can just use engines_names[i] on the next line. @@ +329,3 @@ + const char *name = engines_names[i]; + gtk_entry_completion_insert_action_text (completion, i, name); + controller->search_engine_actions_number = i; This should be controller->search_engine_actions_number++... @@ +333,3 @@ + + if (controller->search_engine_actions_number != 0) + controller->search_engine_actions_number++; // increment in order to transform an index to a number of actions ...so that we can remove this block of code. ;) Also, remember that we use only /* */ comments in Epiphany. @@ +350,3 @@ + +static void +add_completion_actions_on_search_engines_change_cb (EphySearchEngineManager *manager, search_engines_changed_cb
Also I've changed the commit message of the profile migrator patch. The first line of the commit message really ought to be limited to at most 72 characters in all git projects. (Some maintainers have even shorter limits!)
Created attachment 346215 [details] [review] Implement search engine manager
Created attachment 346216 [details] [review] Implement search engine dialog
Created attachment 346217 [details] [review] Remove smart bookmarks
Created attachment 346218 [details] [review] Add search engines in the location entry completion
Created attachment 346219 [details] [review] profile-migrator: Replace smart bookmarks with search engines
(In reply to Michael Catanzaro from comment #123) > One last general note: it's awkward to have such a large list of search > engines in the completion entry by default, so I think we should trim down > the default list to just DuckDuckGo, Google, and Bing, which is the same > list we used before. I forgot to change this, so I'm going to fix it in the first patch, then upload all the subsequent patches again to maintain proper Bugzilla order. Annoying, but there's no other way to keep the patches ordered properly in Bugzilla. Also, I noticed that Google is redirecting our searches from https://google.com to https://www.google.com, so I'll change that too.
Created attachment 346220 [details] [review] Implement search engine manager
Created attachment 346221 [details] [review] Implement search engine dialog
Created attachment 346222 [details] [review] Remove smart bookmarks
Created attachment 346223 [details] [review] Add search engines in the location entry completion
Created attachment 346224 [details] [review] profile-migrator: Replace smart bookmarks with search engines
Review of attachment 346224 [details] [review]: ::: src/profile-migrator/ephy-profile-migrator.c @@ +947,3 @@ + + g_clear_object (&bookmarks_manager); + g_clear_object (&search_engine_manager); Didn't I mention that this should be g_object_unref(), since the variables are guaranteed to be non-null here and are never used again? More importantly, we still need to delete old smart bookmarks in the migration. I'll look at this again tomorrow, but I think it should just be one function call to delete the old bookmarks.
Freeze break request: https://mail.gnome.org/archives/release-team/2017-February/msg00023.html
Review of attachment 346220 [details] [review]: ::: embed/ephy-embed-shell.c @@ +974,3 @@ g_free (cookie_policy); + Remove this extra line @@ +1462,3 @@ + + if (!priv->search_engine_manager) + priv->search_engine_manager = EPHY_SEARCH_ENGINE_MANAGER (g_object_new (EPHY_TYPE_SEARCH_ENGINE_MANAGER, NULL)); Use ephy_search_engine_manager_new () ::: embed/ephy-embed-utils.c @@ +185,3 @@ + + if (strstr(address, buffer->str) == address) { + g_string_free (buffer, TRUE); This code is duplicated in ephy_search_engine_manager_parse_bang_search. I think we could add a method to EphySearchEngineManager to get the bang address for a given url. If not found it returns NULL. Here you would only null check it, and in ephy_search_engine_manager_parse_bang_search you would use it to resolve it. ::: lib/ephy-prefs.h @@ +151,3 @@ +#define EPHY_PREFS_SEARCH_ENGINES "search-engines" +#define EPHY_PREFS_DEFAULT_SEARCH_ENGINE "default-search-engine" Please, move these to the block of general preferences ::: lib/ephy-search-engine-manager.c @@ +78,3 @@ + while (g_variant_iter_next (iter, "(&s&s&s)", &name, &address, &bang)) { + g_hash_table_insert (manager->search_engines, + g_strdup (name), If you don't use & for name you could pass it here without another allocation. @@ +331,3 @@ + (search + buffer->len)); + g_string_free (buffer, TRUE); + break; I would return here
Review of attachment 346223 [details] [review]: ::: lib/ephy-search-engine-manager.c @@ +28,3 @@ #include "ephy-prefs.h" +static guint signal_search_engines_changed; Even if there's only one signal now, please use an array and add the signals enum. @@ +69,3 @@ +{ + EphySearchEngineManager *manager; + manager = EPHY_SEARCH_ENGINE_MANAGER (user_data); This could be the same line, otherwise I would leave an empty line between declaration block oand body. @@ +71,3 @@ + manager = EPHY_SEARCH_ENGINE_MANAGER (user_data); + + g_signal_emit (manager, signal_search_engines_changed, 0); When I saw this the first time I thought you had forgotten the aray index, then I realized there's not array. ::: src/ephy-location-controller.c @@ +307,3 @@ + name = engine_names[index]; + url = ephy_search_engine_manager_build_search_address (controller->search_engine_manager, + name, Use engine_names[index] directly here, since name is only used for this. @@ +408,3 @@ + add_completion_actions (controller, EPHY_LOCATION_ENTRY (controller->title_widget)); + + g_signal_connect (controller->search_engine_manager, "changed", Why don't we connect to the settings changed signal directly?
Review of attachment 346224 [details] [review]: I think the last 3 patches, delete smart bookmarks, add the new ones and migrate, should be squashed in a single one. ::: src/profile-migrator/ephy-profile-migrator.c @@ +943,3 @@ + ""); + + } This is called migrate_search_engines but it's only migrating the smart bookmarks. I would rename it, or migrate the search engines too. For example, these patches changed my default search engine, because that setting hasn't been migrated.
(epiphany:9218): GLib-GObject-WARNING **: invalid unclassed pointer in cast to 'GtkEntry' (epiphany:9218): Gtk-CRITICAL **: gtk_entry_get_completion: assertion 'GTK_IS_ENTRY (entry)' failed (epiphany:9218): Gtk-CRITICAL **: gtk_entry_completion_delete_action: assertion 'GTK_IS_ENTRY_COMPLETION (completion)' failed (epiphany:9218): Gtk-CRITICAL **: gtk_entry_completion_insert_action_text: assertion 'GTK_IS_ENTRY_COMPLETION (completion)' failed
@Carlos: >This code is duplicated in ephy_search_engine_manager_parse_bang_search. I think >we could add a method to EphySearchEngineManager to get the bang address for a >given url. If not found it returns NULL. Here you would only null check it, and >in ephy_search_engine_manager_parse_bang_search you would use it to resolve it. Even if I create a function that returns an address from a bang search, I still have after that to loop over the hash in order to find the related bang in ephy_search_engine_manager_parse_bang_search because the bang is needed to isolate the search string. > while (g_hash_table_iter_next (&iter, NULL, &value)) { > info = (EphySearchEngineInfo *) value; > buffer = g_string_new (info->bang); > g_string_append (buffer, " "); > if (strstr(search, buffer->str) == search) { > search_address = ephy_search_engine_manager_replace_pattern(info->address, > "%s", > (search + buffer->len)); > g_string_free (buffer, TRUE); > break; One alternative would be to create a function : EphySearchEngineInfo * ephy_search_engine_manager_get_info_from_bang_search (EphySearchEngineManager *manager, const char bang_search) We could use it in the function is_bang_search of ephy-embed-utils.c. In the same time we could remove ephy_search_engine_manager_get_bangs. But we will have to expose the EphySearchEngineInfo struct in ephy-search-engine-manager.h.
(In reply to Carlos Garcia Campos from comment #139) > Review of attachment 346223 [details] [review] [review]: > > ::: lib/ephy-search-engine-manager.c > @@ +28,3 @@ > #include "ephy-prefs.h" > > +static guint signal_search_engines_changed; > > Even if there's only one signal now, please use an array and add the signals > enum. Looks like I forgot to apply some of my own review guidance from comment #123.... I also have some outstanding guidance in comment #136. In particular, the migrator really needs to delete smart bookmarks. cedlemo, do you want to handle updating the patches again?
Of course, I already started. Could you answer my question in https://bugzilla.gnome.org/show_bug.cgi?id=776738#c142 ?
(In reply to Carlos Garcia Campos from comment #140) > Review of attachment 346224 [details] [review] [review]: > > I think the last 3 patches, delete smart bookmarks, add the new ones and > migrate, should be squashed in a single one. I prefer them all in separate patches. It makes reviewing easier.
> ::: embed/ephy-embed-utils.c > @@ +185,3 @@ > + > + if (strstr(address, buffer->str) == address) { > + g_string_free (buffer, TRUE); Also there's a missing space between strstr and (address.
The migrator should preserve the default search engine already configured by the user.
(In reply to Michael Catanzaro from comment #145) > (In reply to Carlos Garcia Campos from comment #140) > > Review of attachment 346224 [details] [review] [review] [review]: > > > > I think the last 3 patches, delete smart bookmarks, add the new ones and > > migrate, should be squashed in a single one. > > I prefer them all in separate patches. It makes reviewing easier. I'm fine if they are separated for review, but squash before land, they don't make sense separately
(In reply to cedlemo from comment #142) > @Carlos: > > > >This code is duplicated in ephy_search_engine_manager_parse_bang_search. I think > >we could add a method to EphySearchEngineManager to get the bang address for a >given url. If not found it returns NULL. Here you would only null check it, and >in ephy_search_engine_manager_parse_bang_search you would use it to resolve it. > > > Even if I create a function that returns an address from a bang search, I > still have after that to loop over the hash in order to find the related > bang in ephy_search_engine_manager_parse_bang_search because the bang is > needed to isolate the search string. > > > > while (g_hash_table_iter_next (&iter, NULL, &value)) { > > info = (EphySearchEngineInfo *) value; > > buffer = g_string_new (info->bang); > > g_string_append (buffer, " "); > > if (strstr(search, buffer->str) == search) { > > search_address = ephy_search_engine_manager_replace_pattern(info->address, > > "%s", > > (search + buffer->len)); > > g_string_free (buffer, TRUE); > > break; > > > One alternative would be to create a function : > > EphySearchEngineInfo * > ephy_search_engine_manager_get_info_from_bang_search > (EphySearchEngineManager *manager, > const char bang_search) > > We could use it in the function is_bang_search of ephy-embed-utils.c. In the > same time we could remove ephy_search_engine_manager_get_bangs. > > But we will have to expose the EphySearchEngineInfo struct in > ephy-search-engine-manager.h. I don't have time to look at this in detail again, sorry. Leave the duplicated code for now
Created attachment 346281 [details] [review] implement search engine manager * remove extra line * use ephy_search_engine_manager_new instead of the MACRO form * move search engine macros in the general preferences block in ephy-prefs.c * use "(s&s&s)" in g_variant_iter_next (iter, "(&s&s&s)", &name, &address, &bang)) * Not done (for later): code is duplicated in ephy_search_engine_manager_parse_bang_search and ephy-embed-utils.c
Created attachment 346283 [details] [review] Add search engines in the location entry completion I used an array in order to save the signal id and I fixed some styles isses.
Created attachment 346395 [details] [review] profile migrator for search engines @Michael, The migrator : * get the default search engine of the previous profile * get the smart bookmarks and convert them to search engines * delete the smart bookmarks. There is one issue with this path, which is error messages during the deletion of the smart bookmarks. I use the ephy_bookmark_manager api but I have those messages: > (ephy-profile-migrator:24774): GLib-GObject-WARNING **: instance of invalid non-instantiatable type '<invalid>' > > (ephy-profile-migrator:24774): GLib-GObject-CRITICAL **: g_signal_handlers_disconnect_matched: assertion 'G_TYPE_CHECK_INSTANCE (instance)' failed Does it talk to you ?
That's not good! Use G_DEBUG=fatal-warnings or G_DEBUG=fatal-criticals to make it crash on those errors, then you can get a backtrace to see what is causing them.
Review of attachment 346395 [details] [review]: ::: src/profile-migrator/ephy-profile-migrator.c @@ +938,3 @@ + + default_search_engine_address = g_settings_get_string (EPHY_SETTINGS_MAIN, + EPHY_PREFS_KEYWORD_SEARCH_URL); The indentation is misaligned here. @@ +945,3 @@ + default_engines[i][0], + default_search_engine_address, + ""); This isn't going to work reliably... e.g. it fails if the translation for one of the URLs changes, or if the user has change the URL to anything other than one of the three provided defaults. My recommendation is to always migrate the search engine URL reliably, and just give up on migrating the name. If the URL is not empty, then name the search engine something generic: _("Search the Web"). That should be good enough. @@ +951,3 @@ + } + + bookmarks = ephy_bookmarks_manager_get_bookmarks (bookmarks_manager); I think you're leaking it. Don't you need to free it with g_sequence_free? @@ +955,3 @@ + !g_sequence_iter_is_end (iter); + iter = g_sequence_iter_next (iter)) { + EphyBookmark *bookmark; Since you have to declare the same variable in two different for loops, I would just move it to the top of the function. @@ +967,3 @@ + ""); + id = ephy_bookmark_get_id (bookmark); + smart_bookmarks = g_list_append (smart_bookmarks, g_strdup (id)); Instead of storing bookmark IDs in the list, I would store the bookmarks themselves: smart_bookmarks = g_list_append (smart_bookmarks, bookmark) I don't think the list needs to keep a reference to the bookmarks so long as you don't free the GSequence until after you're done with thi list. @@ +972,3 @@ + + for (GList *l = smart_bookmarks; l != NULL; l = l->next) + { This brace should be on the previous line. @@ +974,3 @@ + { + EphyBookmark *bookmark; + bookmark = ephy_bookmarks_manager_get_bookmark_by_id (bookmarks_manager, Then you don't need this line @@ +980,3 @@ + } + + g_list_free_full (smart_bookmarks, g_free); And then you could use simply g_list_free here.
By the way, I did get the freeze exception approved, so as soon as you have the profile migrator working without those critical warnings we can push this. At long last. :)
Created attachment 346452 [details] [review] profile migrator for search engines Michael, I followed your instructions, I just transform the default search address into a search engine with a default name. I found out that I needed to add a ref count to the bookmark objects before to delete them. It works but ... I am not able to say why and that could be problematic. * migration of default search engine : OK * migration smart bookmarks to search engines : OK * deleteion of smart bookmarks : OK * No warnings or error messages.
Review of attachment 346452 [details] [review]: ::: src/profile-migrator/ephy-profile-migrator.c @@ +923,3 @@ + const char *address; + const char *title; + const char *default_search_engine_address; char * default_search_engine_address; @@ +929,3 @@ + search_engine_manager = ephy_search_engine_manager_new (); + + default_search_engine_address = g_settings_get_string (EPHY_SETTINGS_MAIN, This is leaked. @@ +932,3 @@ + EPHY_PREFS_KEYWORD_SEARCH_URL); + if (default_search_engine_address != NULL) { + No blank line here. @@ +933,3 @@ + if (default_search_engine_address != NULL) { + + ephy_search_engine_manager_modify_engine (search_engine_manager, Why are you using _modify_engine() instead of, say, _add_engine()? @@ +956,3 @@ + address, + ""); + g_object_ref (bookmark); The EphyBookmark objects returned by ephy_bookmarks_manager_get_bookmarks are reffed by the GSequence it returns. It should be harmless to add an additional ref, but if so, you have to remove the ref later... @@ +965,3 @@ + (EphyBookmark *)(l->data)); + + g_list_free (smart_bookmarks); ...so here you would need to use g_list_free_full (smart_bookmarks, g_object_unref). You can't just ref objects and never unref them in order to work around a refcounting bug somewhere else, so if this creates a double-free, then we need to investigate to find out what is going wrong.
Created attachment 346472 [details] [review] profile migrator for search engines Everything works. The fact that I needed to increase the ref count of the bookmark in order to delete it then unref it is quite unexpected for me.
Yeah, I don't understand that either. Something's up there. But there's nothing wrong with doing so. You ref it once, and unref it once, and that's perfectly fine, even if it's odd that you have to ref it at all. Anyway, this looks good to me. Thanks cedlemo! Time to commit. This bug report has turned into a long mess, so if anything breaks due to this, please fix a new bug!
Review of attachment 346472 [details] [review]: ::: src/profile-migrator/ephy-profile-migrator.c @@ +918,3 @@ + EphyBookmarksManager *bookmarks_manager; + EphySearchEngineManager *search_engine_manager; + GSequence *bookmarks; You're still leaking this GSequence. :( Freeing it will cause all the EphyBookmarks objects to be unreffed once more, so I bet something is still wrong here.
(In reply to cedlemo from comment #152) > > (ephy-profile-migrator:24774): GLib-GObject-WARNING **: instance of invalid non-instantiatable type '<invalid>' > > > > (ephy-profile-migrator:24774): GLib-GObject-CRITICAL **: g_signal_handlers_disconnect_matched: assertion 'G_TYPE_CHECK_INSTANCE (instance)' failed It happens once per deleted smart bookmark:
+ Trace 237167
ephy_bookmarks_manager_remove_bookmark() is unreffing the bookmark too early.
> You're still leaking this GSequence No please looks at the ephy_bookmarks_manager_get_bookmarks, it returns directly the internal GSequence. Each time it is used, it is not freed.
(In reply to Michael Catanzaro from comment #161) > (In reply to cedlemo from comment #152) > > > (ephy-profile-migrator:24774): GLib-GObject-WARNING **: instance of invalid non-instantiatable type '<invalid>' > > > > > > (ephy-profile-migrator:24774): GLib-GObject-CRITICAL **: g_signal_handlers_disconnect_matched: assertion 'G_TYPE_CHECK_INSTANCE (instance)' failed > > It happens once per deleted smart bookmark: Do you use the last patch ?
OK good catch, I was wrong! Haven't pushed yet, fortunately, because the testsuite is failing. :) You can try running it with 'make check' or 'jhbuild buildone -n --check epiphany' Anyway, I've fixed that issue, cleaned up the refcounting in the profile migrator, deprecated the keyword-search-url GSettings key since it's not used anymore, and made the new key translatable.
Also the order and names of the keyword/url parameters in the ephy-search-engine-manager.h header file is wrong. It should match the source file.
OK I don't have more time for this today, so I'll post what I have now. I fixed everything I mentioned above except for ephy-web-view-test. I tried to fix it, but it's still failing; apparently ephy_search_engine_manager_modify_engine() is not working properly. Please check it.
Created attachment 346503 [details] [review] Implement search engine manager
Created attachment 346504 [details] [review] Implement search engine dialog
Created attachment 346505 [details] [review] Replace smart bookmarks feature with search engine manager
Created attachment 346506 [details] [review] bookmarks-manager: Fix use-after-free in _remove_bookmark()
@Michael, it seems that the search_engine_manager instance in the web-view-test is modified with the *_add_engine and the *_modify_engine. But the instance in ephy-embed-utils.c in the function ephy_embed_util_autosearch_address is not able to see the modifications of the search engine address. I continue to search for a solution.
Ah, oops! Good catch. The solution is easy, then: just create the EphyShell instance in the web view test (see my modification to the embed utils test in your first patch for an example), and then get the search engine manager from there instead of creating a standalone one.
Now the normalize_or_autosearch pass but the new test does not : > /embed/ephy-web-view/non_search_regex: OK > /embed/ephy-web-view/normalize_or_autosearch: OK > /embed/ephy-web-view/load_url: > (/home/cedlemo/.cache/jhbuild/build/epiphany/tests/.libs/lt-test-ephy-web-view:18329): > GLib-CRITICAL **: g_hash_table_size: assertion 'hash_table != NULL' failed Should I work on it too ? (This does not seem to be related to the search engines)
Created attachment 346604 [details] [review] implement search engine manager With this patch the test embed/ephy-web-view/normalize_or_autosearch passes but still block on embed/ephy-web-view/load_url like I said previously. I have one question Michael, why only the instances of SearchEngineManager that are obtained from the EphyShell can share the g_settings configuration/modifications. For example when it did not work, in the tests we get an instance with ephy_search_engine_manager_new which read the g_settings, we do the modications then in ephy-embed-utils we use with ephy_shell_get_search_engine_manager which in fact just call ephy_search_engine_manager_new and read the g_settings. So why this last one is not able to read the last modifications ?
(In reply to cedlemo from comment #174) > Created attachment 346604 [details] [review] [review] > implement search engine manager > > With this patch the test embed/ephy-web-view/normalize_or_autosearch passes > but still block on embed/ephy-web-view/load_url like I said previously. > > I have one question Michael, why only the instances of SearchEngineManager > that are obtained from the EphyShell can share the g_settings > configuration/modifications. > > For example when it did not work, in the tests we get an instance with > ephy_search_engine_manager_new which read the g_settings, we do the > modications then in ephy-embed-utils we use with > ephy_shell_get_search_engine_manager which in fact just call > ephy_search_engine_manager_new and read the g_settings. So why this last one > is not able to read the last modifications ? Good question. I think I know the answer: I actually warned you about it in comment #112! It's bad API design. We should fix it by adding a useless read of the search-engines setting after connecting to GSettings::changed::search-engines in ephy_search_engine_manager_init().
(In reply to cedlemo from comment #173) > Now the normalize_or_autosearch pass but the new test does not : > > > /embed/ephy-web-view/non_search_regex: OK > > /embed/ephy-web-view/normalize_or_autosearch: OK > > /embed/ephy-web-view/load_url: > > (/home/cedlemo/.cache/jhbuild/build/epiphany/tests/.libs/lt-test-ephy-web-view:18329): > > GLib-CRITICAL **: g_hash_table_size: assertion 'hash_table != NULL' failed > > > Should I work on it too ? (This does not seem to be related to the search > engines) It occurs in ephy_search_engine_manager_get_bangs(). Can you investigate?
(In reply to Michael Catanzaro from comment #175) > Good question. I think I know the answer: I actually warned you about it in > comment #112! It's bad API design. We should fix it by adding a useless read > of the search-engines setting after connecting to > GSettings::changed::search-engines in ephy_search_engine_manager_init(). I see you already ready the setting in ephy_search_engine_manager_init(), so it should be sufficient to just reorder that code to fix it. Like this: /* Warning: has to be ABOVE the g_settings_get() or it doesn't work, * as per the GSettings documentation. */ g_signal_connect (EPHY_SETTINGS_MAIN, "changed::" EPHY_PREFS_SEARCH_ENGINES, G_CALLBACK (search_engines_changed_cb), manager); g_settings_get (EPHY_SETTINGS_MAIN, EPHY_PREFS_SEARCH_ENGINES, "a(sss)", &iter); Otherwise, the changed signal won't work until something else reads the setting.
Created attachment 346658 [details] [review] implement search engine manager It tooks me one day to find out that the search engine manager should not be unref in the normalize_or_autosearch tests. We did it because at the begining we used ephy_search_engine_manager_new in order to create an instance then we used ephy_shell_get_search_engine_manager that returns a pointer to a search engine we don't have to clear/unref. One full day ... for one instruction.
Do you know how to use gdb to debug the tests? If you have coredumpctl installed and functional, then it should be easy to run: $ make check $ coredumpctl gdb And that should open gdb to the crashing line. It should have taken you right to the bad unref. Sorry you wasted a day on it. :(
Created attachment 346671 [details] [review] implement search engine manager
Created attachment 346672 [details] [review] Implement search engine dialog
Created attachment 346673 [details] [review] Replace smart bookmarks feature with search engine manager
Created attachment 346674 [details] [review] bookmarks-manager: Fix use-after-free in _remove_bookmark()
> Do you know how to use gdb to debug the tests? I have a lot of to learn. > If you have coredumpctl installed and functional, then it should be easy to run: > >$ make check >$ coredumpctl gdb Thanks, I will have a look at this. > Sorry you wasted a day on it. :( It is ok, I am not angry, I am just making fun of myself :). What comes next Michael ?
(In reply to cedlemo from comment #184) > What comes next Michael ? Pushing it. :) Let's do that now. Thanks for your persistence in updating your patches through several rounds of code review!
Attachment 346671 [details] pushed as 1d7ba9c - implement search engine manager Attachment 346672 [details] pushed as e57ce8d - Implement search engine dialog Attachment 346673 [details] pushed as cdf7fc5 - Replace smart bookmarks feature with search engine manager Attachment 346674 [details] pushed as 2c9c7e7 - bookmarks-manager: Fix use-after-free in _remove_bookmark()
The following fixes have been pushed: f30bf73 Miscellaneous code style adjustments f6325a2 embed-utils: Fix leak is is_bang_search()
Created attachment 346676 [details] [review] Miscellaneous code style adjustments
Created attachment 346677 [details] [review] embed-utils: Fix leak is is_bang_search()