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 777845 - "Edit" button should be backspace icon
"Edit" button should be backspace icon
Status: RESOLVED FIXED
Product: gnome-control-center
Classification: Core
Component: Keyboard
3.22.x
Other Linux
: Normal normal
: ---
Assigned To: Rui Matos
Control-Center Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-01-27 16:13 UTC by Bastien Nocera
Modified: 2017-01-30 15:27 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
shortcut-editor: switch to the reset button (12.55 KB, patch)
2017-01-30 10:23 UTC, Georges Basile Stavracas Neto
needs-work Details | Review
shortcut-editor: fix editing of custom shortcuts (3.51 KB, patch)
2017-01-30 10:23 UTC, Georges Basile Stavracas Neto
needs-work Details | Review
keyboard: switch to the reset button (12.66 KB, patch)
2017-01-30 10:35 UTC, Georges Basile Stavracas Neto
none Details | Review
keyboard: fix editing of custom shortcuts (3.63 KB, patch)
2017-01-30 10:35 UTC, Georges Basile Stavracas Neto
none Details | Review
keyboard: switch to the reset button (12.55 KB, patch)
2017-01-30 10:48 UTC, Georges Basile Stavracas Neto
none Details | Review
keyboard: fix editing of custom shortcuts (3.61 KB, patch)
2017-01-30 10:48 UTC, Georges Basile Stavracas Neto
none Details | Review
keyboard: switch to the reset button (12.46 KB, patch)
2017-01-30 13:00 UTC, Georges Basile Stavracas Neto
none Details | Review
keyboard: fix editing of custom shortcuts (3.61 KB, patch)
2017-01-30 13:01 UTC, Georges Basile Stavracas Neto
none Details | Review
keyboard: manage shortcut editor state with pages (5.84 KB, patch)
2017-01-30 13:01 UTC, Georges Basile Stavracas Neto
none Details | Review
keyboard: switch to the reset button (12.45 KB, patch)
2017-01-30 13:41 UTC, Georges Basile Stavracas Neto
committed Details | Review
keyboard: fix editing of custom shortcuts (3.59 KB, patch)
2017-01-30 13:42 UTC, Georges Basile Stavracas Neto
committed Details | Review
keyboard: manage shortcut editor state with pages (5.84 KB, patch)
2017-01-30 13:43 UTC, Georges Basile Stavracas Neto
committed Details | Review

Comment 1 Georges Basile Stavracas Neto 2017-01-30 10:23:15 UTC
Created attachment 344521 [details] [review]
shortcut-editor: switch to the reset button

Per the mockups, when editing a custom shortcut, there is
a reset button right after the current shortcut. When there
is no shortcut set, it shows a "Set Shortcut" button instead.
The current flow to edit a shortcut, however, diverges from
the proposed ones since we use an "Edit" button instead.

Fix that by swapping the Edit button with a reset button, and
adapt the code to not depend on the edit button anymore.
Comment 2 Georges Basile Stavracas Neto 2017-01-30 10:23:22 UTC
Created attachment 344522 [details] [review]
shortcut-editor: fix editing of custom shortcuts

After introducing the reset button to match the mockups, the
shortcut editor dialog had some issues exposed. This is visible
e.g. when the user tries to edit a custom shortcut's name and
the shortcut is disabled.

This happens because we assume there is always a shortcut set.
When we open the dialog to edit a custom shortcut, however, nothing
is actually set, and we end up saving the disabled shortcut when
editing the shortcut's name or command.

Fix that by assuming the shortcut's accelerators when editing a
shortcut, and correcting the logic to validate the shortcut.
Comment 3 Bastien Nocera 2017-01-30 10:32:21 UTC
Review of attachment 344521 [details] [review]:

> shortcut-editor: switch to the reset button

Same thing about the prefix

> Per the mockups, when editing a custom shortcut, there is

Link to mockups.

::: panels/keyboard/cc-keyboard-shortcut-editor.c
@@ -324,3 @@
   accel = gtk_accelerator_name (self->custom_keyval, self->custom_mask);
 
-

whitespace changes in an unrelated area, please revert.

@@ +551,3 @@
+       * "Set Shortcut" button.
+       */
+      is_accel_empty = !accel || accel[0] == '\0';

|| *accel == '\0';

