GNOME Bugzilla – Bug 773120
(3.22.1 regression) Keyboard shortcuts for selection and background context menus stopped working in canvas view
Last modified: 2016-12-12 17:03:35 UTC
Nautilus does not respond to the right-click context menu key, or equivalent Alt+F10, and open the same menu as a right-click, as it should. This used to work, in a past life... see https://bugzilla.gnome.org/show_bug.cgi?id=136578 and many old, closed bugs. I guess refactoring has led to this keyboard shortcut getting lost. Thanks!
patch (attempt) underway...
Created attachment 337887 [details] [review] files-view: make the context menu key and Alt+F10 emulate a mouse right-click and open the context menu This patch against the gnome-3-22 branch successfully emulates a mouse right-click when either Menu or Alt+F10 are pressed. This just makes the menu pop up at wherever the mouse pointer happens to be currently, which can even be outside the Nautilus window. I feel like maybe this is not sufficient, and it should somehow get the coordinates for the 'first' (a.k.a. top-start) selected file and pop up near that... Do you think so too?
Created attachment 337888 [details] [review] files-view: make the context menu key and Shift+F10 emulate a mouse right-click and open the context menu fix - oops, it's Shift+F10 that is normally the backup for the Menu key and even that might only be on Windows, from a quick search... so we might want to leave it out, maybe?
Created attachment 337893 [details] [review] (1) files-view: make menu key and Shift+F10 emulate right-click and open context menu 1 of 3 As mentioned, let me know if you would like the Shift+F10 shortcut removed.
Created attachment 337894 [details] [review] (2) ui-utilities: add nautilus_pop_up_context_menu_pointing_to () 2 of 3 > This will be used in the next commit to make the context menu pop up at > the top-left file in the selection when invoked by the menu key. If you'd prefer to use different names or signatures for the new functions, or put them somewhere else, or etc. - then let me know, and I'll move them.
Created attachment 337895 [details] [review] files-view: popup context menu opened by keyboard over top left item in selection 3 of 3 > This uses the function that was just added to ui-utilities, > nautilus_pop_up_context_menu_pointing_to (), passing the GdkRectangle > reused from pnautilus_files_view_compute_rename_popover_pointing_to () Arguably, we might prefer to rename the latter function, but I didn't want to break anything for the purposes of proving this concept!
Created attachment 337896 [details] [review] files-view: make context menu opened by keyboard popup relative to selection improved commit explanation > This uses the same position as renaming, by passing the GdkRectangle > reused from nautilus_files_view_compute_rename_popover_pointing_to () > to ui-utilities' newly added nautilus_pop_up_context_menu_pointing_to () of course, my previous comment still applies, if you'd prefer to rename or relocate any of the functions that are now involved in this.
Created attachment 337897 [details] screencast showing the results of the patch That almost seems too easy... What have I missed? ;-)
Created attachment 337899 [details] [review] (4) files-views: rename compute_rename_popover_pointing to => compute_rectangle_pointing_to 4 of 3... > This is now used to position keyboard-triggered context menus over the > selection, so rename it to reflect that it has a more general role now. just a bonus, assuming it can't somehow break anything (of course, it builds ok)
Created attachment 337901 [details] [review] (3) files-view: make context menu opened by keyboard popup relative to selection This version improves the doc comment on the new function... which I'm not sure should even be exposed/documented. ;-)
Created attachment 337902 [details] [review] (1) ui-utilities: add nautilus_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. These are reordered and squashed versions of the original series, which are much tidier.
Created attachment 337903 [details] [review] (2) files-views: rename compute_rename_popover_pointing to => compute_rectangle_pointing_to > 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 337904 [details] [review] (3) files-view: make menu key and Shift+F10 open context menu, pointing to selection ssssquash. I'll stop spamming now...
Created attachment 337905 [details] [review] (1) ui-utilities: add nautilus_pop_up_context_menu_pointing_to () I guess there should be an assertion for that the GdkRectangle is non-NULL.
Created attachment 337906 [details] [review] (1) ui-utilities: add nautilus_pop_up_context_menu_pointing_to () Again, sorry for the spam... I'll definitely stop now and let you review these without being constantly bombarded with changes! :-) This update adds an assert that we get a valid GdkWindow at which to pop up. Note that gtk_widget_get_window () asserts GTK_IS_WIDGET (), so I removed that check from our call site, as it would've been duplicated if it was left there.
Created attachment 337907 [details] [review] (3) files-view: make menu key and Shift+F10 open context menu, pointing to selection D'oh. Don't segfault if there's no selection.
Created attachment 337908 [details] [review] (4) help-overlay: document keyboard shortcuts for opening context menu > also create a new section for Selecting, to stop Editing getting too big, and rearrange the latter to flow a bit better (IMHO) (If given free reign, I'd probably split Editing further, but!)
Created attachment 337909 [details] [review] (2) files-views: rename compute_rename_popover_pointing_to => compute_rectangle_pointing_to commit msg: fix old func name, add this bug url
The menu key not working as it should is a consequence of 1f57c5b19c099ae44b2c8ec8ca9a1904f1ed7885. You will notice that some warnings are emitted when you use it. I would suggest fixing that (I think it is as simple as passing NULL for GdkEventButton pointer instead of a pointer to a zero-initialized struct from handle_popups()) and then seeing if we want to add the Shift-F10 accel. I really like your idea of popping the context menu up at the selection, however.
Also, the subject line in some of your patches is too long. Try to follow the 50/72 formatting if you can.
(In reply to Ernestas Kulik from comment #19) > The menu key not working as it should is a consequence of > 1f57c5b19c099ae44b2c8ec8ca9a1904f1ed7885. Oh, you're right - I have the previous version of Nautilus on this other machine, and it's OK there. Shift+F10 works there, too! > You will notice that some warnings > are emitted when you use it. I only noticed warnings for Ctrl+F10, and wondered where they were coming from, but on the previous version, this shortcut opens the background context menu. So that's broken in 3.22.1 as well (and could be added to the help-overlay). > I would suggest fixing that (I think it is as simple as passing NULL for > GdkEventButton pointer instead of a pointer to a zero-initialized struct > from handle_popups()) What is handle_popups()? I couldn't find it. > and then seeing if we want to add the Shift-F10 accel. No need, it was already there and will presumably be restored by the fix, if it's as simple as you say. > I really like your idea of popping the context menu up at the selection, > however. Indeed, that looks a lot nicer. I guess we should fix the error first and then think about a separate patch for improving the positioning. Where would be the best place to check whether the event came from the mouse or the keyboard? (In reply to Ernestas Kulik from comment #20) > Also, the subject line in some of your patches is too long. Try to follow > the 50/72 formatting if you can. OK, I wasn't sure whether/what the limit was for subjects, and I think vim just splits at 80 characters in the comment.
(In reply to djb from comment #21) > (In reply to Ernestas Kulik from comment #19) > > I would suggest fixing that (I think it is as simple as passing NULL for > > GdkEventButton pointer instead of a pointer to a zero-initialized struct > > from handle_popups()) > > What is handle_popups()? I couldn't find it. Got it, in nautilus-canvas-container.c. Why, after all this time, do I still start by searching for things manually instead of grepping?
(In reply to djb from comment #21) > Where would be the best place to check whether the event came from the mouse > or the keyboard? I am going to sound like a broken record here, but handle_popups() is called when you try to pop up a menu by using the keyboard, it appears. I honestly do not know off the top of my head, but that seems like a good starting point, at least. You can find where gtk_menu_popup_at_pointer() is called and follow the code backwards, but it will most probably lead you to view code. An interesting thing to note is the list view. The accelerators work fine there, except for Ctrl-F10.
Created attachment 337932 [details] [review] canvas-container: fix opening menus using keyboard > canvas-container: fix opening menus using keyboard > > This was broken by commit 1f57c5b19c099ae44b2c8ec8ca9a1904f1ed7885. > When the keyboard was used to pop up these menus, a zero-initialised > GdkEventButton was passed to gtk_menu_popup_at_pointer, which led to > this warning: > > Gtk-CRITICAL **: gtk_menu_popup_at_rect: assertion 'GDK_IS_WINDOW > (rect_window)' failed" > > As suggested by Ernestas Kulik, we can fix this by passing a NULL event > pointer instead, which leads to the GtkMenu function falling back to the > current cursor position anyway, and the menu opens OK at that location. Here is the basic patch, as suggested by Ernestas. His idea does indeed seem to fix the problem of both context menus not opening, while being a tonne simpler than the equivalent part of my patch, which really just added a redundant 2nd layer of keyboard handling. :D Getting this working again is most important, though it would be nice to improve the positioning like my other patches did, as a bonus after this.
(In reply to Ernestas Kulik from comment #23) > An interesting thing to note is the list view. The accelerators work fine > there, except for Ctrl-F10. I didn't notice that the list worked OK, but did notice that the canvas and list views seem not to share all the keyboard handling code they could, which makes it more difficult to alter things like this. I'll look at submitting a patch to get Ctrl+F10 working. I'll start another BZ for that.
Review of attachment 337932 [details] [review]: Awesome!
(In reply to djb from comment #24) > Getting this working again is most important, though it would be nice to > improve the positioning like my other patches did, as a bonus after this. Could you file another bug report for that? Thanks for catching all these bugs. :)
Attachment 337932 [details] pushed as d48c2e8 - canvas-container: fix opening menus using keyboard
(In reply to Ernestas Kulik from comment #26) > Review of attachment 337932 [details] [review] [review]: > > Awesome! You did all the real work. :D Thanks for the suggestion. (In reply to Ernestas Kulik from comment #27) > (In reply to djb from comment #24) > > Getting this working again is most important, though it would be nice to > > improve the positioning like my other patches did, as a bonus after this. > > Could you file another bug report for that? Thanks for catching all these > bugs. :) Yeah, sure, I'll do that later - maybe once I have sketched out how it could work. This ticket got to be too much of a mess, as I was too excited to upload things, rather than making sure they were final, haha.
(In reply to djb from comment #29) > Yeah, sure, I'll do that later - maybe once I have sketched out how it could > work. This ticket got to be too much of a mess, as I was too excited to > upload things, rather than making sure they were final, haha. Even though your initial patches were not pushed, you came up with a great idea, so that more than makes up for the horror when I noticed over twenty new Nautilus bugmails when I woke up, haha. And to comment a little about the help overlay: it is kind of a restricted area at this juncture. It already suffers from the problem of it being too big and Carlos does not want to play with it willy-nilly, because of its particular design. I will try to bring it up to him when he gets back, but nothing is certain.
Thanks for all the great feedback and very glad I could help! Here's the ticket for enhancing the positioning of these menus: https://bugzilla.gnome.org/show_bug.cgi?id=773154
*** Bug 773227 has been marked as a duplicate of this bug. ***
*** Bug 775530 has been marked as a duplicate of this bug. ***
*** Bug 775989 has been marked as a duplicate of this bug. ***
*** Bug 776000 has been marked as a duplicate of this bug. ***