GNOME Bugzilla – Bug 757909
Add a help overlay for shortcuts
Last modified: 2015-11-18 19:10:22 UTC
Let's add a help overlay for shortcuts to play with the new GtkShortcutsWindow in gtk+.
Created attachment 315393 [details] [review] Added help overlay for shortcuts with the new GtkShortcutsWindow
Created attachment 315394 [details] Shortcut Window
Review of attachment 315393 [details] [review]: Thanks for the patch, Umang. It looks great. A few comments below: According to https://wiki.gnome.org/Initiatives/GnomeGoals/ShortcutWindows , we should add a "Keyboard Shortcuts" entry in the application menu. ::: src/help-overlay.ui @@ +51,3 @@ + <object class="GtkShortcutsShortcut"> + <property name="visible">1</property> + <property name="title" translatable="yes">Gear Menu</property> The term "gear menu" is an informal one. It's not even relevant these days because we changed the icon to be a hamburger. I think a more proper term would be "action menu". @@ +52,3 @@ + <property name="visible">1</property> + <property name="title" translatable="yes">Gear Menu</property> + <property name="accelerator">F12</property> F10, not F12. @@ +114,3 @@ + <property name="title" translatable="yes">Exit from preview mode</property> + <property name="accelerator"><alt>Left</property> + </object> Alt+left is not just for the preview only. eg., if you are inside a collection, then it will take you out of it. Maybe put it in "navigation"? ::: src/photos.gresource.xml @@ +16,3 @@ <gresource prefix="/org/gnome/Photos/gtk"> <file alias="menus.ui" preprocess="xml-stripblanks" compressed="true">photos-menus.ui</file> +<file alias="help-overlay.ui" preprocess="xml-stripblanks" compressed="true">help-overlay.ui</file> This should aligned. @@ +27,3 @@ <file alias="image-filter-symbolic.svg">../data/icons/image-filter-symbolic.svg</file> <file alias="image-sharpen-symbolic.svg">../data/icons/image-sharpen-symbolic.svg</file> + Stray newline. Please remove it.
Created attachment 315424 [details] Shortcut Window[updated]
Created attachment 315427 [details] [review] Added help overlay for shortcuts with the new GtkShortcutsWindow
Created attachment 315634 [details] [review] Added help overlay for shortcuts with the new GtkShortcutsWindow https://bugzilla.gnome.org/show_bug.cgi?id=757909 Added Shortcut item in Application menu
Review of attachment 315634 [details] [review]: Nice patch. I see that you figured out the 'win.show-help-overlay' bit yourself. Good. A few comments: The new help-overlay.ui should be added to po/POTFILES.in. ::: src/help-overlay.ui @@ +93,3 @@ + <property name="visible">1</property> + <property name="title" translatable="yes">Exit from preview mode</property> + <property name="accelerator"><alt>Left</property> I think this should be labelled "Go back" because you can use this while inside a collection too. Also, to resolve the LTR vs RTL situation we need two entries - one for Alt+Left and another for Alt+Right with the direction property set on them. You will need latest gtk+ master and you can look at gnome-documents for an example. ::: src/photos-menus.ui @@ +9,3 @@ + <attribute name="action">win.show-help-overlay</attribute> + <attribute name="label" translatable="yes">Keyboard shortcuts</attribute> + </item> This should be the first item in the next section. ie. just above "help".
Created attachment 315785 [details] [review] Added help overlay for shortcuts with the new GtkShortcutsWindow
Created attachment 315788 [details] [review] Added help overlay for shortcuts with the new GtkShortcutsWindow
Review of attachment 315788 [details] [review]: Much better. ::: src/help-overlay.ui @@ +61,3 @@ + <property name="accelerator"><Primary>a</property> + </object> + </child> Search and select all only work in the 'overview' and should have their own group. @@ +67,3 @@ + <property name="title" translatable="yes">Quit</property> + <property name="accelerator"><Primary>q</property> + </object> I would put the quit at the top.
Created attachment 315851 [details] [review] Add a help overlay for shortcuts I took the liberty to fix the above issues, bumped the GTK+ requirement and pushed it to master.
Thanks for your work on this, Umang.