After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 769314 - General improvements on the new Keyboard panel
General improvements on the new Keyboard panel
Status: RESOLVED FIXED
Product: gnome-control-center
Classification: Core
Component: Keyboard
unspecified
Other All
: Normal normal
: ---
Assigned To: Rui Matos
Control-Center Maintainers
: 770948 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2016-07-29 19:21 UTC by Georges Basile Stavracas Neto
Modified: 2016-09-08 15:17 UTC
See Also:
GNOME target: 3.22
GNOME version: ---


Attachments
keyboard: change standard shortcut top label (1.16 KB, patch)
2016-07-29 19:21 UTC, Georges Basile Stavracas Neto
none Details | Review
keyboard: split current and new shortcuts in edit dialog (8.98 KB, patch)
2016-07-29 19:21 UTC, Georges Basile Stavracas Neto
none Details | Review
keyboard: make Add button insensitive after editing (1.43 KB, patch)
2016-07-29 19:21 UTC, Georges Basile Stavracas Neto
none Details | Review
keyboard: split current and new shortcuts in edit dialog (8.98 KB, patch)
2016-08-03 12:11 UTC, Georges Basile Stavracas Neto
none Details | Review
keyboard: make Add button insensitive after editing (1.43 KB, patch)
2016-08-03 13:21 UTC, Georges Basile Stavracas Neto
none Details | Review
keyboard: use a better icon to represent the reset shortcuts (1.30 KB, patch)
2016-08-03 13:21 UTC, Georges Basile Stavracas Neto
none Details | Review
keyboard: make Add button insensitive after editing (1.46 KB, patch)
2016-08-03 14:32 UTC, Georges Basile Stavracas Neto
none Details | Review
keyboard: split current and new shortcuts in edit dialog (8.45 KB, patch)
2016-08-03 14:40 UTC, Georges Basile Stavracas Neto
rejected Details | Review
keyboard: change standard shortcut top label (1.18 KB, patch)
2016-08-30 18:59 UTC, Georges Basile Stavracas Neto
none Details | Review
keyboard: make Add button insensitive after editing (1.46 KB, patch)
2016-08-30 19:02 UTC, Georges Basile Stavracas Neto
none Details | Review
keyboard: use a better icon to represent the reset shortcuts (1.30 KB, patch)
2016-08-30 19:03 UTC, Georges Basile Stavracas Neto
none Details | Review
shortcut-editor: update header title message (1.33 KB, patch)
2016-08-30 19:03 UTC, Georges Basile Stavracas Neto
none Details | Review
shortcut-editor: add 'Set' button (3.68 KB, patch)
2016-08-30 19:03 UTC, Georges Basile Stavracas Neto
none Details | Review
shortcut-editor: use states to handle headerbar mode (5.63 KB, patch)
2016-08-30 19:03 UTC, Georges Basile Stavracas Neto
none Details | Review
shortcut-editor: cancel editing on Escape (2.66 KB, patch)
2016-08-30 19:04 UTC, Georges Basile Stavracas Neto
none Details | Review
shortcuts: remove bottom label (2.08 KB, patch)
2016-08-30 19:04 UTC, Georges Basile Stavracas Neto
none Details | Review
keyboard: add enter-new-shortcut asset (19.34 KB, patch)
2016-08-30 19:04 UTC, Georges Basile Stavracas Neto
none Details | Review
shortcut-editor: use a different page to edit custom shortcuts (10.60 KB, patch)
2016-08-30 19:04 UTC, Georges Basile Stavracas Neto
none Details | Review
shortcut-editor: update reset button position and style (3.88 KB, patch)
2016-08-31 23:46 UTC, Georges Basile Stavracas Neto
none Details | Review
shortcut-editor: move widgets into a stack (6.52 KB, patch)
2016-08-31 23:56 UTC, Georges Basile Stavracas Neto
none Details | Review
shortcut-editor: move widgets into a stack (6.51 KB, patch)
2016-09-01 00:02 UTC, Georges Basile Stavracas Neto
none Details | Review
shortcut-editor: show custom page while waiting for input (5.41 KB, patch)
2016-09-01 01:18 UTC, Georges Basile Stavracas Neto
none Details | Review
keyboard: fix list width with latest Gtk+ (1.30 KB, patch)
2016-09-01 01:21 UTC, Georges Basile Stavracas Neto
none Details | Review
keyboard: change standard shortcut top label (1.18 KB, patch)
2016-09-05 14:46 UTC, Georges Basile Stavracas Neto
committed Details | Review
keyboard: make Add button insensitive after editing (1.46 KB, patch)
2016-09-05 14:46 UTC, Georges Basile Stavracas Neto
committed Details | Review
keyboard: use a better icon to represent the reset shortcuts (1.30 KB, patch)
2016-09-05 14:46 UTC, Georges Basile Stavracas Neto
accepted-commit_after_freeze Details | Review
shortcut-editor: update header title message (1.33 KB, patch)
2016-09-05 14:46 UTC, Georges Basile Stavracas Neto
committed Details | Review
shortcut-editor: add 'Set' button (3.68 KB, patch)
2016-09-05 14:47 UTC, Georges Basile Stavracas Neto
committed Details | Review
shortcut-editor: use states to handle headerbar mode (5.63 KB, patch)
2016-09-05 14:47 UTC, Georges Basile Stavracas Neto
none Details | Review
shortcut-editor: cancel editing on Escape (2.66 KB, patch)
2016-09-05 14:47 UTC, Georges Basile Stavracas Neto
committed Details | Review
shortcuts: remove bottom label (2.08 KB, patch)
2016-09-05 14:47 UTC, Georges Basile Stavracas Neto
committed Details | Review
keyboard: add enter-new-shortcut asset (19.34 KB, patch)
2016-09-05 14:48 UTC, Georges Basile Stavracas Neto
committed Details | Review
shortcut-editor: use a different page to edit custom shortcuts (10.60 KB, patch)
2016-09-05 14:48 UTC, Georges Basile Stavracas Neto
committed Details | Review
shortcut-editor: update reset button position and style (3.88 KB, patch)
2016-09-05 14:48 UTC, Georges Basile Stavracas Neto
committed Details | Review
shortcut-editor: move widgets into a stack (6.51 KB, patch)
2016-09-05 14:49 UTC, Georges Basile Stavracas Neto
committed Details | Review
shortcut-editor: show custom page while waiting for input (5.41 KB, patch)
2016-09-05 14:49 UTC, Georges Basile Stavracas Neto
committed Details | Review
keyboard: fix list width with latest Gtk+ (1.30 KB, patch)
2016-09-05 14:49 UTC, Georges Basile Stavracas Neto
none Details | Review
keyboard: fix list sizes with latest Gtk+ (1.51 KB, patch)
2016-09-06 14:29 UTC, Georges Basile Stavracas Neto
committed Details | Review
shortcut-editor: use states to handle headerbar mode (6.06 KB, patch)
2016-09-06 15:09 UTC, Georges Basile Stavracas Neto
committed Details | Review
keyboard: Remove unused variable (880 bytes, patch)
2016-09-08 12:09 UTC, Bastien Nocera
committed Details | Review
keyboard: Don't grab the mouse pointer (1.27 KB, patch)
2016-09-08 12:09 UTC, Bastien Nocera
committed Details | Review
keyboard: Don't rely on events to grab keyboard (2.31 KB, patch)
2016-09-08 12:10 UTC, Bastien Nocera
committed Details | Review
keyboard: Don't regrab the keyboard after an event (1.01 KB, patch)
2016-09-08 12:10 UTC, Bastien Nocera
committed Details | Review
keyboard: Fix grabs not working when showing the dialog (2.95 KB, patch)
2016-09-08 12:10 UTC, Bastien Nocera
committed Details | Review
keyboard: Add helper to detect empty keybindings (1.77 KB, patch)
2016-09-08 12:10 UTC, Bastien Nocera
committed Details | Review
keyboard: Don't apply "Backspace" straight away (2.19 KB, patch)
2016-09-08 12:10 UTC, Bastien Nocera
committed Details | Review

