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

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

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.

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()?

(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

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

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

Review of attachment 344640 [details] [review]:

Looks good.

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.

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

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