@@ +646,2 @@
+  /*
+   * Being in the "change-shortcut" page is the only check we must

No empty lines in comments (this comment should be 2 lines).
Comment 4 Bastien Nocera 2017-01-30 10:33:56 UTC
Review of attachment 344522 [details] [review]:

> Fix that by assuming the shortcut's accelerators when editing a
> shortcut

Assuming what about the accelerators?

::: panels/keyboard/cc-keyboard-shortcut-editor.c
@@ +294,3 @@
           gtk_stack_set_visible_child_name (GTK_STACK (self->stack), "custom");
+
+          /*

No empty lines in comments.

@@ +514,3 @@
   accel = gtk_accelerator_name (item->keyval, item->mask);
 
+  /*

Ditto.
Comment 5 Georges Basile Stavracas Neto 2017-01-30 10:35:23 UTC
Created attachment 344524 [details] [review]
keyboard: switch to the reset button

Per the mockups [1], when editing a custom shortcut, there is
a reset button right after the current shortcut. When there
is no shortcut set, it shows a "Set Shortcut" button instead.
The current UI to edit a shortcut, however, diverges from
the proposed one since we use an "Edit" button instead.

Fix that by swapping the Edit button with a reset button, and
adapt the code to not depend on the edit button anymore.

[1] https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/system-settings/keyboard/keyboard-wires.png
Comment 6 Georges Basile Stavracas Neto 2017-01-30 10:35:29 UTC
Created attachment 344525 [details] [review]
keyboard: fix editing of custom shortcuts

After introducing the reset button to match the mockups [1], the
shortcut editor dialog had some issues exposed. This is visible
e.g. when the user tries to edit a custom shortcut's name and
the shortcut is disabled.

This happens because we assume there is always a shortcut set.
When we open the dialog to edit a custom shortcut, however, nothing
is actually set, and we end up saving the disabled shortcut when
editing the shortcut's name or command.

Fix that by assuming the shortcut's accelerators when editing a
shortcut, and correcting the logic to validate the shortcut.

[1] https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/system-settings/keyboard/keyboard-wires.png
Comment 7 Bastien Nocera 2017-01-30 10:45:35 UTC
Review of attachment 344524 [details] [review]:

::: panels/keyboard/cc-keyboard-shortcut-editor.c
@@ +547,3 @@
       gtk_widget_set_sensitive (self->command_entry, item->cmd_editable);
 
+      /*

Comment empty lines.

@@ +643,2 @@
   self = CC_KEYBOARD_SHORTCUT_EDITOR (widget);
+  is_custom = g_str_equal (gtk_stack_get_visible_child_name (GTK_STACK (self->stack)), "custom");

This is pretty ugly.

@@ +646,1 @@
+  /*

Empty lines.

@@ +649,3 @@
+   */
+  if (is_custom)
+    editing = g_str_equal (gtk_stack_get_visible_child_name (GTK_STACK (self->stack)), "change-shortcut");

This is ugly as well.
Comment 8 Georges Basile Stavracas Neto 2017-01-30 10:48:09 UTC
Created attachment 344526 [details] [review]
keyboard: switch to the reset button

Per the mockups [1], when editing a custom shortcut, there is
a reset button right after the current shortcut. When there
is no shortcut set, it shows a "Set Shortcut" button instead.
The current UI to edit a shortcut, however, diverges from
the proposed one since we use an "Edit" button instead.

Fix that by swapping the Edit button with a reset button, and
adapt the code to not depend on the edit button anymore.

[1] https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/system-settings/keyboard/keyboard-wires.png
Comment 9 Georges Basile Stavracas Neto 2017-01-30 10:48:14 UTC
Created attachment 344527 [details] [review]
keyboard: fix editing of custom shortcuts

After introducing the reset button to match the mockups [1], the
shortcut editor dialog had some issues exposed. This is visible
e.g. when the user tries to edit a custom shortcut's name and
the shortcut is disabled.

This happens because we assume there is always a shortcut set.
When we open the dialog to edit a custom shortcut, however, nothing
is actually set, and we end up saving the disabled shortcut when
editing the shortcut's name or command.

Fix that by assuming the shortcut's accelerators when editing a
shortcut, and correcting the logic to validate the shortcut.

[1] https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/system-settings/keyboard/keyboard-wires.png
Comment 10 Georges Basile Stavracas Neto 2017-01-30 13:00:59 UTC
Created attachment 344536 [details] [review]
keyboard: switch to the reset button