Description Georges Basile Stavracas Neto 2016-07-29 19:21:28 UTC
Per feedback, the following patches improve the current keyboard panel.
Comment 1 Georges Basile Stavracas Neto 2016-07-29 19:21:33 UTC
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.
Comment 2 Georges Basile Stavracas Neto 2016-07-29 19:21:39 UTC
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.
Comment 3 Georges Basile Stavracas Neto 2016-07-29 19:21:47 UTC
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.
Comment 4 Bastien Nocera 2016-08-02 13:43:56 UTC
Review of attachment 332366 [details] [review]:

Sure.
Comment 5 Bastien Nocera 2016-08-02 15:05:12 UTC
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.
Comment 6 Bastien Nocera 2016-08-02 15:07:45 UTC
Review of attachment 332367 [details] [review]:

What type of shortcut is this for? I can't find a way to make this appear.
Comment 7 Georges Basile Stavracas Neto 2016-08-03 12:11:30 UTC
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.
Comment 8 Georges Basile Stavracas Neto 2016-08-03 13:21:03 UTC
Created attachment 332616 [details] [review]
keyboard: make Add button insensitive after editing

Updated the commit message.
Comment 9 Georges Basile Stavracas Neto 2016-08-03 13:21:29 UTC
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.
Comment 10 Bastien Nocera 2016-08-03 14:10:48 UTC
Review of attachment 332617 [details] [review]:

