It would be wonderful if Jakub could come up with a fast sketch of what this assignation should look. As soon as I get it, I will implement this.

Adding ui-review.

Created attachment 247461 [details] [review]
wacom: Use the OSD window to edit the tablets' buttons

This adds a new edition mode to the OSD window and two new widgets:
a button editor and the key shortcut button.

It is a big patch because it's a big change.

Hi,

Here is a video showing the edition of buttons:
https://docs.google.com/file/d/0B9mCgEAuiw_IazFtOGdpWm9Zc00/edit?usp=sharing

(BTW, I moved the "old" configure button to the top when on edition mode but comments about this are welcome)

Wow, good work. The only thing I don't like is how the dialog remains open after the assignment (the editing itself is a modal). It would be nicer for the assignment to happen in spatial relation to the button.

As soon as you'd hit the button, along with the nice prelight, the label "None" would become a dropdown and the optional additional control in line on the right of it. I would either add a timer to allow correcting the shortcut or add an inline done button (that's different to the 'done editing' global one at the bottom).

Hi Jakub,

About the modal, I can add a timeout as you say, and hide the editor widget if it remains untouched.

I'll try to put the action combo on top of the buttons' label but it'd be nice that you tried to sketch it because we got two widgets: the combo and the shortcut button. While this is not something complicated, we'll need to check how it looks when the OSD's control aren't on the left (i.e. the tablet is rotated).

Another thing that needs to be given some thought is the old button's assignation. I think that, ideally, when one has no button assigned to the help window, we could have the "Configure Buttons" button in the g-c-c page call the OSD window already in edition mode but the OSD belongs to g-s-d so I don't know how we can easily call it.

We'd only keep the old assignation code for tablet's that don't have a known layout (no OSD?).

I'll do my best to mock these up ASAP. 

As for the old assignment UI, we should really feel ashamed when we fall back to that. :) But yea, it's reasonable to have a fall back if we don't know the model.

Default OSD view — https://raw.github.com/gnome-design-team/gnome-mockups/master/system-settings/tablets/osd-cintiq-21UX-view.png

Edit button is a toggle:
https://raw.github.com/gnome-design-team/gnome-mockups/master/system-settings/tablets/osd-cintiq-21UX-edit-0.png

Pushing tablet button will fade out all other controls and provide a dropdown to select action. Hitting Esc or toggling the edit button will cancel the new assignment:
https://raw.github.com/gnome-design-team/gnome-mockups/master/system-settings/tablets/osd-cintiq-21UX-edit-1.png

Selecting 'send keystroke' will immediately foc the assignment entry and start listening to shortcuts:
https://raw.github.com/gnome-design-team/gnome-mockups/master/system-settings/tablets/osd-cintiq-21UX-edit-2.png

Right hand side buttons are a little eeky as the assignment will make labels/controls jump around, but still better than left aligning I think:
https://raw.github.com/gnome-design-team/gnome-mockups/master/system-settings/tablets/osd-cintiq-21UX-edit-3r.png

Created attachment 249166 [details] [review]
wacom: Add libgd as a git submodule

It will be necessary for the Wacom plugin which will use the GdKeyShortcutButton.

I'm adding this patch because I don't know if the GtkKeyShortcutButton will be used of libgd's.

Created attachment 249167 [details] [review]
wacom: Include and link libgd when linking

Created attachment 249169 [details] [review]
wacom: Use the OSD window to edit the tablets' buttons

This adds a new edition mode to the OSD window and a new widget to edit the tablets' buttons.

Apart from eventual minor cosmetic changes, this should implement the behavior that Jakub has sketched up.

Created attachment 249170 [details] [review]
wacom: Move the OSD related key release processing to the GsdWacomOSDWindow

Instead of doing it in the GsdWacomManager; this way, it gets more modular and
makes it easier for the OSD to process the keys.
Also changes what some keys do.

This is an important patch to go on top of 249169. I separated it because it deals only with how keys are processed.

