After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 753158 - Move pathbar context menu management to pathbar
Move pathbar context menu management to pathbar
Status: RESOLVED FIXED
Product: nautilus
Classification: Core
Component: Views: All
3.17.x
Other Linux
: Normal enhancement
: ---
Assigned To: Nautilus Maintainers
Nautilus Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-08-03 01:29 UTC by Georges Basile Stavracas Neto
Modified: 2015-08-04 14:09 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
pathbar: handle context menu (29.45 KB, patch)
2015-08-03 01:36 UTC, Georges Basile Stavracas Neto
none Details | Review
pathbar: handle context menu (29.47 KB, patch)
2015-08-04 13:50 UTC, Georges Basile Stavracas Neto
committed Details | Review

Description Georges Basile Stavracas Neto 2015-08-03 01:29:21 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.
Comment 1 Georges Basile Stavracas Neto 2015-08-03 01:36:14 UTC
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.
Comment 2 Carlos Soriano 2015-08-04 13:30:41 UTC
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.
Comment 3 Georges Basile Stavracas Neto 2015-08-04 13:50:44 UTC
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."
Comment 4 Carlos Soriano 2015-08-04 13:58:04 UTC
Review of attachment 308735 [details] [review]:

LGTM
Comment 5 Georges Basile Stavracas Neto 2015-08-04 14:09:11 UTC
Attachment 308735 [details] pushed as 5edca92 - pathbar: handle context menu