Sure.
Comment 11 Bastien Nocera 2016-08-03 14:15:00 UTC
Review of attachment 332609 [details] [review]:

The shortcuts only appear after I actually pressed a key, making the dialogue resize.
It's pretty confusing.
Comment 12 Bastien Nocera 2016-08-03 14:15:41 UTC
Review of attachment 332616 [details] [review]:

> user tries to create a new shortcut right after closing,

Right after closing what?
Comment 13 Georges Basile Stavracas Neto 2016-08-03 14:32:48 UTC
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.
Comment 14 Georges Basile Stavracas Neto 2016-08-03 14:40:56 UTC
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.
Comment 15 Bastien Nocera 2016-08-03 17:21:21 UTC
The wires were updated:
https://dl.dropboxusercontent.com/u/5031519/settings/keyboard-wires.png
Comment 16 Allan Day 2016-08-08 11:49:05 UTC
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.
Comment 17 Georges Basile Stavracas Neto 2016-08-30 18:59:46 UTC
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.
Comment 18 Georges Basile Stavracas Neto 2016-08-30 19:02:31 UTC
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.
Comment 19 Georges Basile Stavracas Neto 2016-08-30 19:03:25 UTC
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.
Comment 20 Georges Basile Stavracas Neto 2016-08-30 19:03:39 UTC
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.
Comment 21 Georges Basile Stavracas Neto 2016-08-30 19:03:47 UTC
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.
Comment 22 Georges Basile Stavracas Neto 2016-08-30 19:03:55 UTC
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.
Comment 23 Georges Basile Stavracas Neto 2016-08-30 19:04:02 UTC
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.
Comment 24 Georges Basile Stavracas Neto 2016-08-30 19:04:13 UTC
Created attachment 334478 [details] [review]
shortcuts: remove bottom label
Comment 25 Georges Basile Stavracas Neto 2016-08-30 19:04:22 UTC
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.
Comment 26 Georges Basile Stavracas Neto 2016-08-30 19:04:29 UTC
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.
Comment 27 Alberts Muktupāvels 2016-08-31 11:53:49 UTC
Please look at my comment in re-opened bug:
https://bugzilla.gnome.org/show_bug.cgi?id=769063
Comment 28 Georges Basile Stavracas Neto 2016-08-31 23:46:18 UTC
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.
Comment 29 Georges Basile Stavracas Neto 2016-08-31 23:56:38 UTC
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.
Comment 30 Georges Basile Stavracas Neto 2016-09-01 00:02:51 UTC
Created attachment 334570 [details] [review]
shortcut-editor: move widgets into a stack

The name of this stack page is wrong, updated here.
Comment 31 Georges Basile Stavracas Neto 2016-09-01 01:18:25 UTC
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.
Comment 32 Georges Basile Stavracas Neto 2016-09-01 01:21:33 UTC
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.
Comment 33 Bastien Nocera 2016-09-05 11:38:29 UTC
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?
Comment 34 Georges Basile Stavracas Neto 2016-09-05 14:46:19 UTC
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.
Comment 35 Georges Basile Stavracas Neto 2016-09-05 14:46:26 UTC
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.
Comment 36 Georges Basile Stavracas Neto 2016-09-05 14:46:33 UTC
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.
Comment 37 Georges Basile Stavracas Neto 2016-09-05 14:46:40 UTC
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.
Comment 38 Georges Basile Stavracas Neto 2016-09-05 14:47:14 UTC
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.
Comment 39 Georges Basile Stavracas Neto 2016-09-05 14:47:41 UTC
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.
Comment 40 Georges Basile Stavracas Neto 2016-09-05 14:47:49 UTC
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.
Comment 41 Georges Basile Stavracas Neto 2016-09-05 14:47:56 UTC
Created attachment 334819 [details] [review]
shortcuts: remove bottom label
Comment 42 Georges Basile Stavracas Neto 2016-09-05 14:48:05 UTC
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.
Comment 43 Georges Basile Stavracas Neto 2016-09-05 14:48:15 UTC
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.
Comment 44 Georges Basile Stavracas Neto 2016-09-05 14:48:37 UTC
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.
Comment 45 Georges Basile Stavracas Neto 2016-09-05 14:49:06 UTC
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.
Comment 46 Georges Basile Stavracas Neto 2016-09-05 14:49:27 UTC
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.
Comment 47 Georges Basile Stavracas Neto 2016-09-05 14:49:54 UTC
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.
Comment 48 Georges Basile Stavracas Neto 2016-09-05 14:53:15 UTC
I reattached all the patches so they have the correct ordering. Alternatively, you can test wip/gbsneto/keyboard-improvements branch.
Comment 49 Bastien Nocera 2016-09-05 14:59:57 UTC
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.
Comment 50 Bastien Nocera 2016-09-05 15:01:26 UTC
Review of attachment 334812 [details] [review]:

