GNOME Bugzilla – Bug 775628
Add keyboard shortcuts: F10 for page menu
Last modified: 2017-04-23 14:24:02 UTC
Just a couple of suggestions: * Ctrl+? to open Shortcuts, alongside existing Ctrl+F1. The convention seems to be to include both. * F10 to open/toggle the page menu button. This is the usual convention. Although most options there have their own shortcuts/actions, it would be nice to have one simply to open the menu. I started working on patches against master but might have trouble building it, so if you'd be able to test them, that'd be great! I'll upload later on.
As for 3.22, it seems to use a completely different architecture than I'm used to, based on the old GtkActions, so it might take me a while to figure that one out...
(In reply to Daniel Boles from comment #0) > Just a couple of suggestions: > * Ctrl+? to open Shortcuts, alongside existing Ctrl+F1. The convention > seems to be to include both. What other apps use this shortcut? I tried a couple other apps with keyboard shortcuts dialog but none respected this. > * F10 to open/toggle the page menu button. This is the usual convention. > Although most options there have their own shortcuts/actions, it would be > nice to have one simply to open the menu. OK, I see this one works in nautilus.
(In reply to Daniel Boles from comment #1) > As for 3.22, it seems to use a completely different architecture than I'm > used to, based on the old GtkActions, so it might take me a while to figure > that one out... I wouldn't bother with 3.22. New keyboard shortcuts are nonessential and I think it's sufficient to add them going forward.
(In reply to Michael Catanzaro from comment #2) > (In reply to Daniel Boles from comment #0) > > Just a couple of suggestions: > > * Ctrl+? to open Shortcuts, alongside existing Ctrl+F1. The convention > > seems to be to include both. > > What other apps use this shortcut? I tried a couple other apps with keyboard > shortcuts dialog but none respected this. Off the top of my head: Nautilus, Builder, Totem tbh this seems like more a question of whether you want to join the cargo cult than anything useful, as it really amounts to Ctrl+Shift+/ which is a bit of a finger twister ;)
Created attachment 349674 [details] [review] Add F10 keyboard shortcut for page menu
Review of attachment 349674 [details] [review]: Neat! I started on this a while back but got distracted. IIRC, I did nearly or exactly the same thing. I haven't tested this, so I won't ack it, but I can make one tiny comment on style... ::: src/window-commands.c @@ +862,3 @@ + + gtk_popover_popup (popover); + Delete the extra blank line :P
Created attachment 349680 [details] [review] Add F10 keyboard shortcut for page menu
Review of attachment 349680 [details] [review]: ::: 0001-Add-F10-keyboard-shortcut-for-page-menu.patch @@ +2,3 @@ +From: Stefania Popescu <stef@localhost.localdomain> +Date: Sun, 9 Apr 2017 12:50:02 +0300 +Subject: [PATCH] Add F10 keyboard shortcut for page menu Whoops, looks like you accidentally committed the patch file. We don't want that inside the patch itself. ;) ::: src/ephy-header-bar.c @@ +755,2 @@ gtk_header_bar_pack_end (GTK_HEADER_BAR (header_bar), button); + gtk_actionable_set_action_name (GTK_ACTIONABLE (button), "win.page-menu"); Is this really needed? Looks like you're activating the menu manually. ::: src/window-commands.c @@ +862,3 @@ + + gtk_popover_popup (popover); + Please remove this extra blank line.
> ::: src/ephy-header-bar.c > @@ +755,2 @@ > gtk_header_bar_pack_end (GTK_HEADER_BAR (header_bar), button); > + gtk_actionable_set_action_name (GTK_ACTIONABLE (button), "win.page-menu"); > > Is this really needed? Looks like you're activating the menu manually. Yeah, I think if this is to be added - which might be useful for other reasons (e.g. disabling the action in certain cases) - then the menu should be activated by g_action_activate(). That is: one or the other, but not both.
Created attachment 349696 [details] [review] Add F10 keyboard shortcut for page menu Hi :) Sorry for the mistakes, I'm new here :D I hope that this time everything is ok. I removed all the extra lines, including the one from src/ephy-header-bar.c
(In reply to Michael Catanzaro from comment #8) > ::: src/ephy-header-bar.c > @@ +755,2 @@ > gtk_header_bar_pack_end (GTK_HEADER_BAR (header_bar), button); > + gtk_actionable_set_action_name (GTK_ACTIONABLE (button), "win.page-menu"); > > Is this really needed? Looks like you're activating the menu manually. Iulian pointed out that this is needed to activate the code that activates the menu. Does your patch really still work with it removed?
Created attachment 349713 [details] [review] Add F10 keyboard shortcut for page menu I added that line back. It's strange because I tested it multiple times without that line just to make sure that it works and it was ok. I wouldn't have deleted it if it didn't work after that. :D
Well if it works, then we should use the earlier version of your patch. :)
Review of attachment 349696 [details] [review]: Ah yeah, this version is right. It's not needed on the button because this patch is not about what action happens when you click the button, it's about what happens when you press F10.
Great! Thank you very much :)
Pushing the F10 patch.... (In reply to Daniel Boles from comment #0) > Just a couple of suggestions: > * Ctrl+? to open Shortcuts, alongside existing Ctrl+F1. The convention > seems to be to include both. Neither of these shortcuts work in Nautilus or gedit, the first two apps I checked. Please open another bug for Ctrl+? if you really think it's a GNOME convention.
Attachment 349696 [details] pushed as 4d994e9 - Add F10 keyboard shortcut for page menu