GNOME Bugzilla – Bug 777845
"Edit" button should be backspace icon
Last modified: 2017-01-30 15:27:23 UTC
For new custom shortcuts, as per: https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/system-settings/keyboard/keyboard-wires.png
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.
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.
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).
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.
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
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
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.
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
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
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
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
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.
Review of attachment 344538 [details] [review]: ++
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
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
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.
Review of attachment 344545 [details] [review]: Looks good
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)
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