Sure.
Comment 51 Bastien Nocera 2016-09-05 15:04:29 UTC
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.
Comment 52 Bastien Nocera 2016-09-05 15:05:18 UTC
Review of attachment 334813 [details] [review]:

Sure.
Comment 53 Bastien Nocera 2016-09-05 15:06:29 UTC
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.
Comment 54 Bastien Nocera 2016-09-05 15:09:13 UTC
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.
Comment 55 Bastien Nocera 2016-09-05 15:10:41 UTC
Review of attachment 334816 [details] [review]:

Sure.
Comment 56 Bastien Nocera 2016-09-05 15:14:44 UTC
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.
Comment 57 Bastien Nocera 2016-09-05 15:18:20 UTC
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?)
Comment 58 Bastien Nocera 2016-09-05 15:18:46 UTC
Review of attachment 334819 [details] [review]:

Sure.
Comment 59 Bastien Nocera 2016-09-05 15:21:15 UTC
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.
Comment 60 Bastien Nocera 2016-09-05 15:23:08 UTC
Review of attachment 334821 [details] [review]:

Looks fine.
Comment 61 Bastien Nocera 2016-09-05 15:24:04 UTC
Review of attachment 334822 [details] [review]:

Sure.
Comment 62 Bastien Nocera 2016-09-05 15:27:40 UTC
Review of attachment 334823 [details] [review]:

Sure.
Comment 63 Bastien Nocera 2016-09-05 15:28:19 UTC
Review of attachment 334824 [details] [review]:

Sure.
Comment 64 Bastien Nocera 2016-09-05 15:28:58 UTC
Review of attachment 334825 [details] [review]:

Fine. Please also bump the GTK+ required version in configure.ac for this.
Comment 65 Bastien Nocera 2016-09-05 15:43:02 UTC
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.
Comment 66 Georges Basile Stavracas Neto 2016-09-06 14:29:30 UTC
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.
Comment 67 Georges Basile Stavracas Neto 2016-09-06 15:09:31 UTC
Created attachment 334915 [details] [review]
shortcut-editor: use states to handle headerbar mode

Added the enum namespace and expanded the if condition as requested.
Comment 68 Bastien Nocera 2016-09-06 15:33:07 UTC
Review of attachment 334915 [details] [review]:

The commit log isn't fixed.
Comment 69 Bastien Nocera 2016-09-06 16:06:08 UTC
*** Bug 770948 has been marked as a duplicate of this bug. ***
Comment 70 Bastien Nocera 2016-09-06 16:07:39 UTC
Review of attachment 334913 [details] [review]:

Looks good to commit now. Might want to increase the height though.
Comment 71 Bastien Nocera 2016-09-07 12:27:01 UTC
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+
Comment 72 Bastien Nocera 2016-09-08 12:08:42 UTC
(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.
Comment 73 Bastien Nocera 2016-09-08 12:09:48 UTC
Created attachment 335094 [details] [review]
keyboard: Remove unused variable

grab_device was added in a0a15588 but unused there or since.
Comment 74 Bastien Nocera 2016-09-08 12:09:56 UTC
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.
Comment 75 Bastien Nocera 2016-09-08 12:10:04 UTC
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.
Comment 76 Bastien Nocera 2016-09-08 12:10:12 UTC
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.
Comment 77 Bastien Nocera 2016-09-08 12:10:20 UTC
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.
Comment 78 Bastien Nocera 2016-09-08 12:10:28 UTC
Created attachment 335099 [details] [review]
keyboard: Add helper to detect empty keybindings

We'll use this shortly.
Comment 79 Bastien Nocera 2016-09-08 12:10:37 UTC
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.
Comment 80 Bastien Nocera 2016-09-08 12:16:48 UTC
(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
Comment 81 Matthias Clasen 2016-09-08 12:57:20 UTC
Review of attachment 335098 [details] [review]:

maybe g_signal_connect_after on ::map would be a less hacky alternative ?
Comment 82 Bastien Nocera 2016-09-08 15:12:21 UTC
(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...
Comment 83 Bastien Nocera 2016-09-08 15:13:38 UTC
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 84 Bastien Nocera 2016-09-08 15:16:50 UTC
Comment on attachment 334814 [details] [review]
keyboard: use a better icon to represent the reset shortcuts

Already in cbff1e7b0ac86cd8d9c384d0579587b52b8f673c