Created attachment 250019 [details] [review]
wacom: Add the gsd_wacom_osd_window_set_edition_mode method

Review of attachment 250019 [details] [review]:

Looks good.

Created attachment 250020 [details] [review]
wacom: Add a DBus interface and method to show the OSD window

There are 2 small problems with this patch that I haven't had the time to tackle yet (but wanted to share with you): 1) it takes too long to get a proxy for the DBus interface (around 1 minute) and 2) once the OSD is shown from the g-c-c, it flashes and then it's impossible to show it again (because the OSD Window isn't destroyed but is not visible, which should never happen).
I'll also attach another patch which ensures that the OSD window is always destroyed before showing it from the DBus handler.

Review of attachment 250020 [details] [review]:

::: plugins/wacom/gsd-wacom-manager.c
@@ +85,3 @@
+#define GSD_WACOM_DBUS_PATH GSD_DBUS_PATH "/Wacom"
+#define GSD_WACOM_DBUS_NAME GSD_DBUS_NAME ".Wacom"
+#define SET_OSD_VISIBILITY_DBUS_METHOD    "SetOSDVisibility"

No need for a constant for that.

@@ +122,3 @@
+        /* DBus */
+        GDBusNodeInfo   *introspection_data;
+        GDBusConnection *connection;

You're missing code in stop() to clean this up.

@@ +233,3 @@
+		}
+
+		if (visible ^ right_osd_window_visible) {

why "^"?
No braces for one-liner conditionals.

@@ +1781,3 @@
+
+        if (manager->priv->bus_cancellable == NULL ||
+            g_cancellable_is_cancelled (manager->priv->bus_cancellable)) {

That's not the correct way to check for cancellation.

@@ +1788,3 @@
+        connection = g_bus_get_finish (res, &error);
+
+        if (connection == NULL) {

Here you'd check for the error being G_IO_ERROR, G_IO_ERROR_CANCELLED.

@@ +1796,3 @@
+        manager->priv->connection = connection;
+
+        g_dbus_connection_register_object (connection,

You need to get the retval, and check it for errors, also make sure to unregister the object on stop.

(In reply to comment #3)
> Created an attachment (id=250020) [details] [review]
> wacom: Add a DBus interface and method to show the OSD window
> 
> There are 2 small problems with this patch that I haven't had the time to
> tackle yet (but wanted to share with you): 1) it takes too long to get a proxy
> for the DBus interface (around 1 minute)

Check for errors? This sounds like a timeout. I'm not sure how you tested this, but you might want to use the full gnome-settings-daemon binary with "-r" to replace your running instance.

> and 2) once the OSD is shown from the
> g-c-c, it flashes and then it's impossible to show it again (because the OSD
> Window isn't destroyed but is not visible, which should never happen).
> I'll also attach another patch which ensures that the OSD window is always
> destroyed before showing it from the DBus handler.

(In reply to comment #4)
> Review of attachment 250020 [details] [review]:
> 
> ::: plugins/wacom/gsd-wacom-manager.c
> @@ +85,3 @@
> +#define GSD_WACOM_DBUS_PATH GSD_DBUS_PATH "/Wacom"
> +#define GSD_WACOM_DBUS_NAME GSD_DBUS_NAME ".Wacom"
> +#define SET_OSD_VISIBILITY_DBUS_METHOD    "SetOSDVisibility"
> 
> No need for a constant for that.

We're using the method name twice in the code. It's good not to repeat strings IMO.

> 
> @@ +122,3 @@
> +        /* DBus */
> +        GDBusNodeInfo   *introspection_data;
> +        GDBusConnection *connection;
> 
> You're missing code in stop() to clean this up.

Right. I missed that.

> 
> @@ +233,3 @@
> +        }
> +
> +        if (visible ^ right_osd_window_visible) {
> 
> why "^"?

Because we're using the toggle method (destroys the current OSD, if any, and brings up a new one) to control the OSD and it might be already visible, with the correct device displayed or not.
XOR between whether is should be visible and whether we got the right OSD visible is needed.

> No braces for one-liner conditionals.
> 
> @@ +1781,3 @@
> +
> +        if (manager->priv->bus_cancellable == NULL ||
> +            g_cancellable_is_cancelled (manager->priv->bus_cancellable)) {
> 
> That's not the correct way to check for cancellation.

I got most of this code from other plugins. What is the correct way?

> 
> @@ +1788,3 @@
> +        connection = g_bus_get_finish (res, &error);
> +
> +        if (connection == NULL) {
> 
> Here you'd check for the error being G_IO_ERROR, G_IO_ERROR_CANCELLED.

Right. I missed that too.

> 
> @@ +1796,3 @@
> +        manager->priv->connection = connection;
> +
> +        g_dbus_connection_register_object (connection,
> 
> You need to get the retval, and check it for errors, also make sure to
> unregister the object on stop.

Okay.

(In reply to comment #5)
> (In reply to comment #3)
> > Created an attachment (id=250020) [details] [review] [details] [review]
> > wacom: Add a DBus interface and method to show the OSD window
> > 
> > There are 2 small problems with this patch that I haven't had the time to
> > tackle yet (but wanted to share with you): 1) it takes too long to get a proxy
> > for the DBus interface (around 1 minute)
> 
> Check for errors? This sounds like a timeout. I'm not sure how you tested this,
> but you might want to use the full gnome-settings-daemon binary with "-r" to
> replace your running instance.

I do use --replace.

Created attachment 250207 [details] [review]
wacom: Add a DBus interface and method to show the OSD window

New version. The OSD flashing error I mentioned is no longer happening after I rebased the source to include the change to a popup type window.
I still have the big waiting time till it gets the proxy.

About how I deal with the cancellable, I have based the code on the keyboard and xrandr's; please let me know how it should be handled then.

Created attachment 250220 [details] [review]
wacom: Add a DBus interface and method to show the OSD window

Attachment 250019 [details] pushed as 1b9d99f - wacom: Add the gsd_wacom_osd_window_set_edition_mode method
Attachment 250220 [details] pushed as 8b2f27e - wacom: Add a DBus interface and method to show the OSD window

Hi,

I just tried your changes, with the dbus name being owned, and I still get that long waiting time till it gets the proxy. That's why I hadn't included the code to own the name, because I had tried it and didn't see any changes.

Have you tried this yourself? Does it work for you?

It works fine here.

Okay, fair enough. It must be something in my system then.