Created attachment 217526 [details] [review]
Proposed patch

Patch implementing the feature as seen in the screencast.

Note, this patch goes on top of the patch for bug 676170 as the OSD window needs to match both the output rotation and the device rotation.

The OSD window also helps when switching from left-handed to right-handed mode and vice-versa, as the actual device is rotated by 180 degrees, the buttons are listed in reverse order and placed on the opposite side as seen on the following screen-cast demonstrating the effect:

http://people.redhat.com/ofourdan/gnome3/gnome-settings-wacom-invert-osd-window.webm

Created attachment 217922 [details] [review]
Updated patch

Updated patch after commit ecb0e2d0 (bug 668547) and commit 5825dda4 (Install test application).

Also changes the name of keyboard shortcuts to "Send Keystroke ...." to match the actions names in the button mapping dialog (bug 679067)

(screencasts updated with this change)

This seems like it could be related to bug 647711. Perhaps we could have a common style for these types of OSDs?

I think having a reference like this is extremely useful. Having the buttons closely representing the physical layout is also desirable. 

I think it's useful enough to grant it some better exposure in the panel UI, rather than just listing it among the mappable actions. I am thinking about having a button directly in the mapping list modal. Not sure if people would trade one of those button just to bring up a legend. It doesn't hurt to have it as a mappable action though.

