GNOME Bugzilla – Bug 726462
Improve keyboard navigation for Searchbar
Last modified: 2015-04-09 09:20:17 UTC
Inspired from Bug 725622 There is no provision to exit from Search Preferences using the Escape key. Also, it is currently not possible to open the Search Preferences using the keyboard. In addition to this, I think it would be nice if the focus returns to the Search Entry after exiting from the Search Preferences by clicking on the dropdown button.
Created attachment 272076 [details] [review] Improve keyboard navigation for Searchbar I have made the enhancements to open Search preferences from Searchbar using Tab and Return, to exit from search preferences using Escape and returning focus to search entry after exiting from search preferences.
Review of attachment 272076 [details] [review]: ::: src/photos-overview-searchbar.c @@ +306,3 @@ + g_signal_connect_swapped (priv->dropdown_button, + "key-press-event", Do we really need 'key-press-event'? Can't we just use 'clicked'? @@ +313,3 @@ + "key-press-event", + G_CALLBACK (photos_overview_searchbar_search_entry_key_pressed_cb), + self); Move this up where we connect to the other signals in priv->search_entry.
Review of attachment 272076 [details] [review]: ::: src/photos-overview-searchbar.c @@ +222,3 @@ + +static gboolean +photos_overview_searchbar_search_entry_key_pressed_cb (PhotosOverviewSearchbar *self, GdkEventKey *event) We generally don't use the _cb suffix for callbacks in the rest of the code. @@ +238,3 @@ + +static gboolean +photos_overview_searchbar_dropdown_key_pressed_cb (PhotosOverviewSearchbar *self, GdkEventKey *event) Same here.
Created attachment 272082 [details] [review] Modified the patch. Thanks for the review. Reply to comment 2: >Do we really need 'key-press-event'? Can't we just use 'clicked'? I am using key-press-event to enable the Escape key to exit from search preferences.
We should just do something like these: commit 3e4b0866ad8ddf66aa931e9a094c80e588570f70 Author: Cosimo Cecchi <cosimoc@gnome.org> Date: Wed Jul 30 15:10:59 2014 +0200 searchbar: port dropdown to GMenuModel Now that we have GActions for search options, tie it all together and use them to build the popover contents. We have to steal the child of a GtkPopover to get the UI, since GTK doesn't expose a public API for that yet. https://bugzilla.gnome.org/show_bug.cgi?id=725622 commit a657086cb21cb57fb31983630bdab05543c9299f Author: Cosimo Cecchi <cosimoc@gnome.org> Date: Wed Jul 30 15:08:15 2014 +0200 application: add GActions for search options https://bugzilla.gnome.org/show_bug.cgi?id=725622 commit 8953a000d2ac06d40c3f5a042012f6b7712acbd8 Author: Cosimo Cecchi <cosimoc@gnome.org> Date: Wed Jul 30 15:07:31 2014 +0200 manager: add an optional action ID We're going to use this to build GActions/GMenuModles for the various search options. https://bugzilla.gnome.org/show_bug.cgi?id=725622 commit a69ac5ea462cf6e9173df1efb4d0d577a0c4c306 Author: Cosimo Cecchi <cosimoc@gnome.org> Date: Wed Jul 30 15:04:46 2014 +0200 search: add a define for the stock search types https://bugzilla.gnome.org/show_bug.cgi?id=725622 commit 434a1d0a79c472b680599398c5c53b043a456fbb Author: Cosimo Cecchi <cosimoc@gnome.org> Date: Wed Jul 30 15:03:48 2014 +0200 search: use a consistent name for the search source enum We prefix the other enum types with "Search", so do it here too for consistency. https://bugzilla.gnome.org/show_bug.cgi?id=725622 commit 06b7534ec305c1eaa913a331b5a2e419268ae905 Author: Marta Milakovic <marta.milakovic@gmail.com> Date: Tue Apr 15 23:32:16 2014 +0200 Ports Dropdown of the Overview Searchbar to GtkPopover GtkPopover allows hiding interface elements out of the way. The problem of click handling for dropdown menu is solved by using GtkPopover. https://bugzilla.gnome.org/show_bug.cgi?id=725622
Created attachment 298784 [details] [review] Ports Dropdown of the Overview Searchbar to GtkPopover GtkPopover allows hiding interface elements out of the way. The problem of click handling for dropdown menu is solved by using GtkPopover.
This patch introduces a bug, steps to reproduce it: - Open gnome-photos - Click on search button - Press Alt+F4
(In reply to Alessandro Bono from comment #7) > This patch introduces a bug, steps to reproduce it: > - Open gnome-photos > - Click on search button > - Press Alt+F4 Double click on dropdown menu. It exits with some error.
Created attachment 298915 [details] [review] Ports Dropdown of the Overview Searchbar to GtkPopover GtkPopover allows hiding interface elements out of the way. The problem of click handling for dropdown menu is solved by using GtkPopover.
(In reply to Kunaal Jain from comment #8) > (In reply to Alessandro Bono from comment #7) > > This patch introduces a bug, steps to reproduce it: > > - Open gnome-photos > > - Click on search button > > - Press Alt+F4 > > Double click on dropdown menu. It exits with some error. Did you click on "Source", "Type" or "Match" string? If so I don't think is related to this patch, it exits with errors even on master. Maybe will be fixed with the GMenu port.
Created attachment 298933 [details] [review] manager: add an optional action ID We're going to use this to build GActions/GMenuModles for the various search options.
Created attachment 298951 [details] [review] manager: add an optional action ID We're going to use this to build GActions/GMenuModles for the various search options.
Review of attachment 298915 [details] [review]: Thanks for the patch, Alessandro. It looks good apart from a few minor issues. ::: src/photos-dropdown.c @@ +47,1 @@ Extra newline. Two is enough. @@ +98,3 @@ gtk_container_add (GTK_CONTAINER (priv->grid), priv->match_view); + Extra newline. One is enough. ::: src/photos-overview-searchbar.c @@ +210,2 @@ active = gtk_toggle_button_get_active (GTK_TOGGLE_BUTTON (priv->dropdown_button)); + if(active) For if conditions, you can use the function directly instead of using a variable. @@ +235,3 @@ photos_searchbar_set_search_entry (PHOTOS_SEARCHBAR (self), GTK_WIDGET (priv->search_entry)); + priv->dropdown = photos_dropdown_new (GTK_WIDGET (priv->search_entry)); The dropdown should be relative to the priv->dropdown_button, not priv->search_entry. This is a mistake in the original patch that was fixed in 3e4b0866 @@ +258,1 @@ G_CALLBACK (photos_overview_searchbar_item_activated), Since the name of the signal has changed, we should also rename the callback. @@ +292,2 @@ GTK_WIDGET_CLASS (photos_overview_searchbar_parent_class)->destroy (widget); } Since we are only chaining up to the parent's implementation, we can completely remove this method.
Created attachment 298956 [details] [review] Port the Dropdown widget to GtkPopover I fixed the above issues, tweaked the subject of the commit message and committed it.
Review of attachment 298951 [details] [review]: Thanks for the patch, Alessandro. The commit message has a typo. "GMenuModels" not "GMenuModles". Also, the subject should be "base-manager: Add an ..." to match the name of the file. ::: src/photos-base-manager.c @@ +37,3 @@ GObject *active_object; gchar *title; + gchar *action_id; Put it above 'title'. @@ +44,3 @@ PROP_0, + PROP_TITLE, + PROP_ACTION_ID Ditto. @@ +167,3 @@ break; + case PROP_ACTION_ID: + self->priv->action_id = value; 'value' is a GValue. We should use g_value_dup_string to extract the string from it, and g_free self->priv->action_id in finalize. @@ +213,3 @@ + PROP_ACTION_ID, + g_param_spec_string ("action_id", + "Action_ID", "Action ID" @@ +289,3 @@ +{ + return self->priv->action_id; +} The return type should be const gchar *. In GNOME, the convention with returning strings is that if the caller is supposed to free it, then it is a gchar *, otherwise const gchar *. ::: src/photos-base-manager.h @@ +86,3 @@ void photos_base_manager_clear (PhotosBaseManager *self); +gchar * photos_base_manager_get_action_id (PhotosBaseManager *self); The * should be with the function name. ::: src/photos-search-match-manager.c @@ +203,3 @@ return g_object_new (PHOTOS_TYPE_SEARCH_MATCH_MANAGER, "search-controller", srch_cntrlr, + "title", _("Match"), "action_id", "search-match", It should be "action-id". We use underscores in JavaScript because it can not handle the dashes. In fact, the canonical property name can't have anything but letters, numbers or dashes. So, if you use an underscore GObject will replace it with a dash. See: https://developer.gnome.org/gobject/stable/gobject-GParamSpec.html#gobject-GParamSpec.description Also, put it on a separate line like the other properties. ::: src/photos-search-type-manager.c @@ +152,3 @@ * Favorites and Photos. */ + return g_object_new (PHOTOS_TYPE_SEARCH_TYPE_MANAGER, "title", C_("Search Filter", "Type"), "action_id", "search-type", NULL); Can you please break this statement into multiple lines? It is getting too long. We try not to exceed 120 characters per line.
Review of attachment 272082 [details] [review]: The correct fix would be to mimic gnome-documents.
(In reply to Alessandro Bono from comment #7) > This patch introduces a bug, steps to reproduce it: > - Open gnome-photos > - Click on search button > - Press Alt+F4 Which bug? Did it crash? Didn't we fix it by removing the destroy vfunc?
(In reply to Kunaal Jain from comment #8) > (In reply to Alessandro Bono from comment #7) > > This patch introduces a bug, steps to reproduce it: > > - Open gnome-photos > > - Click on search button > > - Press Alt+F4 > > Double click on dropdown menu. It exits with some error. What error?
Created attachment 298987 [details] [review] base-manager: Add an optional action ID We're going to use this to build GActions/GMenuModels for the various search options.
Created attachment 298988 [details] [review] base-manager: Add an optional action ID We're going to use this to build GActions/GMenuModels for the various search options.
(In reply to Debarshi Ray from comment #17) > (In reply to Alessandro Bono from comment #7) > > This patch introduces a bug, steps to reproduce it: > > - Open gnome-photos > > - Click on search button > > - Press Alt+F4 > > Which bug? Did it crash? Didn't we fix it by removing the destroy vfunc? The comment was related on patch 298784, in which the destroy vfunc contained gtk_widget_hide();. In patch 298915 I removed gtk_widget_hide(); so it didn't crash anymore. When you commited, you have removed directly the destroy vfunc so the bug should be fixed now.
Review of attachment 298988 [details] [review]: Thanks, Alessandro. A few minor cosmetic things. ::: src/photos-base-manager.c @@ +150,3 @@ PhotosBaseManager *self = PHOTOS_BASE_MANAGER (object); + g_free (self->priv->action_id); You can define a 'PhotosBaseManagerPrivate *priv = self->priv' pointer at the top if you need to use the priv more than once. @@ +166,3 @@ case PROP_TITLE: self->priv->title = g_value_dup_string (value); break; A new line between the two properties would be good. @@ +168,3 @@ break; + case PROP_ACTION_ID: + self->priv->action_id = g_value_dup_string(value); Please add a white space between the function name and the opening parenthesis. @@ +210,3 @@ "The name of this manager", NULL, G_PARAM_CONSTRUCT_ONLY | G_PARAM_WRITABLE)); A new line between the two properties would be good.
Created attachment 299076 [details] [review] base-manager: Add an optional action ID Fixed the nitpicks and committed.
Created attachment 299240 [details] [review] photos-application: Add GActions for search options
Review of attachment 299240 [details] [review]: Mostly looks good. Some minor comments. You could use coloured Git output to detect trailing whitespaces: $ git config --global color.diff auto ::: src/photos-application.c @@ +73,3 @@ GSimpleAction *search_action; + GSimpleAction *search_match_action; + GSimpleAction *search_source_action; Trailing white space alert. @@ +705,3 @@ g_simple_action_set_enabled (priv->set_bg_action, enable); g_simple_action_set_enabled (priv->set_ss_action, enable); + Ditto. @@ +712,3 @@ + g_simple_action_set_enabled (priv->search_match_action, enable); + g_simple_action_set_enabled (priv->search_source_action, enable); + g_simple_action_set_enabled (priv->search_type_action, enable); How about moving it above the sel_all_action / sel_none_action block? That way similar things will remain in closer proximity. @@ +940,3 @@ + priv->search_match_action = g_simple_action_new_stateful ("search-match", + g_variant_type_new ("s"), + g_variant_new(PHOTOS_SEARCH_MATCH_STOCK_ALL)); Please add a white space between the function name and opening parenthesis. @@ +943,3 @@ + g_action_map_add_action (G_ACTION_MAP (self), G_ACTION (priv->search_match_action)); + + priv->search_source_action = g_simple_action_new_stateful ("search-source", We need to add "search-source" as action-id to PhotosSourceManager in a separate commit. It was missing in the original commit because it got mixed up in a different commit. @@ +945,3 @@ + priv->search_source_action = g_simple_action_new_stateful ("search-source", + g_variant_type_new ("s"), + g_variant_new(PHOTOS_SOURCE_STOCK_ALL)); Please add a white space between the function name and opening parenthesis. @@ +947,3 @@ + g_variant_new(PHOTOS_SOURCE_STOCK_ALL)); + g_action_map_add_action (G_ACTION_MAP (self), G_ACTION (priv->search_source_action)); + Trailing white space alert. @@ +950,3 @@ + priv->search_type_action = g_simple_action_new_stateful ("search-type", + g_variant_type_new ("s"), + g_variant_new(PHOTOS_SEARCH_TYPE_STOCK_ALL)); Ditto. @@ +952,3 @@ + g_variant_new(PHOTOS_SEARCH_TYPE_STOCK_ALL)); + g_action_map_add_action (G_ACTION_MAP (self), G_ACTION (priv->search_type_action)); + Ditto. @@ +1048,3 @@ g_clear_object (&priv->search_action); + g_clear_object (&priv->search_match_action); + g_clear_object (&priv->search_source_action); Ditto.
Review of attachment 299240 [details] [review]: The commit message subject should be "application: Add GActions ..." We generally don't use the 'photos' prefix because it is common.
Created attachment 299276 [details] [review] application: Add GActions for search options
Created attachment 299295 [details] [review] source-manager: Add "search-source" as action-id
Review of attachment 299295 [details] [review]: ::: src/photos-source-manager.c @@ +175,3 @@ + "title",_("Sources"), + "action-id", "search-source", + NULL); This could stay on the same line. We usually allow upto 120 characters per line. This helps to accommodate longer class and method names due to the namespace prefixes, and terminals on modern systems can easily handle more than 80 characters. See the README file. Also, please put "action-id" before "title" so that the properties are in alphabetical order.
Review of attachment 299276 [details] [review]: ::: src/photos-application.c @@ +940,3 @@ + priv->search_match_action = g_simple_action_new_stateful ("search-match", + g_variant_type_new ("s"), You are leaking some memory here. g_variant_type_new returns a pointer to newly created GVariantType, and when you pass it to g_simple_action_new_stateful, a new copy is created for use within the GSimpleAction. The original pointer returned by g_variant_type_new should, therefore, be freed. I would suggest using a separate 'GVariantType *parameter_type' variable, and using g_variant_type_free on it once the action has been created. @@ +941,3 @@ + priv->search_match_action = g_simple_action_new_stateful ("search-match", + g_variant_type_new ("s"), + g_variant_new (PHOTOS_SEARCH_MATCH_STOCK_ALL)); The first parameter to g_variant_new should be a format string like "b" or "s". In this case you want "s". Otherwise you get CRITICALs like these: 'all' is not a valid GVariant format string I would also prefer to create the variant in a separate line, as is done elsewhere in the function.
Created attachment 299701 [details] [review] source-manager: Add "search-source" as action-id
Review of attachment 299701 [details] [review]: Perfect. Thanks, Alessandro.
Created attachment 299703 [details] [review] application: Add GActions for search options
Created attachment 299761 [details] [review] dropdown: Delete no more needed function prototypes Since ad0bab7c194f21b09b4d711a0aa2d9e3a68997b2 they don't exist
Created attachment 299762 [details] [review] base-model: port dropdown to GMenuModel Now that we have GActions for search options, tie it all together and use them to build the popover contents. We have to steal the child of a GtkPopover to get the UI, since GTK doesn't expose a public API for that yet.
Created attachment 299763 [details] [review] base-model: port dropdown to GMenuModel Now that we have GActions for search options, tie it all together and use them to build the popover contents. We have to steal the child of a GtkPopover to get the UI, since GTK doesn't expose a public API for that yet.
Created attachment 299770 [details] [review] base-model: port dropdown to GMenuModel Now that we have GActions for search options, tie it all together and use them to build the popover contents. We have to steal the child of a GtkPopover to get the UI, since GTK doesn't expose a public API for that yet.
Review of attachment 299761 [details] [review]: Thanks for noticing this.
Review of attachment 299703 [details] [review]: Perfect, thanks.
Review of attachment 299770 [details] [review]: Thanks for another nice patch, Alessandro. ::: src/photos-base-model.c @@ +36,3 @@ PhotosBaseManager *mngr; + GMenu *model; + gulong action_state_changed_handler; There is no need to retain the handler. See below. @@ +53,3 @@ +photos_base_model_action_state_changed (PhotosBaseModel *self, + const gchar *action_name, + GVariant *value) These can be on one line. You are allowed close to 120 characters on a line, because modern monitors / terminals can accommodate much more than 80 characters which is a big help when it comes to GObject code. @@ +55,3 @@ + GVariant *value) +{ + const gchar *item_id = g_variant_get_string (value, NULL); Please put the function call in a separate line. @@ +63,3 @@ +photos_base_model_active_changed (PhotosBaseModel *self, + GObject *active_object, + PhotosBaseManager *mngr) No need for the third parameter, because we already have priv->mngr. In C it is acceptable for the function definition to ignore a few parameters (from the end) that the caller has passed to it. All the parameter names can then go on the same line. @@ +68,3 @@ + const gchar *action_id; + const gchar *active_object_id; + GVariant *value; Nitpick. Move it below 'application', and we usually call it 'state'. @@ +72,3 @@ + application = g_application_get_default (); + action_id = photos_base_manager_get_action_id (mngr); + g_object_get (G_OBJECT (active_object), "id", &active_object_id, NULL); g_object_get returns a new copy of the string, so we need to free it. Better to use photos_filterable_get_id. All objects that are kept in any BaseManager implement the PhotosFilterable interface. Also, we usually name this variable just 'id', not 'active_object_id'. @@ +92,1 @@ + g_menu_remove_all (G_MENU (priv->model)); priv->model is already a pointer to a GMenu. No need to cast it using G_MENU. @@ +122,3 @@ photos_base_model_constructed (GObject *object) { + GApplication *application = g_application_get_default (); Nitpick. Just 'app' is enough, and please put the function call on a separate line. @@ +170,1 @@ + g_signal_handler_disconnect (application, priv->action_state_changed_handler); You don't need to manually disconnect it when the BaseModel is destroyed. g_signal_connect_object will take care of it for you. @@ +191,3 @@ + break; + } +} It is better to define a public method to get the model (eg., photos_base_model_get_model) instead of using a property. We use properties mostly to pass values during construction, and in a few exceptional cases. Properties are generally more useful in libraries, where every getter and setter is backed by a property, because it lets you introspect a type. ::: src/photos-base-model.h @@ +60,3 @@ struct _PhotosBaseModel { + GObject parent_instance; We no longer need the gtk+ header for this class. It can be replaced by gio/gio.h. ::: src/photos-dropdown.c @@ +56,3 @@ + GtkWidget *w; + PhotosBaseModel *model = photos_base_model_new (mngr); + GtkWidget *popover = gtk_popover_new (NULL); Please put the function calls in a separate line. @@ +59,3 @@ + + // HACK: see https://bugzilla.gnome.org/show_bug.cgi?id=733977 + g_object_get (G_OBJECT (model), "model", &menu_model, NULL); We need to unref the menu_model when using g_object_get. However, you won't have to do that if you use a public getter method. @@ +109,3 @@ + photos_dropdown_add_manager(self, priv->src_mngr); + photos_dropdown_add_manager(self, priv->srch_typ_mngr); + photos_dropdown_add_manager(self, priv->srch_mtch_mngr); Please add a space before the opening parenthesis. @@ +133,3 @@ + "relative-to", relative_to, + "position", GTK_POS_BOTTOM, + NULL); Can be on one line. ::: src/photos-overview-searchbar.c @@ +209,2 @@ if (gtk_toggle_button_get_active (GTK_TOGGLE_BUTTON (priv->dropdown_button))) + gtk_widget_show_all (GTK_WIDGET(priv->dropdown)); priv->dropdown is already a pointer to a GtkWidget. No need to cast it.
Created attachment 300504 [details] [review] base-model: port dropdown to GMenuModel Now that we have GActions for search options, tie it all together and use them to build the popover contents. We have to steal the child of a GtkPopover to get the UI, since GTK doesn't expose a public API for that yet.
Review of attachment 300504 [details] [review]: ::: src/photos-base-model.c @@ +52,3 @@ +photos_base_model_action_state_changed (PhotosBaseModel *self, const gchar *action_name, GVariant *value) +{ + const gchar *item_id; Add a new line after the definition. @@ +63,3 @@ + GApplication *app; + GVariant *state; + PhotosBaseModelPrivate *priv = self->priv; We generally keep the priv at the top followed by the rest. @@ +177,3 @@ + return self->priv->model; +} + Public functions go towards the end of the file after the *_new function. ::: src/photos-base-model.h @@ +69,3 @@ }; +const GMenu *photos_base_model_get_model (PhotosBaseModel *self); This should be at the end, and the columns are not misaligned by one character. ::: src/photos-dropdown.c @@ +56,3 @@ + GtkWidget *w; + PhotosBaseModel *model; + GtkWidget *popover; Nitpick. Put it with the other GtkWidget * above. @@ +58,3 @@ + GtkWidget *popover; + + model = photos_base_model_new (mngr); We have a problem with the lifetime of PhotosBaseModel here. Although we won't use the instance after this function, we can't unref it either, because then we will lose all the signal connections inside the class. One solution would be to have a priv->models member in PhotosDropdown, and add each BaseModel to it, and then drop the reference during destruction in dispose. @@ +66,3 @@ + w = g_object_ref (gtk_bin_get_child (GTK_BIN (popover))); + gtk_container_remove (GTK_CONTAINER (popover), w); + gtk_container_add (GTK_CONTAINER (self->priv->grid), w); You need to unref 'w' after you have added it to its new parent.
Created attachment 300564 [details] [review] base-model: port dropdown to GMenuModel I fixed everything except the reference counting issues.
Created attachment 300695 [details] [review] base-model: port dropdown to GMenu Model Now that we have GActions for search options, tie it all together and use them to build the popover contents. We have to steal the child of a GtkPopover to get the UI, since GTK doesn't expose a public API for that yet.
Created attachment 300697 [details] [review] application: we should compare with mode. Fix a bug introduced in commit a46451e866f4b0551a9849a5568c84a2e0296024
Review of attachment 300695 [details] [review]: ::: src/photos-dropdown.c @@ +38,3 @@ struct _PhotosDropdownPrivate { + GArray *models; I think it will be more idiomatic to use a GList, because that is what we widely use to keep simple lists. There is nothing wrong in using a GArray, but we don't need their biggest selling point - random access in O(1) time. At the same time, we are incurring the cost of having to grow the array as we add elements to it, unless we are going to reserve some initial size. We could do that, but then we will have to adjust it as we change the number of managers. I am sure you can argue against GList in the same way too, but unless someone profiles [1] the run-time behaviour, I would lean towards consistency and not introduce another data structure. [1] I doubt that profiling will reveal anything because this part of the code is not performance critical. @@ +61,3 @@ + + model = photos_base_model_new (mngr); + g_array_append_val (self->priv->models, model); I prefer to use an explicit g_object_ref (model) when passing the pointer to the data structure, and dropping the local reference at the end. The explicit referencing makes it very obvious that the data structure has its own ref, and it becomes harder to introduce bugs when refactoring the code. When we move to the GLib's new g_autoptr, every pointer will be automatically freed / unreffed when leaving the scope, and we won't need to drop the local reference ourselves. @@ +86,3 @@ g_clear_object (&priv->srch_typ_mngr); g_clear_object (&priv->src_mngr); + g_array_free (priv->models, TRUE); The BaseModel instances, whose pointers are in the array, are not getting unreferenced and hence being leaked. You could do that by passing g_object_unref to g_array_set_clear_func. If you use a GList, then: g_list_free_full (priv->models, g_object_unref); priv->models = NULL;
Review of attachment 300697 [details] [review]: Thanks Alessandro. One nitpick about the commit message. The subject should not end with a full stop, and it is better to start it with a bare infinitive verb. eg., compare. That is what git itself does by default. eg., when you do git revert. ::: src/photos-application.c @@ +681,3 @@ + || mode == PHOTOS_WINDOW_MODE_FAVORITES + || mode == PHOTOS_WINDOW_MODE_OVERVIEW + || mode == PHOTOS_WINDOW_MODE_SEARCH); Good catch! I wonder how I missed that during the review.
Created attachment 300847 [details] [review] base-model: port dropdown to GMenu Model Now that we have GActions for search options, tie it all together and use them to build the popover contents. We have to steal the child of a GtkPopover to get the UI, since GTK doesn't expose a public API for that yet.
Review of attachment 300847 [details] [review]: Almost there. ::: src/photos-base-model.c @@ +94,3 @@ + + section = g_menu_new (); + g_menu_append_section (priv->model, title, G_MENU_MODEL (section)); section needs to be unreffed. ::: src/photos-base-model.h @@ +73,3 @@ +PhotosBaseModel *photos_base_model_new (PhotosBaseManager *mngr); + +const GMenu *photos_base_model_get_model (PhotosBaseModel *self); We generally don't use the const modifier for return types unless it is a C style string. ::: src/photos-dropdown.c @@ +63,3 @@ + model = photos_base_model_new (mngr); + g_object_ref (model); + priv->models = g_list_append (priv->models, model); g_list_prepend is usually better because it is O(1), whereas append traverses the whole list and is O(n). And in this case we don't care about the order either. @@ +64,3 @@ + g_object_ref (model); + priv->models = g_list_append (priv->models, model); + g_object_unref (model); This should be at the end of the function, because we are using model below. @@ +119,3 @@ gtk_container_add (GTK_CONTAINER (self), priv->grid); + priv->models = NULL; GObjects are always 0 initialized when their structs are allocated. So, we don't need to initialize things to 0, FALSE or NULL.
Created attachment 301190 [details] [review] base-model, dropdown: Port to GMenuModel Fixed the last batch of issues and pushed.
Thanks for all your hard work, Alessandro.