Created attachment 249178 [details] [review]
wacom: Use the OSD window to edit the tablets' buttons

This adds a new edition mode to the OSD window and a new widget to edit the tablets' buttons.

(This new version removes the "mapped" function which had mistakenly been left in when rebasing with master)

Review of attachment 249166 [details] [review]:

As mentioned on IRC, I'd rather whatever we needed in libgd (apparently just one widget) was in g-s-d directly.

Review of attachment 249167 [details] [review]:

As above.

Review of attachment 249170 [details] [review]:

::: plugins/wacom/gsd-wacom-manager.c
@@ +974,3 @@
 	/* Connect some signals to the OSD window */
+	g_signal_connect (widget, "destroy",
+			  G_CALLBACK(on_osd_window_destroyed), manager);

Or simply:
g_object_add_weak_pointer (G_OBJECT (widget), &manager->priv->osd_window);
Presumably destroying the window also unref's it, right?

::: plugins/wacom/gsd-wacom-osd-window.c
@@ +1589,3 @@
+	osd_window = GSD_WACOM_OSD_WINDOW (widget);
+
+	if (event->type != GDK_KEY_RELEASE)

This isn't an event handler, you need to chain up.
if (event->...)
  goto out;

and then in out: call the parent vfunc.

Review of attachment 249178 [details] [review]:

::: plugins/wacom/gsd-wacom-button-editor.c
@@ +1,2 @@
+/*
+ * Copyright © 2013 Red Hat, Inc.

Bonus points for UTF-8!

@@ +27,3 @@
+#include "gsd-wacom-button-editor.h"
+
+#define BACK_OPACITY                .8

0.8 please.

@@ +113,3 @@
+
+static void
+change_button_action_type (GsdWacomButtonEditor *self,

It looks like a lot of that code is the same as gnome-control-center but doesn't do thing like setting the OLED label.
Can that code be shared? If we're going to remove the button editing from gnome-control-center (to only use the OSD) then make sure that bug fixes are getting propagated before removing the g-c-c code.

::: plugins/wacom/gsd-wacom-key-shortcut-button.c
@@ +29,3 @@
+#define GSD_WACOM_KEY_SHORTCUT_BUTTON_GET_PRIVATE(obj) (G_TYPE_INSTANCE_GET_PRIVATE ((obj), GSD_WACOM_KEY_SHORTCUT_BUTTON_TYPE, GsdWacomKeyShortcutButtonPrivate))
+
+G_DEFINE_TYPE (GsdWacomKeyShortcutButton, gsd_wacom_key_shortcut_button, GTK_TYPE_BUTTON);

Is this the button that would have been in libgd?

::: plugins/wacom/gsd-wacom-manager.c
@@ +923,3 @@
+
+	/* If it's in edition mode, we don't destroy the window */
+	g_object_get (widget, "edition-mode", &editing_mode, NULL);

Add an accessor for edition-mode to the window object directly.

::: plugins/wacom/gsd-wacom-osd-window.c
@@ +1245,3 @@
+	gchar       *message;
+
+	if (osd_window->priv->pad == NULL)

How come?

