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 777840 - Add "Reset All" button
Add "Reset All" button
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:06 UTC by Bastien Nocera
Modified: 2017-01-31 11:58 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
keyboard: put the shortcut list inside a box (4.96 KB, patch)
2017-01-30 13:35 UTC, Georges Basile Stavracas Neto
needs-work Details | Review
keyboard: add a reset all button (4.13 KB, patch)
2017-01-30 13:35 UTC, Georges Basile Stavracas Neto
needs-work Details | Review
keyboard: make the entire panel scrollable (5.06 KB, patch)
2017-01-31 11:00 UTC, Georges Basile Stavracas Neto
committed Details | Review
keyboard: add a reset all button (6.04 KB, patch)
2017-01-31 11:00 UTC, Georges Basile Stavracas Neto
none Details | Review
keyboard: add a reset all button (6.18 KB, patch)
2017-01-31 11:56 UTC, Georges Basile Stavracas Neto
committed Details | Review

Comment 1 Georges Basile Stavracas Neto 2017-01-30 13:35:32 UTC
Created attachment 344543 [details] [review]
keyboard: put the shortcut list inside a box

This commit puts the main shortcut list inside a box, so that
in the future we can add more widgets like the "Reset All" button
and the "Alternate Characters Key" row as the mockups [1] propose.

[1] https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/system-settings/keyboard/keyboard-wires.png
Comment 2 Georges Basile Stavracas Neto 2017-01-30 13:35:38 UTC
Created attachment 344544 [details] [review]
keyboard: add a reset all button

As described in the proposed mockups [1], the Keyboard panel
should have a Reset All button above the list of shortcuts that
allows the user to quickly reset all the shortcuts to their
default keybinding. The current implementation, however, lacks
this button.

Fix that by adding a "Reset All" button, and implementing the
reset all action.

[1] https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/system-settings/keyboard/keyboard-wires.png
Comment 3 Bastien Nocera 2017-01-30 14:25:42 UTC
Review of attachment 344543 [details] [review]:

::: panels/keyboard/gnome-keyboard-panel.ui
@@ +45,2 @@
             <child>
+              <object class="GtkScrolledWindow">

I'm pretty sure that the whole panel should be scrollable, not just the list box with the screenshots. We want the filtered view to not show the "Alternate Character keys" box for example.
Comment 4 Bastien Nocera 2017-01-30 14:27:15 UTC
Review of attachment 344544 [details] [review]:

::: panels/keyboard/cc-keyboard-panel.c
@@ +147,2 @@
 static void
+reset_all_clicked_cb (CcKeyboardPanel *self)

This might need a confirmation dialogue, need to ask Allan.

@@ +150,3 @@
+  GList *children, *l;
+
+  children = gtk_container_get_children (GTK_CONTAINER (self->listbox));

Any reason why you can't use gtk_container_foreach()?
Comment 5 Bastien Nocera 2017-01-30 14:33:19 UTC
(In reply to Bastien Nocera from comment #4)
> Review of attachment 344544 [details] [review] [review]:
> 
> ::: panels/keyboard/cc-keyboard-panel.c
> @@ +147,2 @@
>  static void
> +reset_all_clicked_cb (CcKeyboardPanel *self)
> 
> This might need a confirmation dialogue, need to ask Allan.

It would need a confirmation dialogue, or an in-app notification with undo. Given how complicated the latter would be, a confirmation dialogue would be enough for now.

Please make sure that the confirmation button on the dialogue is styled properly (red), and that only the "builtin" shortcuts are reset, not the custom shortcuts. Custom shortcuts should however be reset if they conflict with restored keybindings[1].

[1]: For example:
- Set "Calculator" to "Alt+Shift+C"
- Set a custom shortcut to launch a terminal to "Calculator"
- Click "Reset All"
- the Calculator keybinding should be reassigned to the Calculator key, and the custom shortcut blank
Comment 6 Georges Basile Stavracas Neto 2017-01-31 11:00:12 UTC
Created attachment 344640 [details] [review]
keyboard: make the entire panel scrollable

The current Keyboard panel allows only the shortcut list to scroll.
Since we want to add more widgets above the list, and we want them
to scroll out of sight per the latest mockups [1], making only the
shortcut list scrollable poses a problem.

Fix that by making the entire panel scrollable.

[1] https://github.com/gnome-design-team/gnome-mockups/blob/master/system-settings/keyboard/keyboard-wires.png
Comment 7 Georges Basile Stavracas Neto 2017-01-31 11:00:19 UTC
Created attachment 344641 [details] [review]
keyboard: add a reset all button

As described in the proposed mockups [1], the Keyboard panel
should have a Reset All button above the list of shortcuts that
allows the user to quickly reset all the shortcuts to their
default keybinding. The current implementation, however, lacks
this button.

Fix that by adding a "Reset All" button, and implementing the
reset all action. A message dialog is shown in order to confirm
the action, and custom shortcuts are not reset (unless the conflict
with the default keybinding of another standard shortcut).

[1] https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/system-settings/keyboard/keyboard-wires.png
Comment 8 Bastien Nocera 2017-01-31 11:25:18 UTC
Review of attachment 344640 [details] [review]:

Looks good.
Comment 9 Bastien Nocera 2017-01-31 11:27:11 UTC
Review of attachment 344641 [details] [review]:

Looks good otherwise.

::: panels/keyboard/cc-keyboard-panel.c
@@ +154,3 @@
+  self = user_data;
+
+  if (widget == (GtkWidget*) self->add_shortcut_row)

Space before "*"

@@ +182,3 @@
+
+  gtk_message_dialog_format_secondary_text (GTK_MESSAGE_DIALOG (dialog),
+                                            _("Once finished, this action cannot be undone."));

Please check with Allan for the exact wording for this.
Comment 10 Georges Basile Stavracas Neto 2017-01-31 11:56:54 UTC
Created attachment 344645 [details] [review]
keyboard: add a reset all button

As described in the proposed mockups [1], the Keyboard panel
should have a Reset All button above the list of shortcuts that
allows the user to quickly reset all the shortcuts to their
default keybinding. The current implementation, however, lacks
this button.

Fix that by adding a "Reset All" button, and implementing the
reset all action. A message dialog is shown in order to confirm
the action, and custom shortcuts are not reset (unless the conflict
with the default keybinding of another standard shortcut).

[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-31 11:58:26 UTC
Thanks for the reviews.

Attachment 344640 [details] pushed as 8355521 - keyboard: make the entire panel scrollable
Attachment 344645 [details] pushed as 8180249 - keyboard: add a reset all button