GNOME Bugzilla – Bug 692817
Wacom: Add a "configure" button to the OSD window
Last modified: 2013-06-12 12:52:50 UTC
So that users can invoke the Wacom panel in control-center from the OSD window. Could also possibly add a link to the button configuration dialog, so keyboard shortcuts could be changed easily. Could also possibly show a "calibrate" button to invoke calibration, but only on screen tablets. This requires command-line arguments to be parsed in control-center Wacom panel, see bug 692816
Note, the pointer should be made visible while the OSD is showing, otherwise clicking on the buttons won't be possible. The pointer could be made visible/invisible depending on the device that caused the motion event, or completely visible all the time (not sure we really need to hide the pointer in the OSD).
Maintainer change
I'd like to get a suggestion from a designer about how this should look.
Short term, a link to the shortcut assignment UI would be nice as it's a likely flow from figuring out a missing binding. The button mapping UI would actually become a lot clearer if done in a physical layout like it is provided in the OSD. But that redesign is outside of the scope of this bug. I'll look into refreshing the design to avoid a list of button[1-9] assignments.
If you're wondering about positioning the link, I'd go with center bottom edge, the same way we indicate messages in the shell overview.
Created attachment 243025 [details] [review] wacom: Add a configure button to the OSD window Pressing the button will show the GNOME Control Center at the device's configuration page. It also shows the pointer when the movement was performed by a mouse type device, hiding it again after a little while.
Created attachment 243060 [details] Screenshot with the link unfocused Here you have a screenshot of what it looks like.
Created attachment 243061 [details] Screenshot with the link focused Another one with the focused link.
Please use the osd style/class for the button.
Created attachment 244520 [details] Screenshot with the link focused
Created attachment 244521 [details] [review] wacom: Add a configure button to the OSD window
Created attachment 244522 [details] [review] wacom: Add a configure button to the OSD window
Created attachment 244922 [details] [review] wacom: Add a configure button to the OSD window New version where the button is not underlined when entered. (following Jakub's comments)
Created attachment 244923 [details] Screenshot with the link focused New screenshot showing the changes when the button is focused.
Created attachment 245045 [details] [review] wacom: Add a configure button to the OSD window New version.
Removing the ui-review keyword because Jakub had mentioned this was the last change (UI-wise).
Review of attachment 245045 [details] [review]: ::: plugins/wacom/gsd-wacom-osd-window.c @@ +49,3 @@ +#define CONFIGURE_BUTTON_UNFOCUSED_COLOR "4a90d9" +#define CONFIGURE_BUTTON_FOCUSED_COLOR "ffffff" I'd really like to avoid colours defined in the sources, so let's not use custom styling. @@ +1216,3 @@ + +static void +set_configure_button_label (GtkWidget *button, gboolean hover) Don't do that, just make it a normal GtkLinkButton. If theming changes are necessary, they should live in gnome-themes-standard. @@ +1308,3 @@ + if (window->priv->cursor_timeout != 0) + g_source_remove (window->priv->cursor_timeout); + window->priv->cursor_timeout = g_timeout_add (2000, add_seconds? And magic number. @@ +1643,3 @@ + g_signal_connect (configure_button, + "clicked", + G_CALLBACK (on_configure_button_clicked), All those callbacks can be removed too. @@ +1716,3 @@ priv = osd_window->priv; + if (priv->cursor_timeout != 0) + g_source_remove (priv->cursor_timeout); That should be done in _stop(), and _stop() should be called in _finalize().
(In reply to comment #17) > @@ +1216,3 @@ > + > +static void > +set_configure_button_label (GtkWidget *button, gboolean hover) > > Don't do that, just make it a normal GtkLinkButton. If theming changes are > necessary, they should live in gnome-themes-standard. The normal GtkLinkButton is underlined also on hover and Jakub wanted it not to be underlined. Also, it's underlined using Pango's attributes which makes it not possible to change through CSS. I don't remember correctly whether the link color could be changed (Jakub wants it white on :hover/:active) but I also think it doesn't. I discussed this with you on IRC at the time :) I am okay with just changing it back to a GtkLinkButton if we can commit it that way.
Any styling problem should be fixed in GtkLinkButton and/or gnome-themes-standard, not in our tool. Is there any reason it's not simply a button? That would solve the underline problem.
I guess the only reason is the "looks". Jakub can comment on this.
At the end of the day, it could just be a button, considering the fact the configuration will be done in here and then it will be a button.
Created attachment 246389 [details] [review] wacom: Add a configure button to the OSD window New version with Bastien's suggestions.
Review of attachment 246389 [details] [review]: ::: plugins/wacom/gsd-wacom-osd-window.c @@ +1191,3 @@ + gchar *command; + const gchar *device_name; + GError *error = NULL; line feed here. @@ +1229,3 @@ + +static void +osd_window_set_cursor (GsdWacomOSDWindow *window, GdkCursorType cursor_type) { Curly brace on the line below. @@ +1240,3 @@ + +static void +show_cursor (GsdWacomOSDWindow *window) { Ditto. @@ +1245,3 @@ + +static void +hide_cursor (GsdWacomOSDWindow *window) { Ditto. @@ +1250,3 @@ + +static gboolean +cursor_timeout_source_func (gpointer data) { Ditto. @@ +1254,3 @@ + hide_cursor (window); + window->priv->cursor_timeout = 0; + return FALSE; Use G_SOURCE_REMOVE instead. @@ +1267,3 @@ +on_motion_event (GsdWacomOSDWindow *window, GdkEventMotion *event, gpointer data) +{ + GdkInputSource source; Linefeed after that. @@ +1600,3 @@ NULL); + configure_button = create_osd_configure_button (osd_window); A lot of the things you're doing in gsd_wacom_osd_window_new() should be in _init() instead. Stuff like: g_signal_connect (GTK_WIDGET (osd_window), "realize", G_CALLBACK (gsd_wacom_osd_window_realized), NULL); g_signal_connect (GTK_WIDGET (osd_window), "map", G_CALLBACK (gsd_wacom_osd_window_mapped), NULL); should be ->map and ->realize on the widget_class instead. Please file a separate bug about this, and don't add any more code to _new(). @@ +1649,3 @@ + osd_window->priv->cursor_timeout = 0; + g_signal_connect (osd_window, "draw", + G_CALLBACK (gsd_wacom_osd_window_draw), ->draw on the widget_class instead. @@ +1653,3 @@ + gtk_widget_add_events (GTK_WIDGET (osd_window), GDK_POINTER_MOTION_MASK); + g_signal_connect (osd_window, "motion-notify-event", + G_CALLBACK (on_motion_event), ->motion_notify_event on the widget_class as well.
(In reply to comment #23) > Stuff like: > g_signal_connect (GTK_WIDGET (osd_window), "realize", > G_CALLBACK (gsd_wacom_osd_window_realized), > NULL); > g_signal_connect (GTK_WIDGET (osd_window), "map", > G_CALLBACK (gsd_wacom_osd_window_mapped), > NULL); > should be ->map and ->realize on the widget_class instead. Please file a > separate bug about this, and don't add any more code to _new(). > Okay. (this wasn't introduced by my patches) > @@ +1649,3 @@ > + osd_window->priv->cursor_timeout = 0; > + g_signal_connect (osd_window, "draw", > + G_CALLBACK (gsd_wacom_osd_window_draw), > > ->draw on the widget_class instead. I added the draw signal because AFAIR, overriding the method wouldn't paint the configure button... > > @@ +1653,3 @@ > + gtk_widget_add_events (GTK_WIDGET (osd_window), GDK_POINTER_MOTION_MASK); > + g_signal_connect (osd_window, "motion-notify-event", > + G_CALLBACK (on_motion_event), > > ->motion_notify_event on the widget_class as well. Okay, I'll change this one in this patch.
(In reply to comment #24) > (In reply to comment #23) > > Stuff like: > > g_signal_connect (GTK_WIDGET (osd_window), "realize", > > G_CALLBACK (gsd_wacom_osd_window_realized), > > NULL); > > g_signal_connect (GTK_WIDGET (osd_window), "map", > > G_CALLBACK (gsd_wacom_osd_window_mapped), > > NULL); > > should be ->map and ->realize on the widget_class instead. Please file a > > separate bug about this, and don't add any more code to _new(). > > > > Okay. (this wasn't introduced by my patches) That's why I asked to file a separate bug :) > > @@ +1649,3 @@ > > + osd_window->priv->cursor_timeout = 0; > > + g_signal_connect (osd_window, "draw", > > + G_CALLBACK (gsd_wacom_osd_window_draw), > > > > ->draw on the widget_class instead. > > I added the draw signal because AFAIR, overriding the method wouldn't paint the > configure button... Did you forget to chain up?
Created attachment 246617 [details] [review] wacom: Add a configure button to the OSD window New version meeting Bastien's latest comments.
Review of attachment 246617 [details] [review]: Looks fine otherwise. ::: plugins/wacom/gsd-wacom-osd-window.c @@ +1267,3 @@ +{ + if (window->priv->cursor_timeout != 0) + g_source_remove (window->priv->cursor_timeout); and: window->priv->cursor_timeout = 0;
Pushed after that small fix. Thanks. commit 7e67bd5a5a7e254cfd7650d2a35471720d6db528