@@ +1260,3 @@
+	gchar       *message;
+
+	message = g_strdup_printf ("<big><b>%s</b></big>\n<span foreground=\"%s\">%s</span>",

return g_strdup_printf ()...

@@ +1922,3 @@
 	osd_window->priv = GSD_WACOM_OSD_WINDOW_GET_PRIVATE (osd_window);
 	osd_window->priv->cursor_timeout = 0;
+	gtk_widget_add_events (GTK_WIDGET (osd_window), GDK_ALL_EVENTS_MASK);

You don't need all events to capture keyboard events.

Created attachment 249306 [details] [review]
wacom: Use the OSD window to edit the tablets' buttons

New version after Bastien's comments.

Created attachment 249307 [details] [review]
wacom: Move the OSD related key release processing to the GsdWacomOSDWindow

... and the same for the keys commit.

Created attachment 249420 [details] [review]
wacom: Use the OSD window to edit the tablets' buttons

New version after removing the old configure button that called g-c-c. Jakub explained to me that it is no longer necessary.

Created attachment 249592 [details]
Screenshot #0

Created attachment 249593 [details]
Screenshot #1

Created attachment 249594 [details]
Screenshot #2

Series of screenshots with the latest implementation of this feature.

Review of attachment 249307 [details] [review]:

::: plugins/wacom/gsd-wacom-osd-window.c
@@ +1563,3 @@
+	osd_window = GSD_WACOM_OSD_WINDOW (widget);
+
+	if (event->type != GDK_KEY_RELEASE)

You don't need that, by definition, ->key_release_event will only receive key release events :)

@@ +1567,3 @@
+
+	if (osd_window->priv->edition_mode) {
+		if (event->keyval == GDK_KEY_Escape && !gtk_widget_get_visible (osd_window->priv->editor))

Split on two lines after the &&

@@ +1568,3 @@
+	if (osd_window->priv->edition_mode) {
+		if (event->keyval == GDK_KEY_Escape && !gtk_widget_get_visible (osd_window->priv->editor))
+			gtk_toggle_button_set_active (GTK_TOGGLE_BUTTON (osd_window->priv->change_mode_button),

One line for that.

Review of attachment 249420 [details] [review]:

Will need to review the code still, that's a drive-by comment.

::: plugins/wacom/gsd-wacom-osd-window.c
@@ +1325,3 @@
+{
+	return g_strdup_printf ("<big><b>%s</b></big>\n<span foreground=\"%s\">%s</span>",
+				_("Push a button to configure"), INACTIVE_COLOR, _("(Esc to cancel)"));

Push? I would like to make it clear that the physical button on the tablet needs to be pressed to edit it. For a little while, I thought you meant clicking on the buttons in the OSD with a mouse (or stylus).

Review of attachment 249420 [details] [review]:

::: plugins/wacom/gsd-wacom-button-editor.c
@@ +74,3 @@
+  GtkCellView     *cell_view;
+  GtkCellRenderer *renderer;
+} ActivateCellAreaHelper;

That's not use anymore, is it?

@@ +92,3 @@
+  strv = g_settings_get_strv (button->settings, CUSTOM_ELEVATOR_ACTION_KEY);
+
+  if (strv != NULL)

That can never happen.

@@ +109,3 @@
+                       (const gchar * const*) strs);
+
+  g_clear_pointer (&strv, g_strfreev);

strv cannot be null

@@ +190,3 @@
+reset_shortcut_button_label (GsdWacomButtonEditor *self)
+{
+  gtk_button_set_label (GTK_BUTTON (self->priv->shortcut_button), _("None"));

This will need a context.

@@ +243,3 @@
+      strs[2] = NULL;
+      strs[0] = strs[1] = "";
+      strv = g_settings_get_strv (button->settings, CUSTOM_ELEVATOR_ACTION_KEY);

Can never be NULL.

@@ +313,3 @@
+  for (i = 0; i < G_N_ELEMENTS (action_table); i++)
+    {
+      if ((button->type == WACOM_TABLET_BUTTON_TYPE_STRIP ||

This really isn't readable, please split it up.

::: plugins/wacom/gsd-wacom-key-shortcut-button.c
@@ +27,3 @@
+
+/**
+ * SECTION:gtkkeyshortcutbutton

It's not gtk.

@@ +168,3 @@
+{
+  GsdWacomKeyShortcutButtonPrivate *priv;
+  GdkDevice *device, *keyb, *pointer;

*kbd;

@@ +189,3 @@
+    return;
+
+  if (gdk_device_get_source (device) == GDK_SOURCE_KEYBOARD)

All that event code isn't needed. Get the keyboard and pointer by looping over the devices from the display manager.

@@ +328,3 @@
+    }
+
+  tmp_event = priv->tmp_event;

I really don't understand what tmp_event is for.

@@ +340,3 @@
+
+  g_signal_emit (self, signals[KEY_SHORTCUT_EDITED], 0);
+

Extraneous line feed.

::: plugins/wacom/gsd-wacom-osd-window.c
@@ +828,3 @@
+		if (osd_button->priv->active) {
+			color_str = gsd_wacom_osd_button_get_color_str (osd_button);
+			buttons_section = g_strconcat (buttons_section,

g_strdup_printf would probably have been cleaner.

@@ +841,3 @@
+		} else if (osd_window_editing_button (osd_window) &&
+			   osd_button != osd_window->priv->current_button) {
+			buttons_section = g_strconcat (buttons_section,

Ditto.

@@ +1758,3 @@
+		gtk_widget_hide (priv->editor);
+
+		gsd_wacom_button_editor_set_button (GSD_WACOM_BUTTON_EDITOR (priv->editor), button, dir);

Remove the linefeeds above and below.

@@ +1809,3 @@
+						gtk_widget_hide (osd_window->priv->editor);
+
+						gsd_wacom_button_editor_set_button (GSD_WACOM_BUTTON_EDITOR (osd_window->priv->editor), tablet_button, dir);

Ditto.

Created attachment 249821 [details] [review]
wacom: Use the OSD window to edit the tablets' buttons

New version.

Created attachment 249822 [details] [review]
wacom: Use the OSD window to edit the tablets' buttons

Actually this one's better :)

Review of attachment 249821 [details] [review]:

It's just minor niggles now, apart from the tmp_event stuff which I still don't understand.

::: plugins/wacom/gsd-wacom-key-shortcut-button.c
@@ +194,3 @@
+      current_device = l->data;
+      if (!kbd && gdk_device_get_source (current_device) == GDK_SOURCE_KEYBOARD)
+        {

Remove the brace for one-line conditionals.

@@ +198,3 @@
+        }
+      else if (!pointer && gdk_device_get_source (current_device) == GDK_SOURCE_MOUSE)
+        {

Ditto.

@@ +335,3 @@
+    }
+
+  tmp_event = priv->tmp_event;

I still don't understand the tmp_event code.

::: plugins/wacom/gsd-wacom-osd-window.c
@@ +820,3 @@
 	buttons_section = g_strdup ("");
 	for (l = osd_window->priv->buttons; l != NULL; l = l->next) {
+		gchar *color_str, *css;

const char *css;

Created attachment 249827 [details] [review]
wacom: Use the OSD window to edit the tablets' buttons

Hope we're there now ;)

Review of attachment 249827 [details] [review]:

::: plugins/wacom/gsd-wacom-key-shortcut-button.c
@@ +428,3 @@
+       * So, we keep track (with tmp_event) of the pressed keys if they're modifiers
+       * and so far and assign them when a key-release happens */
+      priv->tmp_event = (GdkEventKey *) gdk_event_copy ((GdkEvent *) event);

Seeing as you only use a few of the fields from the event, could you please copy those fields instead of the whole event?
It will make it easier to see which fields are of importance, and which are completely ignored.

Created attachment 249895 [details] [review]
wacom: Use the OSD window to edit the tablets' buttons

New version.

Review of attachment 249895 [details] [review]:

::: plugins/wacom/gsd-wacom-key-shortcut-button.c
@@ +65,3 @@
+  GdkModifierType mods;
+  guint32 time;
+} ShortcutInfo;

As mentioned on IRC, the goal was to avoid allocating/deallocating a structure. Move those in GsdWacomKeyShortcutButtonPrivate instead.

Created attachment 249941 [details] [review]
wacom: Use the OSD window to edit the tablets' buttons

New version.

Review of attachment 249941 [details] [review]:

Looks good.

Pushed.

commit b9744e37a4ea263f5959b0f09642f3fabfb92a5c
commit e4df87580db38444a85e72c45d129ce78ff51db6