Per the mockups [1], when editing a custom shortcut, there is
a reset button right after the current shortcut. When there
is no shortcut set, it shows a "Set Shortcut" button instead.
The current UI to edit a shortcut, however, diverges from
the proposed one since we use an "Edit" button instead.

Fix that by swapping the Edit button with a reset button, and
adapt the code to not depend on the edit button anymore.

[1] https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/system-settings/keyboard/keyboard-wires.png
Comment 11 Georges Basile Stavracas Neto 2017-01-30 13:01:06 UTC
Created attachment 344537 [details] [review]
keyboard: fix editing of custom shortcuts

After introducing the reset button to match the mockups [1], the
shortcut editor dialog had some issues exposed. This is visible
e.g. when the user tries to edit a custom shortcut's name and
the shortcut is disabled.

This happens because we assume there is always a shortcut set.
When we open the dialog to edit a custom shortcut, however, nothing
is actually set, and we end up saving the disabled shortcut when
editing the shortcut's name or command.

Fix that by assuming the shortcut's accelerators when editing a
shortcut, and correcting the logic to validate the shortcut.

[1] https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/system-settings/keyboard/keyboard-wires.png
Comment 12 Georges Basile Stavracas Neto 2017-01-30 13:01:13 UTC
Created attachment 344538 [details] [review]
keyboard: manage shortcut editor state with pages

The current shortcut editor state is managed by setting and
comparing the page name directly, making the code look more
complicated than it should.

Fix this by introducing the concept of pages, and using this
to set and get the current shortcut editor dialog state.
Comment 13 Bastien Nocera 2017-01-30 13:12:15 UTC
Review of attachment 344538 [details] [review]:

++
Comment 14 Georges Basile Stavracas Neto 2017-01-30 13:41:28 UTC
Created attachment 344545 [details] [review]
keyboard: switch to the reset button

Per the mockups [1], when editing a custom shortcut, there is
a reset button right after the current shortcut. When there
is no shortcut set, it shows a "Set Shortcut" button instead.
The current UI to edit a shortcut, however, diverges from
the proposed one since we use an "Edit" button instead.

Fix that by swapping the Edit button with a reset button, and
adapt the code to not depend on the edit button anymore.

[1] https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/system-settings/keyboard/keyboard-wires.png
Comment 15 Georges Basile Stavracas Neto 2017-01-30 13:42:14 UTC
Created attachment 344546 [details] [review]
keyboard: fix editing of custom shortcuts

After introducing the reset button to match the mockups [1], the
shortcut editor dialog had some issues exposed. This is visible
e.g. when the user tries to edit a custom shortcut's name and
the shortcut is disabled.

This happens because we assume there is always a shortcut set.
When we open the dialog to edit a custom shortcut, however, nothing
is actually set, and we end up saving the disabled shortcut when
editing the shortcut's name or command.

Fix that by assuming the shortcut's accelerators when editing a
shortcut, and correcting the logic to validate the shortcut.

[1] https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/system-settings/keyboard/keyboard-wires.png
Comment 16 Georges Basile Stavracas Neto 2017-01-30 13:43:07 UTC
Created attachment 344547 [details] [review]
keyboard: manage shortcut editor state with pages

The current shortcut editor state is managed by setting and
comparing the page name directly, making the code look more
complicated than it should.

Fix this by introducing the concept of pages, and using this
to set and get the current shortcut editor dialog state.
Comment 17 Bastien Nocera 2017-01-30 15:10:01 UTC
Review of attachment 344545 [details] [review]:

Looks good
Comment 18 Bastien Nocera 2017-01-30 15:12:45 UTC
Review of attachment 344546 [details] [review]:

> Fix that by assuming the shortcut's accelerators when editing a
> shortcut, and correcting the logic to validate the shortcut.

s/assuming/initialising/

Looks good otherwise.

::: panels/keyboard/cc-keyboard-shortcut-editor.c
@@ +513,3 @@
   accel = gtk_accelerator_name (item->keyval, item->mask);
 
+  /* To avoid accidentaly thinking we unset the current keybinding, set the values

accidentally (2 l's)
Comment 19 Georges Basile Stavracas Neto 2017-01-30 15:27:07 UTC
Attachment 344545 [details] pushed as 20f52da - keyboard: switch to the reset button
Attachment 344546 [details] pushed as 147a1e8 - keyboard: fix editing of custom shortcuts
Attachment 344547 [details] pushed as 1e13e97 - keyboard: manage shortcut editor state with pages