GNOME Bugzilla – Bug 773154
Make context menus opened by keyboard pop up relative to the selected items, not the pointer
Last modified: 2017-08-23 13:12:18 UTC
Created attachment 337938 [details] demo video showing menus opened at rename_popover rectangle While puzzling through https://bugzilla.gnome.org/show_bug.cgi?id=773120 I got my first (totally redundant!) attempt working, but felt it was clumsy that the keyboard-activated menus just pop up wherever the mouse cursor is, even if that is far away from the Nautilus window. Users preferring keynav might not keep track of their mouse cursor, and so the menu might appear very far away, on a totally different screen, etc. That can be quite inconvenient/distracting indeed. It would be nicer to have the keyboard open the menu relative to the selected items, so things wouldn't depend on the mouse cursor at all. This is done in * other famous file managers * things like Firefox, which uses the text cursor position if available, otherwise just the top-left of the body * various text editors, etc. This precedent is probably why I immediately thought the current behaviour was less than ideal and tried to patch in something more intuitive/convenient. Ernestas agreed, and said it was a great idea to open the menu at the selection. My initial patch set for the other ticket achieved this, but because the way I handled the keyboard shortcuts was redundant/incorrect, integrating this would require a somewhat different approach. I'd like to investigate this and hopefully get this figured out soon. Attached is the proof-of-concept video from the previous implementation of this; since my current plan is to use the same overall logic and GdkRectangle (recycled from the rename popover), the end result will probably look identical to this.
cool!
Thanks for the interest, both! Carlos's comment both reminded and encouraged me to look at this again. :D I have done some cleanups (or at least, I think they're better ;) as preparation, and I'd like to handle the background menu as part of this, too. Patches coming your way...
Created attachment 338454 [details] [review] [PATCH 1/6] files-view: ren context_ > selection_menu_position > We manage 2 context menus, but context_menu_position variables and > functions only apply to 1, so their names were needlessly ambiguous. > This makes it clear they're only relevant to the selection menu. It also > avoids updating said position on events coming from the background menu. (At least, I hope that's right!)
Created attachment 338455 [details] [review] [PATCH 2/6] files-view: remove unnecessary 'helper' function > selection_not_empty_in_menu_callback () was misnamed: it was called from > 2 non-callback sites. And all it did was to check if a selection was not > NULL, in an amusingly roundabout way. It's easier just to do that inline. > > 1 of the checks was redundant, and if not, was called in the wrong order This just bugged me, really! I needed to check whether the selection was empty in one of the next patches, and I saw that function was used elsewhere for this, and both its implementation and uses just seem silly to me. :P
Created attachment 338456 [details] [review] [PATCH 3/6] views: rename compute_rename_popover_pointing_to… > …to rectangle_pointing_to_selection. > > This will be used to position keyboard-triggered context menus over the > selection, so rename it to reflect that it has a more general role now.
Created attachment 338457 [details] [review] [PATCH 4/6] ui-utilities: add _pop_up_context_menu_pointing_to > This will be used to make the context menu pop up at an appropriate > place relative to the selected items when invoked by the menu key. Instead of a GdkEventButton, this takes a GdkRectangle, such as our very own artist formerly known as compute_rename_popover_pointing_to () from the previous commit.
Created attachment 338459 [details] [review] [PATCH 5/6] Show selection menu opened by keyboard at the file > Selection context menus opened by keyboard would just popup wherever the > mouse pointer happened to be - which wasn't necessarily in any Nautilus > window or even on the same screen, etc. Users who prefer keyboard > navigation might not have any idea where their mouse pointer is! Make > this nicer, and catch up with other big file managers, by contextually > popping up this menu at a position relative to the selected item(s). If > there are 2+ items selected, the menu is positioned at the 1st of them. Finally! :D
Created attachment 338460 [details] [review] [PATCH 6/6] ui-utils: popup bg menu opened by kb at top-right… > …rather than wherever the pointer happens to be. As per the last commit, > this is much nicer for keynav users, whose pointer may be far away in > an inconvenient place. It also follows e.g. Firefox if no text cursor is > available. But unlike FF, we pop up at top-right instead of -left, as > this is less obstructive and aligns nicely with the HeaderBar Buttons. > > We presume a NULL EventButton means the menu was opened by the keyboard. This fixes the same issue for the background context menu, which now pops up at the top-right of the window, rather than possibly miles away at the mouse pointer. I debated myself back and forth about whether the top-left, top-right, or even bottom-something was better for this... and in the end, I feel that top-right is best. And here's why... Firefox, if no text cursor is available, puts its keyboard-triggered context menu at the top left. However, I felt that was a big ugly in Nautilus - and that if we must arbitrarily obscure some of the user's files, then doing so at the top-right is both least obstructive - and also has the handy side-effect of being nearer all the action buttons, which are at that position in the HeaderBar. Lastly, hopefully the convention established in the last line is accurate. I haven't found a case for which any of this this misbehaves, at least!
Review of attachment 338455 [details] [review]: Yup, the check is also inside can_set_wallpaper() (update your commit message - you should inspire confidence!).
Review of attachment 338456 [details] [review]: I am missing a verb here… _get_rectangle_at_selection()? Not sure. Carlos, could you chime in? Also, you can just say “rename compute_rename_popover_pointing_to()” or somesuch in the subject line, as the diff will show what it was renamed to.
Review of attachment 338457 [details] [review]: As with patch three, I would use “pop_up_context_menu_at_rectangle()”. That is what GTK+ uses (more or less). ::: src/nautilus-ui-utilities.h @@ -1,1 @@ - Eh, you can leave this in. I am sure there is already a number of files that have this.
Review of attachment 338459 [details] [review]: Going on the same thread as with the last patches. ::: src/nautilus-files-view.c @@ +7788,3 @@ GdkEventButton *event) { + GdkRectangle *pointing_to; As with previous patches, if we are going to stick with “rectangle” as opposed to “pointing to”, this should be renamed as wel. @@ +7799,3 @@ update_selection_menu_position_from_event (view, event); + pointing_to = nautilus_files_view_rectangle_pointing_to_selection(view); You forgot the space! And time!
Review of attachment 338460 [details] [review]: I think this is a reasonable choice. Any thoughts, Carlos?
Review of attachment 338454 [details] [review]: Again, it would be better to use “rename context_menu_position” or something to the same effect in the subject line. ::: src/nautilus-files-view.c @@ -7831,3 @@ update_context_menus_if_pending (view); - update_context_menu_position_from_event (view, event); This does not seem to be doing anything noticeable, indeed, so I think removing this is fine.
And thanks for working on this!
Created attachment 338509 [details] [review] [PATCH 1/5] files-view: remove unnecessary helper function files-view: remove unnecessary helper function selection_not_empty_in_menu_callback was simply a verbose way to test whether the selection GList was NULL, which is tidier to just do inline. The call that gave this function its name was out of order if it were really needed, but it's not since can_set_wallpaper already checks this. I'm not sure about the whole context_menu_position thing right now, so I'm not applying that patch, so this is now 1/5 instead of 2/6.
Created attachment 338510 [details] [review] [PATCH 2/5] views: rename compute_rename_popover_pointing_to It will also be used to position keyboard-triggered context menus at the selection, so rename it to reflect that it now has a more general role.
Created attachment 338511 [details] [review] [PATCH 3/5] ui-utilities: add _pop_up_context_menu_pointing_to This will be used to make the context menu pop up at an appropriate place relative to the selected items when invoked by the menu key.
Created attachment 338512 [details] [review] [PATCH 4/5] Show selection menu opened by keyboard at the file Selection context menus opened by keyboard would just popup wherever the mouse pointer happened to be - which wasn't necessarily in any Nautilus window or even on the same screen, etc. Users who prefer keyboard navigation might not have any idea where their mouse pointer is! Make this nicer, and catch up with other big file managers, by contextually popping up this menu at a position relative to the selected item(s). If there are 2+ items selected, the menu is positioned at the 1st of them.
Created attachment 338513 [details] [review] [PATCH 5/5] ui-utils: popup bg menu opened by kb at top-right… …rather than wherever the pointer happens to be. As per the last commit, this is much nicer for keynav users, whose pointer may be far away in an inconvenient place. It also follows e.g. Firefox if no text cursor is available. But unlike FF, we pop up at top-right instead of -left, as this is less obstructive and aligns nicely with the HeaderBar Buttons. We presume a NULL EventButton means the menu was opened by the keyboard. This precedent has been established by recent commits in these areas.
Btw, unless I've missed something, a side effect of this series of patches is that it also seems to fix popup menus in nautilus-desktop, which with unpatched 3.22, don't open and generate this backtrace: (nautilus-desktop:27938): Gtk-CRITICAL **: gtk_menu_popup_at_rect: assertion 'GDK_IS_WINDOW (rect_window)' failed Thread 1 "nautilus-deskto" received signal SIGTRAP, Trace/breakpoint trap. 0x00007ffff4602241 in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0 (gdb) bt
+ Trace 236774
Now, using the context menu key/s on the desktop not only works, but also pops up in a nice place too. :-)
Ah ...never mind. I was accidentally running 3.22.1 as distributed, rather than a fresh build from the gnome-3-22 branch (prior to my patches). So the prior comment will just be the same problem I already fixed in Bug 773120.
Review of attachment 338509 [details] [review]: It is funny, because Carlos and I were talking about redundancy, explicitness or whatever you may call it in the codebase. I do not think there is much value in writing such functions.
Review of attachment 338510 [details] [review]: LGTM
Review of attachment 338511 [details] [review]: Looks good. Oh, and update the subject line in the commit message. ::: src/nautilus-ui-utilities.c @@ +191,3 @@ +nautilus_pop_up_context_menu_at_rectangle (GtkWidget *parent, + GMenu *menu, + GdkRectangle *pointing_to) Picking nits again. Substitute “rectangle” for “pointing_to”. ::: src/nautilus-ui-utilities.h @@ +39,3 @@ +void nautilus_pop_up_context_menu_at_rectangle (GtkWidget *parent, + GMenu *menu, + GdkRectangle *pointing_to); Same here.
Created attachment 338515 [details] [review] [PATCH 3/5] ui-utilities: add pop_up_context_menu_at_rectangle Oops!
Review of attachment 338454 [details] [review]: Hey, thanks for working on this. How is that the background menu shouldn't update the current event? What about a new folder or new document? Those need the last known position to get passed to the operation.
(In reply to Carlos Soriano from comment #27) > Review of attachment 338454 [details] [review] [review]: > > Hey, thanks for working on this. > > How is that the background menu shouldn't update the current event? What > about a new folder or new document? Those need the last known position to > get passed to the operation. Indeed, I realised the same, so I've excluded that patch from my new set. ;-)
Come to think of it - does the selection menu need to cache the position? Maybe I got that the wrong way round altogether!
Review of attachment 338509 [details] [review]: ugh, I only hope I was not the one writing this :)
(In reply to Carlos Soriano from comment #27) > Review of attachment 338454 [details] [review] [review]: > > Hey, thanks for working on this. > > How is that the background menu shouldn't update the current event? What > about a new folder or new document? Those need the last known position to > get passed to the operation. Argh, my bad.
Review of attachment 338510 [details] [review]: "at selection" sounds like the rectangle is already there, not from it. How about get_rectangle_for_selection, or get_rectangle_of_selection, or compute_rectangle_of_selection, or compute_rectangle_for_selection.
Review of attachment 338512 [details] [review]: That assumption is not correct. What if the first item of the selection is out of the view?
(In reply to Carlos Soriano from comment #33) > Review of attachment 338512 [details] [review] [review]: > > That assumption is not correct. What if the first item of the selection is > out of the view? Oh, right. Same goes for the last and any other nth. Scroll to the item, then?
(In reply to Ernestas Kulik from comment #34) > (In reply to Carlos Soriano from comment #33) > > Review of attachment 338512 [details] [review] [review] [review]: > > > > That assumption is not correct. What if the first item of the selection is > > out of the view? > > Oh, right. Same goes for the last and any other nth. Scroll to the item, > then? Or special-case out-of-bounds rects and gravitate towards and edge of the view?
Created attachment 338521 [details] [review] [PATCH 4/5] Show selection menu opened by keyboard at the file Good point, Carlos. This one scrolls to the 1st selected file before opening the menu.
...gah. It should only do that if the file is out-of-view, as otherwise it looks ugly and is kind of inconvenient. Please hold!
Created attachment 338522 [details] [review] [PATCH 4/5] Show selection menu opened by keyboard at the file Only scroll to the 1st selected file if the computed rectangle does not intersect with the View widget. In that case, we scroll to the file, and then re-compute the rectangle, then pop up there.
Created attachment 338523 [details] [review] [PATCH 2/5] views: rename compute_rename_popover_pointing_to Change from _at_selection to _for_selection as per Carlos's comment.
Created attachment 338524 [details] [review] [PATCH 4/5] Show selection menu opened by keyboard at the file Ditto, and also rename leftover pointing_to occurrences to rectangle as requested by Ernestas.
can you push a wip branch with the patches in order? I'm a little confused :)
(In reply to Carlos Soriano from comment #41) > can you push a wip branch with the patches in order? I'm a little confused :) My Bugzilla threads tend to do that to people. ;) I hoped the [PATCH n/5] would be OK. But I appreciate the number of comments makes things more confusing than they need to be. If you do need a branch, I haven't had commit access before, so I have had to request an account through the normal process.
(In reply to db0451 from comment #42) > (In reply to Carlos Soriano from comment #41) > > can you push a wip branch with the patches in order? I'm a little confused :) > > My Bugzilla threads tend to do that to people. ;) I hoped the [PATCH n/5] > would be OK. But I appreciate the number of comments makes things more > confusing than they need to be. > > If you do need a branch, I haven't had commit access before, so I have had > to request an account through the normal process. It's not your fault, and that helped. But bugzilla is a mess for reviewing a serie of patches that should go in order. Or maybe it's me that I'm slow :D In any case, feel free to use any other service (github, etc.) that you have an account already to have a branch, at least that can help me a little.
nevermind, I can do it locally just reordering the patches with the number you put :)
Indeed, I've found we can just save as 1, 2, 3, 4, 5.patch in the right order, then run > git am *.patch which due to globbing will expand and apply the patches in alphanumeric order. :D I've done this on my home machine now and will try to test this in more detail. I was distracted by this at work, I think because I hoped it could be rushed into 3.22.2, but it's probably more complicated than a 1-day turnaround. :P I don't have an active github at the moment, but I should probably set one up. Testing some more, I've already seen that in list view, after scrolling to the file, the menu does not open in the right place, until closed then opened again. Maybe I'm not using gtk_widget_intersect() properly, or it doesn't interact right with the ScrolledWindow... But in canvas view, this seems OK (it gets the rectangle again after scrolling to the file and works just the same as if you close/reopen). Having said that, the scroll_to_file does nothing if a scroll is already underway when you press the key. I don't know whether there's a way around that, e.g. to say 'stop scrolling that way, and do this instead!'
Created attachment 338558 [details] [review] [PATCH 6] files-view: ensure rename popup is within viewport Another one, just to confuse Carlos some more. ;-) This follows a previous commit to do the same for the selection context menu when opened by keyboard. I noticed that the rename Popover when opened by F2 would just appear over the selection too. If the user had since scrolled away from the selection, the Popover would not be visible. If they figured out what was going on and tried to scroll to it, that would fail, as it was grabbing all input! The result was very confusing. This applies the same fix to the rename Popover, making it usable for keyboard navigating users who had scrolled away from their selection. I use a new helper function to scroll to ensure the selection is visible, if it is not already. This is shared between the rename Popover and the key-activated selection menu.
Hey sorry for the delay, I had a week working on a different stuff. I will review now.
So I'm trying the patches and they doesn't work at all, the popup is always open in the top right corner of the window.
Review of attachment 338513 [details] [review]: I don't think this is a good idea. I would open it in the middle of the view, no? ::: src/nautilus-files-view.c @@ +7808,3 @@ * nautilus_files_view_pop_up_background_context_menu * + * Pop up a context menu appropriate to the view overall. The patch is about the background menu, but you are modifying this which is for the selection menu no? ::: src/nautilus-ui-utilities.c @@ +191,3 @@ + GDK_GRAVITY_NORTH_EAST, + GDK_GRAVITY_NORTH_EAST, + gtk_get_current_event ()); So this will be done no matter if there is selection or not?
Review of attachment 338558 [details] [review]: I would do the oposite, scroll_to_file should work already for this pourpose, but it's quite cumbersome that it's using a URI instead of a NautilusFile. So I would just refactor the function to take a NautilusFile, then you can do always scroll_to_file, not need to check for the intersection.
Review of attachment 338523 [details] [review]: The name is misleading. You are returning the bounding box of just one element of the selection, not the whole bounding box. So that's one issue. But also, why only the first bounding box? Wouldn't you be interested in returning the bounding box of the whole rectangle of the selection? (you will need some math here :)) That's more likely to be the source of your problems with the patches serie.
Review of attachment 338524 [details] [review]: As said before, I think is a better idea to popup in the middle of the rectangle of the whole selection. Any reason you make it to be the first one?
Review of attachment 338515 [details] [review]: ::: src/nautilus-ui-utilities.c @@ +209,3 @@ + rectangle, + GDK_GRAVITY_SOUTH_WEST, + GDK_GRAVITY_NORTH_WEST, Any reason for this values? A comment would be good. @@ +210,3 @@ + GDK_GRAVITY_SOUTH_WEST, + GDK_GRAVITY_NORTH_WEST, + gtk_get_current_event ()); you can pass NULL if it's the current event. However, are we sure is always the current event who triggers this? We might need, in a future, make some async call and popup the menu after that async call (for example...scrolling :)) and an event can happen in between. Not sure how this would work out in a good way, most probably this function is okay not event-aware, and instead check on the caller whether the current event is the same as the one that is going to be used here. ::: src/nautilus-ui-utilities.h @@ +34,3 @@ + const gchar *submodel_name, + gboolean prepend); +void nautilus_pop_up_context_menu (GtkWidget *parent, I would rename this to *_at_mouse_position. In the current situation, this one looks generic, and the other one not. But it's truly not generic none of them right now.
The popup menu for selection also doesn't detect I have a selection, so something might be wrong in these patches. Does it happens to you? In any case, thanks a lot for working on this! I think you are doing a good progress here :)
(In reply to Carlos Soriano from comment #48) > So I'm trying the patches and they doesn't work at all, the popup is always > open in the top right corner of the window. Something must have changed since I wrote these, if that's the case. they worked fine for me, although in list view the popup was a little bit offset from the 1st file in the selection. Are you applying on 3.22 or master? I didn't try the latter.
(In reply to Carlos Soriano from comment #49) > Review of attachment 338513 [details] [review] [review]: > > I don't think this is a good idea. I would open it in the middle of the > view, no? I guess that could work too. I like top-right, but whatever you prefer. > ::: src/nautilus-files-view.c > @@ +7808,3 @@ > * nautilus_files_view_pop_up_background_context_menu > * > + * Pop up a context menu appropriate to the view overall. > > The patch is about the background menu, but you are modifying this which is > for the selection menu no? IIRC/AFAICT (it's been a while) the selection menu now uses popup_at_rectangle, so it never hits this path, which is effectively only for the background menu. > ::: src/nautilus-ui-utilities.c > @@ +191,3 @@ > + GDK_GRAVITY_NORTH_EAST, > + GDK_GRAVITY_NORTH_EAST, > + gtk_get_current_event ()); > > So this will be done no matter if there is selection or not? As above, it shouldn't be relevant to the selection anymore; it should only fire for the background menu, and check whether or not we have a pointer event, and if not, pop up at the top-right (or maybe middle instead). but maybe the binary I was running wasn't a complete reflection of the source, or something. I'll need to rebase and check.
I'm in master, patches has to always be for master, then they can be backported.
(In reply to Carlos Soriano from comment #51) > Review of attachment 338523 [details] [review] [review]: > > The name is misleading. You are returning the bounding box of just one > element of the selection, not the whole bounding box. So that's one issue. > > But also, why only the first bounding box? Wouldn't you be interested in > returning the bounding box of the whole rectangle of the selection? (you > will need some math here :)) I considered this, but I thought it would be more confusing if the user has a widely dispersed selection. We would always have to make an arbitrary choice, and I thought the 1st was as good as any. But again, maybe my intuition isn't the same as yours and/or the general user. ;)
(In reply to dboles from comment #56) > (In reply to Carlos Soriano from comment #49) > > Review of attachment 338513 [details] [review] [review] [review]: > > > > I don't think this is a good idea. I would open it in the middle of the > > view, no? > > I guess that could work too. I like top-right, but whatever you prefer. > I'm not sure top right is where you are looking when firing the background menu no? You are looking at the view. If not, you would open the view menu or hamburguer menu in the toolbar. > > ::: src/nautilus-files-view.c > > @@ +7808,3 @@ > > * nautilus_files_view_pop_up_background_context_menu > > * > > + * Pop up a context menu appropriate to the view overall. > > > > The patch is about the background menu, but you are modifying this which is > > for the selection menu no? > > IIRC/AFAICT (it's been a while) the selection menu now uses > popup_at_rectangle, so it never hits this path, which is effectively only > for the background menu. > I see. In any case, this is a function in nautilus-ui-utilities. So it has to work fine if we are going to use it for anything else. Then we should require to have a mouse event here, because wouldn't make sense otherwise. > > ::: src/nautilus-ui-utilities.c > > @@ +191,3 @@ > > + GDK_GRAVITY_NORTH_EAST, > > + GDK_GRAVITY_NORTH_EAST, > > + gtk_get_current_event ()); > > > > So this will be done no matter if there is selection or not? > > As above, it shouldn't be relevant to the selection anymore; it should only > fire for the background menu, and check whether or not we have a pointer > event, and if not, pop up at the top-right (or maybe middle instead). > > but maybe the binary I was running wasn't a complete reflection of the > source, or something. I'll need to rebase and check. I see. Another issue I'm having is that the popup for the selection using mouse is being poped up using the rectangle, instead of the mouse event position.
(In reply to dboles from comment #58) > (In reply to Carlos Soriano from comment #51) > > Review of attachment 338523 [details] [review] [review] [review]: > > > > The name is misleading. You are returning the bounding box of just one > > element of the selection, not the whole bounding box. So that's one issue. > > > > But also, why only the first bounding box? Wouldn't you be interested in > > returning the bounding box of the whole rectangle of the selection? (you > > will need some math here :)) > > I considered this, but I thought it would be more confusing if the user has > a widely dispersed selection. We would always have to make an arbitrary > choice, and I thought the 1st was as good as any. > > But again, maybe my intuition isn't the same as yours and/or the general > user. ;) That is actually my point, for a wider selection you are not likely looking at the first item you selected, but most probably looking at the end of the selection (think that we usually select items going from up to down, because is how we are used to read, from top to down). So wouldn't make sense to be the first one, most probably the last one. But I think none of those are better than the middle or something like that, where is the most approachable from a selection.
(In reply to Carlos Soriano from comment #60) > So wouldn't make sense to be the first one, most probably the last one. But > I think none of those are better than the middle or something like that It should also be noted that not all selections are continuous. I could select a file at the top of the view, a file at the bottom and one somewhere in between, so I would go for the last one, else you could get very inconvenient scrolling.
Good point
Belated thanks for all the feedback, both. Clearly this will require a lot of rethinking! I'd better save it for one of the long weekends I have coming up. :) Looking forward to getting stuck in though, lots of interesting challenges.
And we are looking forward to work with you! Those are nice patches and discussions :)
Comment on attachment 338509 [details] [review] [PATCH 1/5] files-view: remove unnecessary helper function I need something to both ensure my git access doesn't get cancelled, and very slightly ease me back into looking at nautilus source, so!
Let's mark this as a dupe of the old one, and just reupload/restart discussion of the stuff here if it's useful at the other bug. *** This bug has been marked as a duplicate of bug 102666 ***