(In reply to comment #5)
> I think having a reference like this is extremely useful. Having the buttons
> closely representing the physical layout is also desirable. 

The buttons closely and accurately representing the physical layout will require having the information provided by libwacom. Right now, both the list of button in the treeview and in the OSD window rely on libwacom's reported buttons, but the actual order may not be accurate, but a patch to expose the actual order from libwacom got denied so there is no technical solution yet to have an accurate button order.

In the future, the plan is to use a description similar to xkb layout to have an accurate representation of the buttons, their shape and position, provided by libwacom for each device that the OSD window would parse and use to draw the buttons. That will solve both the problem of the button order and the accurate representation of the pad, but that's quite a lot more work and it's not implemented yet.

> I think it's useful enough to grant it some better exposure in the panel UI,
> rather than just listing it among the mappable actions. I am thinking about
> having a button directly in the mapping list modal. Not sure if people would
> trade one of those button just to bring up a legend. It doesn't hurt to have it
> as a mappable action though.

Being able to trigger the OSD window from the UI panel would require a communication mechanism to be put in place (thinking on dbus here). Right now, the OSD window is implemented in the Wacom plugin of gnome-settings-daemon as it's supposed to be brought up at any time by the user (not just when the UI panel is being run).

Would that be a pre-requisite for review and possible inclusion of this patch or could this be for a later improvement (as the OSD window is thought to be already useful in this current form)?

As a side note, another OSD smaller window for the modeswitch button is also desirable (but that would be another feature), something similar the the multimedia OSD windows to show the current mode and mapped actions a couple of seconds when the mode switch button is pressed just like we do with sound volume buttons for example.

(In reply to comment #6)
> Would that be a pre-requisite for review and possible inclusion of this patch
> or could this be for a later improvement (as the OSD window is thought to be
> already useful in this current form)?

In terms of phasing the function in, I can see this work well by having the overlay as a shortcut first and then exposing it in the panel UI as a second step.

> As a side note, another OSD smaller window for the modeswitch button is also
> desirable (but that would be another feature), something similar the the
> multimedia OSD windows to show the current mode and mapped actions a couple of
> seconds when the mode switch button is pressed just like we do with sound
> volume buttons for example.

Is there hardware that has a mode switch with no indicator on the hardware itself? We don't do OSD for persistent mode status like caps/num lock and this seems to fall into the same category. We should of course make sure the status led/lcd gets properly updated, which doesn't seem to be the case with my cintiq 21ux2.

(In reply to comment #7)
> Is there hardware that has a mode switch with no indicator on the hardware
> itself? We don't do OSD for persistent mode status like caps/num lock and this
> seems to fall into the same category. We should of course make sure the status
> led/lcd gets properly updated, which doesn't seem to be the case with my cintiq
> 21ux2.

Yes, we should have hardware without any mode status on the device shortly.

LED not being lit-up on some hardware is another issue, see bug 676558.

Created attachment 223756 [details] [review]
Updated patch

Updated patch to apply cleanly after the merge of the patch from bug 668908

Also fixes the button position after rotation of an screen tablet.

Review of attachment 223756 [details] [review]:

Have you tested this on a dual screen setup? What happens when the help is visible and you work on the other monitor? Should it block out both screens?

::: plugins/wacom/gsd-wacom-device.h
@@ +100,3 @@
+/* Tablet Buttons Position*/
+typedef enum {
+	WACOM_TABLET_BUTTON_POS_LEFT = 0,

Add an "undefined" position, so that we can detect buttons with incorrect or not filled in positions.

::: plugins/wacom/gsd-wacom-manager.c
@@ +1092,3 @@
+	if (manager->priv->osd_window) {
+		gtk_widget_hide (manager->priv->osd_window);
+		gtk_widget_destroy (manager->priv->osd_window);

Just destroy it.

@@ +1124,3 @@
+
+static gboolean
+osd_window_show_hide (GsdWacomManager *manager,

toggle_visibility()

@@ +1130,3 @@
+	gchar *message, *name;
+
+	if (manager->priv->osd_window) {

Looks to me that this only handles one tablet? The osd_window should be per device.
g_object_set_data_full (G_OBJECT (device), "osd-window", ...

::: plugins/wacom/gsd-wacom-osd-window.c
@@ +35,3 @@
+#define ACTION_TYPE_KEY             "action-type"
+#define CUSTOM_ACTION_KEY           "custom-action"
+#define CUSTOM_ELEVATOR_ACTION_KEY  "custom-elevator-action"

Do these need to be exported from somewhere instead?

@@ +44,3 @@
+#define WINDOW_OPACITY		0.8
+
+/* ------------------------------------------------------------------------- */

Remove those please.

@@ +337,3 @@
+	{
+	    case GSD_WACOM_ROTATION_NONE:
+		return position;

4 spaces or 8 spaces, please choose.

@@ +442,3 @@
+	if (osd_button->priv->type == WACOM_TABLET_BUTTON_TYPE_HARDCODED)
+		draw_circle (cr, priv->area.x + priv->area.width / 2,
+		                 priv->area.y + priv->area.height / 2,

indentation again.

@@ +561,3 @@
+
+	if (gtk_cairo_should_draw_window (cr, gtk_widget_get_window (widget)))
+	{

curly braces on the same line.

Created attachment 225828 [details] [review]
Updated patch after review

(In reply to comment #10)
> Have you tested this on a dual screen setup? What happens when the help is
> visible and you work on the other monitor? Should it block out both screens?

The OSD window will show up on the same monitor the tablet is mapped to (and if not mapped to a specific output, it shows on first monitor).

> Add an "undefined" position, so that we can detect buttons with incorrect or
> not filled in positions.

Done.

> @@ +1092,3 @@
> +    if (manager->priv->osd_window) {
> +        gtk_widget_hide (manager->priv->osd_window);
> +        gtk_widget_destroy (manager->priv->osd_window);
> 
> Just destroy it.

Ah, there was actually a nasty bug, the disposal of the object was not chained to the parent class properly... Removing the gtk_widget_hide() allowed to spot the bug!

Fixed.

> +osd_window_show_hide (GsdWacomManager *manager, 
> toggle_visibility()

Done.

> Looks to me that this only handles one tablet? The osd_window should be per
> device.
> g_object_set_data_full (G_OBJECT (device), "osd-window", ...

No, I don't want to pile up multiple OSD windows on screen at the same time from different devices, that would look awful imho.

But the remark remains relevant though, so what I did is to hide the OSD window id an PAD button event is coming from another tablet.

Additionally, the OSD window will be automatically unmapped if the device is unplugged (using g_object_weak_ref() and g_object_add_weak_pointer())

> +#define ACTION_TYPE_KEY             "action-type"
> +#define CUSTOM_ACTION_KEY           "custom-action"
> +#define CUSTOM_ELEVATOR_ACTION_KEY  "custom-elevator-action"
> 
> Do these need to be exported from somewhere instead?

They could, but I would rather keep that for a more global clean up (we already duplicate those definitions between g-c-c and g-s-d)

> +/* -------------------------------------------------------------------------
> */
> Remove those please.

Sure.

> 4 spaces or 8 spaces, please choose.
> indentation again.
> curly braces on the same line.

Should be much better now wrt coding style.

Thanks for the review!

Created attachment 229644 [details] [review]
Updated patch implementing actual device layout

This is mostly a rewrite of the OSD window using the newly added layout to libwacom.

The layouts are SVG and g-s-d uses CSS via librsvg to render the buttons being pressed.

I add this patch now in advance for initial review, we shall need a release of libwacom 0.7 with the layouts added for this to work as expected. When libwacom 0.7 is released, the required version for libwacom will be updated in g-s-d.

Note, if libwacom is missign the layout definition for the given device, the on-screen help window is not avaialble (ie there is no guessed/computed/fallback solution)

More details about layouts in libwacom here:

http://linuxwacom.git.sourceforge.net/git/gitweb.cgi?p=linuxwacom/libwacom;a=tree;f=data/layouts;hb=HEAD

Created attachment 229714 [details] [review]
Updated patch implementing actual device layout

New patch moving initial SVG handling to a separate function and avoiding a possible leak of the SVG handle if the image size is incorrect.

Created attachment 229715 [details]
Screenshot of the new OSD window with a Cintiq 12WX

Created attachment 229716 [details]
Screenshot of the new OSD window with an Intuos 5 M touch

Add dependency on bug 689599 to use libwacom 0.7 new layout definitions and API.

Created attachment 230740 [details] [review]
Updated patch after review from GNOME design team

Created attachment 230741 [details]
Screenshot of the new OSD window with a Cintiq 12WX

Created attachment 230743 [details]
Screenshot of the new OSD window with an Intuos 5 M touch

Review of attachment 230740 [details] [review]:

First pass at review.

::: configure.ac
@@ +265,3 @@
     ;;
 esac
+PKG_CHECK_MODULES(LIBRSVG2, librsvg-2.0)

You'll need to check for this in the WACOM pkg-config check, if you're going to use it in the wacom plugin.

::: plugins/wacom/Makefile.am
@@ +24,3 @@
 	$(PLUGIN_CFLAGS)		\
 	$(SETTINGS_PLUGIN_CFLAGS)	\
+	$(LIBRSVG2_CFLAGS)		\

As per configure.ac comment

@@ +34,3 @@
 	$(top_builddir)/plugins/common/libcommon.la			\
 	$(SETTINGS_PLUGIN_LIBS)						\
+	$(LIBRSVG2_LIBS)						\

As per configure.ac comment

::: plugins/wacom/gsd-wacom-device.c
@@ +1601,3 @@
         p->name = NULL;
 
+        g_free (p->layout_file);

g_clear_pointer (&p->layout_file, g_free);

::: plugins/wacom/gsd-wacom-device.h
@@ +99,3 @@
 } GsdWacomTabletButtonType;
 
+/* Tablet Buttons Position*/

Missing space before end of comment.

Can you mention that those are the button positions when the tablet isn't rotated?

@@ +152,3 @@
 GList          * gsd_wacom_device_list_styli       (GsdWacomDevice *device);
 const char     * gsd_wacom_device_get_name         (GsdWacomDevice *device);
+const char     * gsd_wacom_device_get_layout_file  (GsdWacomDevice *device);

Does this return a path? a URI? A file name?

I'm guessing a full path, so I'd prefer gsd_wacom_device_get_layout_path ();

::: plugins/wacom/gsd-wacom-manager.c
@@ +857,2 @@
 static void
+osd_window_hide (GsdWacomManager *manager)

Rename this to destroy(), you're not hiding it (not in the GtkWidget sense).

@@ +871,3 @@
+                                 GsdWacomManager *manager)
+{
+	if (event->type != GDK_KEY_RELEASE)

You can remove that, you're capturing key-release events only.

@@ +873,3 @@
+	if (event->type != GDK_KEY_RELEASE)
+		return FALSE;
+	if (event->keyval != GDK_KEY_Escape)

Should you be checking that no modifier is used as well?
Alt+Esc switches windows for example.

@@ +886,3 @@
+                               GsdWacomManager *manager)
+{
+	/* If the OSD window looses focus, hide it */

"loses".

@@ +916,3 @@
+        if (widget == NULL) {
+                g_warning ("Cannot display the on-screen help window as the tablet "
+                           "definition for %s is invalid\n"

Would be good to have some error message returned (is the SVG broken, the file missing?)

@@ +917,3 @@
+                g_warning ("Cannot display the on-screen help window as the tablet "
+                           "definition for %s is invalid\n"
+                           "Please report this to libwacom at linuxwacom-devel@lists.sourceforge.net\n",

Or better, use the libwacom bugzilla on freedesktop.org.

@@ +950,3 @@
+
+	return TRUE;
+

Extraneous line feed.

@@ +1234,3 @@
 
+	if ((manager->priv->osd_window != NULL) &&
+	    (device != gsd_wacom_osd_window_get_device(GSD_WACOM_OSD_WINDOW(manager->priv->osd_window))))

space before bracket.

@@ +1476,3 @@
+
+        if (p->osd_window) {
+                gtk_widget_destroy (p->osd_window);

g_clear_object()

::: plugins/wacom/gsd-wacom-osd-window.c
@@ +526,3 @@
+			return WACOM_TABLET_BUTTON_POS_TOP;
+		break;
+	/* We only support half rotation */

Left-handed/right-handed?

@@ +628,3 @@
+	height = g_strdup_printf ("%d", osd_window->priv->tablet_area.height);
+
+	data = g_strconcat ("<?xml version=\"1.0\" encoding=\"UTF-8\" standalone=\"no\"?>\n"

Could we use GResource to embed this into the executable instead?
Would be easier to modify in the future.

@@ +714,3 @@
+	g_free (markup);
+
+	pango_layout_get_pixel_extents (layout, NULL, &logical_rect);

As discussed on IRC, see get_baseline();

@@ +892,3 @@
+		type = g_settings_get_enum (button->settings, ACTION_TYPE_KEY);
+		if (type == GSD_WACOM_ACTION_TYPE_NONE)
+			return g_strdup (_("None"));

You'll need to use a gettext context for all the "None" that you use in the code:
See C_();

@@ +951,3 @@
+			return g_strdup_printf ("Ring%s", (dir == GTK_DIR_UP ? "CCW" : "CW"));
+		if (id[0] == 'r') /* right-ring */
+			return g_strdup_printf("Ring2%s", (dir == GTK_DIR_UP ? "CCW" : "CW"));

Space before bracket.

@@ +1005,3 @@
+
+static void
+gsd_wacom_osd_window_add_normal_button (GsdWacomOSDWindow    *osd_window,

Looks quite a bit like the code in _add_elevator_button().

@@ +1087,3 @@
+	osd_window->priv->buttons = g_list_append (osd_window->priv->buttons, osd_button);
+
+	/* ... and one "Down" */

Looks like a huge amount of copy/paste. Make it into a separate function?

@@ +1184,3 @@
+	                         &osd_window->priv->tablet_area.height);
+	if (status == FALSE)
+		g_debug ("No layout found or failed to load the layout image");

And what do we do afterwards?

@@ +1204,3 @@
+	screen = gtk_window_get_screen (GTK_WINDOW (osd_window));
+	if (screen == NULL)
+		screen = gdk_screen_get_default ();

If you didn't _realize() the widget before, it won't have a screen set, and will always take the NULL path.
Either postpone all the initialisation after realize(), or just use _get_default().

@@ +1330,3 @@
+		GsdWacomOSDButton *osd_button = l->data;
+		if (g_strcmp0 (osd_button->priv->id, id) == 0)
+			gsd_wacom_osd_button_set_active (osd_button, active);

break;?

@@ +1369,3 @@
+	                             osd_window->priv->screen_area.height);
+
+	rgba_color = g_strdup_printf ("rgba(0,0,0,%f", BACK_OPACITY);

Why do that?
transparent.red = transparent.green = transparent.blue = 0.0;
transparent.alpha = BACK_OPACITY;

@@ +1440,3 @@
+	                                                      "The Wacom device represented by the OSD window",
+	                                                      GSD_TYPE_WACOM_DEVICE,
+	                                                      G_PARAM_READWRITE));

G_PARAM_CONSTRUCT_ONLY?

@@ +1466,3 @@
+	priv = osd_window->priv;
+
+	if (priv->handle) {

g_clear_object();

@@ +1471,3 @@
+	}
+
+	g_free (priv->message);

g_clear_pointer();

@@ +1475,3 @@
+
+	if (priv->buttons) {
+		g_list_free_full (priv->buttons, g_object_unref);

g_clear_pointer();

Created attachment 231887 [details] [review]
Updated patch after review

(In reply to comment #20)
> First pass at review.
> 
> ::: configure.ac
> @@ +265,3 @@
>      ;;
>  esac
> +PKG_CHECK_MODULES(LIBRSVG2, librsvg-2.0)
> 
> You'll need to check for this in the WACOM pkg-config check, if you're going to
> use it in the wacom plugin.

Done

> ::: plugins/wacom/Makefile.am
> @@ +24,3 @@
>      $(PLUGIN_CFLAGS)        \
>      $(SETTINGS_PLUGIN_CFLAGS)    \
> +    $(LIBRSVG2_CFLAGS)        \
> 
> As per configure.ac comment

Modified

> @@ +34,3 @@
>      $(top_builddir)/plugins/common/libcommon.la            \
>      $(SETTINGS_PLUGIN_LIBS)                        \
> +    $(LIBRSVG2_LIBS)                        \
> 
> As per configure.ac comment

Modified

> ::: plugins/wacom/gsd-wacom-device.c
> @@ +1601,3 @@
>          p->name = NULL;
> 
> +        g_free (p->layout_file);
> 
> g_clear_pointer (&p->layout_file, g_free);

Replaced. However the rest of the code still uses the old "g_free(p); p = NULL;" construct so I would send another patch to keep that consistent (ie update the rest to use g_clear_pointer()/g_clear_object() where possible)

> ::: plugins/wacom/gsd-wacom-device.h
> @@ +99,3 @@
>  } GsdWacomTabletButtonType;
> 
> +/* Tablet Buttons Position*/
> 
> Missing space before end of comment.

Fixed.

> Can you mention that those are the button positions when the tablet isn't
> rotated?

Added.

> @@ +152,3 @@
>  GList          * gsd_wacom_device_list_styli       (GsdWacomDevice *device);
>  const char     * gsd_wacom_device_get_name         (GsdWacomDevice *device);
> +const char     * gsd_wacom_device_get_layout_file  (GsdWacomDevice *device);
> 
> Does this return a path? a URI? A file name?
> 
> I'm guessing a full path, so I'd prefer gsd_wacom_device_get_layout_path ();

Changed.

> ::: plugins/wacom/gsd-wacom-manager.c
> @@ +857,2 @@
>  static void
> +osd_window_hide (GsdWacomManager *manager)
> 
> Rename this to destroy(), you're not hiding it (not in the GtkWidget sense).

Done.

> @@ +871,3 @@
> +                                 GsdWacomManager *manager)
> +{
> +    if (event->type != GDK_KEY_RELEASE)
> 
> You can remove that, you're capturing key-release events only.

Removed.

> @@ +873,3 @@
> +    if (event->type != GDK_KEY_RELEASE)
> +        return FALSE;
> +    if (event->keyval != GDK_KEY_Escape)
> 
> Should you be checking that no modifier is used as well?
> Alt+Esc switches windows for example.

No, actually, I want the OSD to be destroyed on any keyboard event, so I also removed the keyval test alltogether.

> @@ +886,3 @@
> +                               GsdWacomManager *manager)
> +{
> +    /* If the OSD window looses focus, hide it */
> 
> "loses".

Fixed.

> @@ +916,3 @@
> +        if (widget == NULL) {
> +                g_warning ("Cannot display the on-screen help window as the
> tablet "
> +                           "definition for %s is invalid\n"
> 
> Would be good to have some error message returned (is the SVG broken, the file
> missing?)

Added error messages in relevant part of the librsvg() parsing in gsd-wacom-osd-window.c

> @@ +917,3 @@
> +                g_warning ("Cannot display the on-screen help window as the
> tablet "
> +                           "definition for %s is invalid\n"
> +                           "Please report this to libwacom at
> linuxwacom-devel@lists.sourceforge.net\n",
> 
> Or better, use the libwacom bugzilla on freedesktop.org.

There is no libwacom component in bugzilla on freedesktop.org AFAICS.

https://bugs.freedesktop.org/enter_bug.cgi shows no "wacom" nor "libwacom".

> 
> @@ +950,3 @@
> +
> +    return TRUE;
> +
> 
> Extraneous line feed.

Removed.

 
> @@ +1234,3 @@
> 
> +    if ((manager->priv->osd_window != NULL) &&
> +        (device !=
> gsd_wacom_osd_window_get_device(GSD_WACOM_OSD_WINDOW(manager->priv->osd_window))))
> 
> space before bracket.

Fixed.

> @@ +1476,3 @@
> +
> +        if (p->osd_window) {
> +                gtk_widget_destroy (p->osd_window);
> 
> g_clear_object()

I believe g_clear_object() would crash in this context, I used g_clear_pointer (&p->osd_window, gtk_widget_destroy) instead.

> ::: plugins/wacom/gsd-wacom-osd-window.c
> @@ +526,3 @@
> +            return WACOM_TABLET_BUTTON_POS_TOP;
> +        break;
> +    /* We only support half rotation */
> 
> Left-handed/right-handed?

Rephrased.

> @@ +628,3 @@
> +    height = g_strdup_printf ("%d", osd_window->priv->tablet_area.height);
> +
> +    data = g_strconcat ("<?xml version=\"1.0\" encoding=\"UTF-8\"
> standalone=\"no\"?>\n"
> 
> Could we use GResource to embed this into the executable instead?
> Would be easier to modify in the future.

Ah, that one was a much more intrusive change...

So I put all of the CSS in a GResource (and updated Makefile.am to auto-generate the C file and build it) but using a printf() with a static resource file would not have been convenient and much more error prone (e.g extraneous '%s' in the CSS file from the resource would lead to a crash).

So I also opted to a dfferent approach, using regex to replace the color names in the CSS with actual values. It has the nice side effect of making later changes in CSS even easier.

> @@ +714,3 @@
> +    g_free (markup);
> +
> +    pango_layout_get_pixel_extents (layout, NULL, &logical_rect);
> 
> As discussed on IRC, see get_baseline();

Actually pango_layout_get_baseline() is not what we want.

  [1] http://en.wikipedia.org/wiki/Typeface
  [2] http://upload.wikimedia.org/wikipedia/commons/3/39/Typography_Line_Terms.svg

What we would want, ideally, is to center the label vertically in between the baseline and the meanline. While getting the baseline is easy, the meanline is not and there is no simple API for that:

https://mail.gnome.org/archives/gtk-i18n-list/2012-December/msg00037.html

So I took another approach which gives good result visually.

Assuming Pango rightfully places the strikethrough, I retrieved the position of the strikethrough using Pango API and used that to compoute the vertical positionning of the label.

And that gives (what I believe is) an acceptable result, at least from my point of view. 


> @@ +892,3 @@
> +        type = g_settings_get_enum (button->settings, ACTION_TYPE_KEY);
> +        if (type == GSD_WACOM_ACTION_TYPE_NONE)
> +            return g_strdup (_("None"));
> 
> You'll need to use a gettext context for all the "None" that you use in the
> code:
> See C_();

Context added.

> @@ +951,3 @@
> +            return g_strdup_printf ("Ring%s", (dir == GTK_DIR_UP ? "CCW" :
> "CW"));
> +        if (id[0] == 'r') /* right-ring */
> +            return g_strdup_printf("Ring2%s", (dir == GTK_DIR_UP ? "CCW" :
> "CW"));
> 
> Space before bracket.

Fixed.


> @@ +1005,3 @@
> +
> +static void
> +gsd_wacom_osd_window_add_normal_button (GsdWacomOSDWindow    *osd_window,
> 
> Looks quite a bit like the code in _add_elevator_button().
> 
> @@ +1087,3 @@
> +    osd_window->priv->buttons = g_list_append (osd_window->priv->buttons,
> osd_button);
> +
> +    /* ... and one "Down" */
> 
> Looks like a huge amount of copy/paste. Make it into a separate function?

Refactored to a single function gsd_wacom_osd_window_add_button_with_dir() called twice for elevator buttons.
 
> @@ +1184,3 @@
> +                             &osd_window->priv->tablet_area.height);
> +    if (status == FALSE)
> +        g_debug ("No layout found or failed to load the layout image");
> 
> And what do we do afterwards?

No layout is shown. We should not reach this at that point anyway, so removed the debug statement.

> @@ +1204,3 @@
> +    screen = gtk_window_get_screen (GTK_WINDOW (osd_window));
> +    if (screen == NULL)
> +        screen = gdk_screen_get_default ();
> 
> If you didn't _realize() the widget before, it won't have a screen set, and
> will always take the NULL path.
> Either postpone all the initialisation after realize(), or just use
> _get_default().

Fixed.

> @@ +1330,3 @@
> +        GsdWacomOSDButton *osd_button = l->data;
> +        if (g_strcmp0 (osd_button->priv->id, id) == 0)
> +            gsd_wacom_osd_button_set_active (osd_button, active);
> 
> break;?

No, breaking at first found id is not what we'd want here, some tablets have the same button more than once (e.g. Wacom DTI-520)

> > @@ +1369,3 @@
> +                                 osd_window->priv->screen_area.height);
> +
> +    rgba_color = g_strdup_printf ("rgba(0,0,0,%f", BACK_OPACITY);
> 
> Why do that?
> transparent.red = transparent.green = transparent.blue = 0.0;
> transparent.alpha = BACK_OPACITY;

Fixed.

> @@ +1440,3 @@
> +                                                          "The Wacom device
> represented by the OSD window",
> +                                                         
> GSD_TYPE_WACOM_DEVICE,
> +                                                          G_PARAM_READWRITE));
> 
> G_PARAM_CONSTRUCT_ONLY?

Added.

> @@ +1466,3 @@
> +    priv = osd_window->priv;
> +
> +    if (priv->handle) {
> 
> g_clear_object();
> 
> @@ +1471,3 @@
> +    }
> +
> +    g_free (priv->message);
> 
> g_clear_pointer();

Replaced.

> @@ +1475,3 @@
> +
> +    if (priv->buttons) {
> +        g_list_free_full (priv->buttons, g_object_unref);
> 
> g_clear_pointer();

But that does not work here and segfaults.

We want to unref each element of the list so we need to pass g_object_unref() as function to g_list_free_full()

  g_list_free_full (priv->buttons, g_object_unref);
  priv->buttons = NULL;

Cannot be replaced with g_clear_pointer();

  g_clear_pointer (&priv->buttons, g_list_free_full);

Because we have no way here to pass g_object_unref() as function to g_list_free_full() so we end calling... dunno what.

Created attachment 231888 [details]
Screenshot of the new OSD window with a Cintiq 12WX

Updated screenshot to show label positionning

Created attachment 231889 [details]
Screenshot of the new OSD window with an Intuos 5 M touch

Updated screen capture to show new label positioning