GNOME Bugzilla – Bug 753673
view: handle view menu
Last modified: 2015-08-16 17:35:29 UTC
NautilusToolbar handles the view menu, requiring direct access to the underlying view inside the window slot. Since we're wiping out every access to the underlying view, we shouldn't access it from NautilusToolbar. To fix that, makes the view handle the view widget. Since we're making NautilusWindowSlot a wrapper, add the necessary properties for it to expose view data without exposing the view itself.
Created attachment 309351 [details] [review] view: handle view menu
Review of attachment 309351 [details] [review]: So far I like how all of this is going...much better isolation, understandable, and probability to catch bugs and understand them at first sight! ::: src/nautilus-toolbar.c @@ +862,3 @@ + + if (view_widget) { + GtkWidget *view_widget; we provide a widget, not a popover. This will be always the case afaics. Do you had somethign in mind? @@ +889,3 @@ + toolbar->priv->active_slot = slot; + + gtk_container_add (GTK_CONTAINER (popover), view_widget); Can this actually happen? @@ +894,3 @@ + icon = nautilus_window_slot_get_icon (slot); + + } not needed, add G_BINDING_SYNC_CREATE and let the bind work as expected. @@ +898,3 @@ + toolbar->priv->icon_binding = g_object_bind_property (slot, "icon", + toolbar->priv->view_icon, "gicon", + We have a problem here tho, the mockups [0] show how when no view-menu is available we show the previous icon but insensitive. To be honest I'm not sure this is correct for every case, but at least it will be for our near-future case that we have either icon-view/list-view or other-locations. So I would say, bind it full and let the previous one there if not specified otherwise? (like with an empty gicon or so). ::: src/nautilus-view.c @@ +6815,3 @@ + gtk_widget_insert_action_group (GTK_WIDGET (view), + "view", + gboolean show_sort_trash, show_sort_search, show_sort_access, show_sort_modification, enable_sort; no, my patch actually removed this code. It's not necesary since we are adding/removing the GActionGroup on slot active/inactive parent_set/parent_unset, which covers all the cases. @@ +6820,3 @@ + g_action_group_has_action (view_action_group, "visible-columns")); + + /* Allow actions from the current view to be activated through Now that we are managing the menus in the view, we can move this along the hierarchy similar to the update_state_actions, which is the one that sets all of these things... So I guess we can have a vfunc like reset_view_menu and let the views modify it alongside the update_actions_state @@ -6758,3 @@ - toolbar = NAUTILUS_TOOLBAR (nautilus_window_get_toolbar (window)); - nautilus_toolbar_reset_menus (toolbar); - nautilus_window_reset_menus (window); here you lost the synchronization of the allow_stop, since window_reset_menus was doing that. ::: src/nautilus-window.c @@ +2273,3 @@ nautilus_window_report_location_change (window); + nautilus_toolbar_set_active_slot (NAUTILUS_TOOLBAR (window->priv->toolbar), new_slot); is this a related change?
Created attachment 309365 [details] [review] (In reply to Carlos Soriano from comment #2) > Review of attachment 309351 [details] [review] [review]: > > So far I like how all of this is going...much better isolation, > understandable, and probability to catch bugs and understand them at first > sight! > > ::: src/nautilus-toolbar.c > @@ +862,3 @@ > + > + if (view_widget) { > + GtkWidget *view_widget; > > we provide a widget, not a popover. This will be always the case afaics. Do > you had somethign in mind? > This code, for the moment, is indeed superfulous. > @@ +889,3 @@ > + toolbar->priv->active_slot = slot; > + > + gtk_container_add (GTK_CONTAINER (popover), > view_widget); > > Can this actually happen? For the moment, no. > > @@ +894,3 @@ > + icon = nautilus_window_slot_get_icon (slot); > + > + } > > not needed, add G_BINDING_SYNC_CREATE and let the bind work as expected. > > @@ +898,3 @@ > + toolbar->priv->icon_binding = > g_object_bind_property (slot, "icon", > + > toolbar->priv->view_icon, "gicon", > + > > We have a problem here tho, the mockups [0] show how when no view-menu is > available we show the previous icon but insensitive. To be honest I'm not > sure this is correct for every case, but at least it will be for our > near-future case that we have either icon-view/list-view or other-locations. > So I would say, bind it full and let the previous one there if not specified > otherwise? (like with an empty gicon or so). > Fixed. > ::: src/nautilus-view.c > @@ +6815,3 @@ > + gtk_widget_insert_action_group (GTK_WIDGET (view), > + "view", > + gboolean show_sort_trash, show_sort_search, show_sort_access, > show_sort_modification, enable_sort; > > no, my patch actually removed this code. It's not necesary since we are > adding/removing the GActionGroup on slot active/inactive > parent_set/parent_unset, which covers all the cases. > Fixed. > @@ +6820,3 @@ > + g_action_group_has_action (view_action_group, "visible-columns")); > + > + /* Allow actions from the current view to be activated through > > Now that we are managing the menus in the view, we can move this along the > hierarchy similar to the update_state_actions, which is the one that sets > all of these things... > So I guess we can have a vfunc like reset_view_menu and let the views modify > it alongside the update_actions_state > The burden to access the NautilusViewDetails instance from subclasses is what makes me avoid this approach. > @@ -6758,3 @@ > - toolbar = NAUTILUS_TOOLBAR (nautilus_window_get_toolbar (window)); > - nautilus_toolbar_reset_menus (toolbar); > - nautilus_window_reset_menus (window); > > here you lost the synchronization of the allow_stop, since > window_reset_menus was doing that. > Fixed. > ::: src/nautilus-window.c > @@ +2273,3 @@ > nautilus_window_report_location_change (window); > > + nautilus_toolbar_set_active_slot (NAUTILUS_TOOLBAR > (window->priv->toolbar), new_slot); > > is this a related change? It is.
> > ::: src/nautilus-window.c > > @@ +2273,3 @@ > > nautilus_window_report_location_change (window); > > > > + nautilus_toolbar_set_active_slot (NAUTILUS_TOOLBAR > > (window->priv->toolbar), new_slot); > > > > is this a related change? > > It is. aah toolbar_* I read it wrong.
> > > @@ +6820,3 @@ > > + g_action_group_has_action (view_action_group, "visible-columns")); > > + > > + /* Allow actions from the current view to be activated through > > > > Now that we are managing the menus in the view, we can move this along the > > hierarchy similar to the update_state_actions, which is the one that sets > > all of these things... > > So I guess we can have a vfunc like reset_view_menu and let the views modify > > it alongside the update_actions_state > > > > The burden to access the NautilusViewDetails instance from subclasses is > what makes me avoid this approach. Ah true, I forgot we are using a language without OOP emulating OOP =) not going to fix inheritance in c anytime soon heh (rant about c done)
Review of attachment 309365 [details] [review]: OK to push with the comment added. Thanks! ::: src/nautilus-toolbar.c @@ +855,3 @@ + + icon = g_value_get_object (from_value); + GValue *to_value, Add a comment since it's a design decision. Along the lines of "As per design decision, we let the previous used icon if no view menu is available"
https://git.gnome.org/browse/nautilus/commit/?id=764b98f6f47395559fde40d47f09ce0cafc4719c Pushed it already since I need to do some stuff in nautilus-view