GNOME Bugzilla – Bug 753158
Move pathbar context menu management to pathbar
Last modified: 2015-08-04 14:09:15 UTC
NautilusView currently manages various context menus, including the pathbar's one. It, however, is not necessary, since the views have no influence over the pathbar's context menu.
Created attachment 308641 [details] [review] pathbar: handle context menu This patch: - Moves the pathbar context menu code from NautilusView to NautilusPathBar - Add a new NautilusPathBar::open-location signal that matches GtkPlacesSidebar::open-location - Adapt NautilusWindow to the changes Hopefully it's enought.
Review of attachment 308641 [details] [review]: About the commit message. Probably is because you don't know the history, but before the GAction rework it actually depended on the view. So, instead of "depends on the current view by no means" which sounds like "it never made sense", can you say that it's not related since the GAction rework? Also, can you say that this is going to go away probably soon with the work on GtkPathBar that is intended to make it public soon? As a side note, I won't be picky in the review with this patch since it will go more or less away (not the idea and way of implementation, which I will for sure use). (just reviewed, and even if I would be picky, imho the patch looks fine as is). ::: src/nautilus-pathbar.c @@ +95,3 @@ gboolean drag_slider_timeout_for_up_button; + + GActionGroup *action_group; Since this doesn't change depending on more than the current window, there is not need to use a separate action group, and you can attach it directly to the window. To be honest, not always doing it make it fail for some cases like a GtkDialog, which it's window is not a GtkApplicationWindow, which implements GActionGroup and GActionMap. So, I would say, I will start creating an action group for every class that implements some actions. So this looks fine.
Created attachment 308735 [details] [review] pathbar: handle context menu Updated commit message as follows: "While this in theory provides a good isolation from other classes, in practice NautilusView manages the pathbar context menu, which is not necessary, as it doesn't depend on the current view anymore after the GAction rework."
Review of attachment 308735 [details] [review]: LGTM
Attachment 308735 [details] pushed as 5edca92 - pathbar: handle context menu