GNOME Bugzilla – Bug 769314
General improvements on the new Keyboard panel
Last modified: 2016-09-08 15:17:20 UTC
Per feedback, the following patches improve the current keyboard panel.
Created attachment 332366 [details] [review] keyboard: change standard shortcut top label Per feedback, the current call-to-action label isn't very clear and is too long. Fix that by rewording the label.
Created attachment 332367 [details] [review] keyboard: split current and new shortcuts in edit dialog When editing a shortcut, the current shortcut must be displayed side-by-side with the new one. This gives a better understanding of what's going on, and may avoid mistakes. This patch splits the shortcut accelerator in 2: the current and the new one, and also adapts the code to update only the new shortcut label.
Created attachment 332368 [details] [review] keyboard: make Add button insensitive after editing When creating a new keyboard shortcut, the Add button gets sensitive when all the fields are valid. If the user immediately tries to create a new shortcut, the Add button is still sensitive even with the custom fields wrong. Fix that by making the Add button sensitive whenever we finish editing the new custom shortcut.
Review of attachment 332366 [details] [review]: Sure.
Review of attachment 332368 [details] [review]: > If the > user immediately tries to create a new shortcut, the > Add button is still sensitive even with the custom > fields wrong. When does "Immediately" mean? Immediately after what? Looks fine otherwise.
Review of attachment 332367 [details] [review]: What type of shortcut is this for? I can't find a way to make this appear.
Created attachment 332609 [details] [review] keyboard: split current and new shortcuts in edit dialog Updated the commit message. This is visible on the non-custom shortcuts page.
Created attachment 332616 [details] [review] keyboard: make Add button insensitive after editing Updated the commit message.
Created attachment 332617 [details] [review] keyboard: use a better icon to represent the reset shortcuts The current button to reset shortcuts is represented by the 'x' icon, which induces the user to think this is a remove button rather then a reset button. Fix that by using edit-clear-symbolic icon to represent the reset action.
Review of attachment 332617 [details] [review]: Sure.
Review of attachment 332609 [details] [review]: The shortcuts only appear after I actually pressed a key, making the dialogue resize. It's pretty confusing.
Review of attachment 332616 [details] [review]: > user tries to create a new shortcut right after closing, Right after closing what?
Created attachment 332624 [details] [review] keyboard: make Add button insensitive after editing When creating a new keyboard shortcut, the Add button gets sensitive when all the fields are valid. If the user tries to create a new shortcut right after closing the shortcut editor dialog, the Add button is still sensitive even with invalid custom fields. Fix that by making the Add button sensitive whenever we finish editing the new custom shortcut.
Created attachment 332625 [details] [review] keyboard: split current and new shortcuts in edit dialog When editing a standard shortcut, the current shortcut must be displayed side-by-side with the new one. This gives a better understanding of what's going on, and may avoid mistakes. This patch splits the shortcut accelerator of non-custom shortcuts in 2: the current and the new one, and also adapts the code to update only the new shortcut label.
The wires were updated: https://dl.dropboxusercontent.com/u/5031519/settings/keyboard-wires.png
The updated mockups are now in the mockups repository: https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/system-settings/keyboard/keyboard-wires.png The main problem we have at the moment is that shortcuts aren't always detected when you enter them. The new mockups attempt to help with this by either hiding any controls or making them insensitive. It might be worth experimenting to see whether this is sufficient; an alternative version could be designed where all controls are hidden, for example. Also, I think it's better to make feedback on the shortcut that has been entered the main focus of the shortcut windows; otherwise it can be a bit too much to take in and the purpose of the dialog feels a bit confused. Some other general comments about the branch: • The X icons used for the button to clear a custom shortcut makes it look like the whole shortcut will be deleted. It would be better to use edit-clear. • When a shortcut hasn't been set, it is said to be "Disabled". This isn't a good word: it implies that the shortcut has been deactivated. Better to say "Not set". • There always needs to be a heading for the custom shortcut section, even when there aren't custom shortcuts. Otherwise the + row looks like it adds a "windows" shortcut rather than a custom one.
Created attachment 334470 [details] [review] keyboard: change standard shortcut top label Per feedback, the current call-to-action label isn't very clear and is too long. Fix that by rewording the label.
Created attachment 334471 [details] [review] keyboard: make Add button insensitive after editing When creating a new keyboard shortcut, the Add button gets sensitive when all the fields are valid. If the user tries to create a new shortcut right after closing the shortcut editor dialog, the Add button is still sensitive even with invalid custom fields. Fix that by making the Add button sensitive whenever we finish editing the new custom shortcut.
Created attachment 334472 [details] [review] keyboard: use a better icon to represent the reset shortcuts The current button to reset shortcuts is represented by the 'x' icon, which induces the user to think this is a remove button rather then a reset button. Fix that by using edit-clear-symbolic icon to represent the reset action.
Created attachment 334473 [details] [review] shortcut-editor: update header title message Instead of showing the shortcut description, show an action-oriented title, according to the mockups. Precisely, "Set Shortcut" for standard shortcuts and "Set Custom Shortcut" for custom shortcuts.
Created attachment 334474 [details] [review] shortcut-editor: add 'Set' button The Set button will be used to update a standard shortcut. This patch adds it to the headerbar.
Created attachment 334476 [details] [review] shortcut-editor: use states to handle headerbar mode Instead of manually handling every button in the headerbar, using states to set it is a much better approach because it's more semantic and easier to understand. This patch adds the headerbar mode handling code, and updates the current code to use it rather than by updating each individual button manually.
Created attachment 334477 [details] [review] shortcut-editor: cancel editing on Escape When editing a standard shortcut, the current code only cancels the editing on Escape, but doesn't hide the dialog. Fix that by properly handling the canceling of shortcut editing and making sure we always hide the dialog on cancel.
Created attachment 334478 [details] [review] shortcuts: remove bottom label
Created attachment 334479 [details] [review] keyboard: add enter-new-shortcut asset This is a stub icon shown whenever we want to tell the user to start typing the new shortcut.
Created attachment 334480 [details] [review] shortcut-editor: use a different page to edit custom shortcuts When adding a new keyboard shortcut, in accordance to the mockups, the dialog should present a new page calling for an action from the user. This patch adds this page, and adapts the code to show it whenever the user wants to change the shortcut.
Please look at my comment in re-opened bug: https://bugzilla.gnome.org/show_bug.cgi?id=769063
Created attachment 334567 [details] [review] shortcut-editor: update reset button position and style The Reset button, according to the mockups, should be placed at the right side (left on rtl) of the shortcut label. Also, rather than a plain string, the button should use a symbolic icon for 'edit-clear' action. This patch moves and updates the Reset button to match the mockups.
Created attachment 334568 [details] [review] shortcut-editor: move widgets into a stack The stack will be used by the next patches to show an call for action page.
Created attachment 334570 [details] [review] shortcut-editor: move widgets into a stack The name of this stack page is wrong, updated here.
Created attachment 334575 [details] [review] shortcut-editor: show custom page while waiting for input While waiting for keyboard input, as per the new proposed mockup, the shortcut editor dialog should show a custom page with an icon that indicates the required action. The current code, however, does not expose this new customized page. Fix that by adding this new page and controlling the time when it shows and hides.
Created attachment 334576 [details] [review] keyboard: fix list width with latest Gtk+ Gtk+ changed again the behavior of the scrolled window, fixing the weird height issue but introducing a new issue where the keyboard shortcut list doesn't expand horizontally. Fortunately, this is easily solvable with the newly introduced GtkScrolledWindow:propagate-natural-width property. Fix the misbehavior by setting the new property.
A number of those patches above don't apply cleanly on master, or don't apply at all. Can you please rebase them and make sure only the ones that need to be applied are in the bug?
Created attachment 334812 [details] [review] keyboard: change standard shortcut top label Per feedback, the current call-to-action label isn't very clear and is too long. Fix that by rewording the label.
Created attachment 334813 [details] [review] keyboard: make Add button insensitive after editing When creating a new keyboard shortcut, the Add button gets sensitive when all the fields are valid. If the user tries to create a new shortcut right after closing the shortcut editor dialog, the Add button is still sensitive even with invalid custom fields. Fix that by making the Add button sensitive whenever we finish editing the new custom shortcut.
Created attachment 334814 [details] [review] keyboard: use a better icon to represent the reset shortcuts The current button to reset shortcuts is represented by the 'x' icon, which induces the user to think this is a remove button rather then a reset button. Fix that by using edit-clear-symbolic icon to represent the reset action.
Created attachment 334815 [details] [review] shortcut-editor: update header title message Instead of showing the shortcut description, show an action-oriented title, according to the mockups. Precisely, "Set Shortcut" for standard shortcuts and "Set Custom Shortcut" for custom shortcuts.
Created attachment 334816 [details] [review] shortcut-editor: add 'Set' button The Set button will be used to update a standard shortcut. This patch adds it to the headerbar.
Created attachment 334817 [details] [review] shortcut-editor: use states to handle headerbar mode Instead of manually handling every button in the headerbar, using states to set it is a much better approach because it's more semantic and easier to understand. This patch adds the headerbar mode handling code, and updates the current code to use it rather than by updating each individual button manually.
Created attachment 334818 [details] [review] shortcut-editor: cancel editing on Escape When editing a standard shortcut, the current code only cancels the editing on Escape, but doesn't hide the dialog. Fix that by properly handling the canceling of shortcut editing and making sure we always hide the dialog on cancel.
Created attachment 334819 [details] [review] shortcuts: remove bottom label
Created attachment 334820 [details] [review] keyboard: add enter-new-shortcut asset This is a stub icon shown whenever we want to tell the user to start typing the new shortcut.
Created attachment 334821 [details] [review] shortcut-editor: use a different page to edit custom shortcuts When adding a new keyboard shortcut, in accordance to the mockups, the dialog should present a new page calling for an action from the user. This patch adds this page, and adapts the code to show it whenever the user wants to change the shortcut.
Created attachment 334822 [details] [review] shortcut-editor: update reset button position and style The Reset button, according to the mockups, should be placed at the right side (left on rtl) of the shortcut label. Also, rather than a plain string, the button should use a symbolic icon for 'edit-clear' action. This patch moves and updates the Reset button to match the mockups.
Created attachment 334823 [details] [review] shortcut-editor: move widgets into a stack The stack will be used by the next patches to show an call for action page.
Created attachment 334824 [details] [review] shortcut-editor: show custom page while waiting for input While waiting for keyboard input, as per the new proposed mockup, the shortcut editor dialog should show a custom page with an icon that indicates the required action. The current code, however, does not expose this new customized page. Fix that by adding this new page and controlling the time when it shows and hides.
Created attachment 334825 [details] [review] keyboard: fix list width with latest Gtk+ Gtk+ changed again the behavior of the scrolled window, fixing the weird height issue but introducing a new issue where the keyboard shortcut list doesn't expand horizontally. Fortunately, this is easily solvable with the newly introduced GtkScrolledWindow:propagate-natural-width property. Fix the misbehavior by setting the new property.
I reattached all the patches so they have the correct ordering. Alternatively, you can test wip/gbsneto/keyboard-improvements branch.
Review of attachment 332625 [details] [review]: Looks fine. ::: panels/keyboard/cc-keyboard-shortcut-editor.c @@ -275,3 @@ accel = gtk_accelerator_name (self->custom_keyval, self->custom_mask); - Please, try to avoid whitespace changes in your patches.
Review of attachment 334812 [details] [review]: Sure.
Comment on attachment 332625 [details] [review] keyboard: split current and new shortcuts in edit dialog I'm going to assume this is obsolete, as applying it breaks patch application for more recent ones.
Review of attachment 334813 [details] [review]: Sure.
Review of attachment 334814 [details] [review]: > use a better icon to represent the reset shortcuts Either: use a better icon to represent the reset shortcuts button or: use a better icon to represent "Reset Shortcut" Looks fine otherwise.
Review of attachment 334815 [details] [review]: ::: panels/keyboard/cc-keyboard-shortcut-editor.c @@ +437,3 @@ /* Headerbar */ + gtk_header_bar_set_title (GTK_HEADER_BAR (self->headerbar), + is_custom ? _("Set Custom Shortcut") : _("Set Shortcut")); Might need a translator comment above to explain what each one is.
Review of attachment 334816 [details] [review]: Sure.
Review of attachment 334817 [details] [review]: > to set it is a much better approach because it's > more semantic ... That really doesn't mean much. Just say that it's clearer, it's a perfectly valid reason to rewrite code :) ::: panels/keyboard/cc-keyboard-shortcut-editor.c @@ +92,3 @@ + CUSTOM_CANCEL, + CUSTOM_EDIT +} HeaderMode; Please namespace those, even if the namespace is short (say, HM_NONE, etc.) to avoid conflicts with whatever system headers might add in the future. @@ +276,3 @@ } + set_header_mode (self, valid ? ADD : is_custom ? CUSTOM_CANCEL : NONE); No. Split this into different conditions.
Review of attachment 334818 [details] [review]: > Subject: [PATCH] shortcut-editor: cancel editing on Escape That's not what the patch does though, it hides the dialogue when cancelling editing (cancelling editing for a custom shortcut only it seems as well?)
Review of attachment 334819 [details] [review]: Sure.
Review of attachment 334820 [details] [review]: That icon looks like "find those 3 tabbed folders in your drawer" more than it does "press a key combination". Can you please talk to Jakub about a new icon for this? It's fine for now though.
Review of attachment 334821 [details] [review]: Looks fine.
Review of attachment 334822 [details] [review]: Sure.
Review of attachment 334823 [details] [review]: Sure.
Review of attachment 334824 [details] [review]: Sure.
Review of attachment 334825 [details] [review]: Fine. Please also bump the GTK+ required version in configure.ac for this.
Testing results: 1) Using GTK+ eaa6ca4a4920ee2146a91a20dbacc321ea55908b the window is not tall enough. It's barely tall enough to hold 6 shortcuts, making scrolling the long list difficult. 2) Keyboard capture doesn't override the shell's (eg. I tried setting the "Calculator" action to the Calculator button, which was already set as the default, it launched the Calculator instead of telling me of my "new" shortcut) 3) There's no user-visible way to unset a shortcut (not have it assigned to any keys), and "Backspace" in the capture dialogue changes the treeview underneath, but with no feedback at all in the dialogue. 4) Follow-up from the previous problem, if I use backspace when editing "Switch to next input source", it changes "Switch to previous input source" without asking me. 5) When editing the keyboard shortcut of a custom shortcut, the "Cancel" button isn't clickable, nor are any other application, or the shell. Only pressing "Esc" gets it out of there. Good job though, it's a lot of changes.
Created attachment 334913 [details] [review] keyboard: fix list sizes with latest Gtk+ Ok, since we're sticking with the old shell for now, I'm tentatively adding these default values. When we move to the new shell, we'll have to revert part of these changes.
Created attachment 334915 [details] [review] shortcut-editor: use states to handle headerbar mode Added the enum namespace and expanded the if condition as requested.
Review of attachment 334915 [details] [review]: The commit log isn't fixed.
*** Bug 770948 has been marked as a duplicate of this bug. ***
Review of attachment 334913 [details] [review]: Looks good to commit now. Might want to increase the height though.
Comment on attachment 334913 [details] [review] keyboard: fix list sizes with latest Gtk+ Attachment 334913 [details] pushed as b39bc93 - keyboard: fix list sizes with latest Gtk+
(In reply to Bastien Nocera from comment #65) > Testing results: > 1) Using GTK+ eaa6ca4a4920ee2146a91a20dbacc321ea55908b the window is not > tall enough. It's barely tall enough to hold 6 shortcuts, making scrolling > the long list difficult. Better with the patch already committed. > 2) Keyboard capture doesn't override the shell's (eg. I tried setting the > "Calculator" action to the Calculator button, which was already set as the > default, it launched the Calculator instead of telling me of my "new" > shortcut) Fixed locally. > 3) There's no user-visible way to unset a shortcut (not have it assigned to > any keys), and "Backspace" in the capture dialogue changes the treeview > underneath, but with no feedback at all in the dialogue. Fixed locally. > 4) Follow-up from the previous problem, if I use backspace when editing > "Switch to next input source", it changes "Switch to previous input source" > without asking me. This wasn't in the mockups, and couldn't get an answer at this time. I'll file a separate bug. > 5) When editing the keyboard shortcut of a custom shortcut, the "Cancel" > button isn't clickable, nor are any other application, or the shell. Only > pressing "Esc" gets it out of there. Fixed locally.
Created attachment 335094 [details] [review] keyboard: Remove unused variable grab_device was added in a0a15588 but unused there or since.
Created attachment 335095 [details] [review] keyboard: Don't grab the mouse pointer In a0a15588, we starting using a separate shortcut editor window, which was doing its own capture instead of using the GtkCellRendererAccel. But this started grabbing both keyboard and pointer, making it impossible to cancel captures using the pointer. We now only grab keyboard keys, making the pointer usable again.
Created attachment 335096 [details] [review] keyboard: Don't rely on events to grab keyboard Rather than relying on us being in the middle of processing an event to grab the keyboard, get the keyboard for the first seat of the display.
Created attachment 335097 [details] [review] keyboard: Don't regrab the keyboard after an event It was already grabbed if we received the event, so no need to grab it again.
Created attachment 335098 [details] [review] keyboard: Fix grabs not working when showing the dialog We couldn't override gnome-shell's keybindings without having a working grab, but the grab was only started when clicking the "edit" button when editing a custom shortcut, or *after* receiving the first key press event. To fix that problem, we need to grab the keyboard once we've shown the dialog itself, but not in the ->map vfunc, otherwise it will block the dialog from showing up. We set up a short timeout instead. Hopefully this isn't too fragile.
Created attachment 335099 [details] [review] keyboard: Add helper to detect empty keybindings We'll use this shortly.
Created attachment 335100 [details] [review] keyboard: Don't apply "Backspace" straight away Before, when pressing "Backspace" in the editing dialogue, the keybinding would be changed straight away, *behind* the dialogue, and the dialogue would still be expecting a new shortcut. Instead, we should make it behave like other shortcuts, which means special handling empty shortcuts as valid ones.
(In reply to Bastien Nocera from comment #72) > (In reply to Bastien Nocera from comment #65) > > 4) Follow-up from the previous problem, if I use backspace when editing > > "Switch to next input source", it changes "Switch to previous input source" > > without asking me. > > This wasn't in the mockups, and couldn't get an answer at this time. I'll > file a separate bug. https://bugzilla.gnome.org/show_bug.cgi?id=771053
Review of attachment 335098 [details] [review]: maybe g_signal_connect_after on ::map would be a less hacky alternative ?
(In reply to Matthias Clasen from comment #81) > Review of attachment 335098 [details] [review] [review]: > > maybe g_signal_connect_after on ::map would be a less hacky alternative ? I'm afraid it doesn't work...
Attachment 334812 [details] pushed as cf3ff88 - keyboard: change standard shortcut top label Attachment 334813 [details] pushed as d8d85ba - keyboard: make Add button insensitive after editing Attachment 334815 [details] pushed as 5637d57 - shortcut-editor: update header title message Attachment 334816 [details] pushed as 4729596 - shortcut-editor: add 'Set' button Attachment 334819 [details] pushed as faef035 - shortcuts: remove bottom label Attachment 334820 [details] pushed as a0001b1 - keyboard: add enter-new-shortcut asset Attachment 334821 [details] pushed as c5cd32f - shortcut-editor: use a different page to edit custom shortcuts Attachment 334822 [details] pushed as cbff1e7 - shortcut-editor: update reset button position and style Attachment 334823 [details] pushed as 9c4b273 - shortcut-editor: move widgets into a stack Attachment 334824 [details] pushed as 778395f - shortcut-editor: show custom page while waiting for input Attachment 334915 [details] pushed as 40ee225 - shortcut-editor: use states to handle headerbar mode Attachment 335094 [details] pushed as b32f58e - keyboard: Remove unused variable Attachment 335095 [details] pushed as b10a1ac - keyboard: Don't grab the mouse pointer Attachment 335096 [details] pushed as 6c7746a - keyboard: Don't rely on events to grab keyboard Attachment 335097 [details] pushed as 141441e - keyboard: Don't regrab the keyboard after an event Attachment 335098 [details] pushed as 58b175f - keyboard: Fix grabs not working when showing the dialog Attachment 335099 [details] pushed as 69258a9 - keyboard: Add helper to detect empty keybindings Attachment 335100 [details] pushed as 784d8f8 - keyboard: Don't apply "Backspace" straight away
Comment on attachment 334814 [details] [review] keyboard: use a better icon to represent the reset shortcuts Already in cbff1e7b0ac86cd8d9c384d0579587b52b8f673c