GNOME Bugzilla – Bug 640981
Wacom cpanel applet
Last modified: 2011-08-26 10:39:59 UTC
g-s-d has a few keys to configure wacom devices now. The control panel as of yet doesn't. I've put what I currently have on git://people.freedesktop.org/~whot/gnome-control-center.git (wacom branch). Screenshot is attached. Basic principle: There are a few global configuration settings in the "General" tab. These are always available. On the left is a list of currently detected tools, based on the selected tools more tabs become available (e.g. Tool Feel). Note that I have yet to find/draw decent graphics for the tools, they currently are simply some images I found on the web. As of filing this bug, it is rebased on GNOME_CONTROL_CENTER_2_91_5. Any feedback is welcome.
Created attachment 179668 [details] wacom-general.png "General" tab
This will need designers to review it before we add it to the control-center.
what's the time frame I'm looking at here? I'm getting flak from all sides for not having a graphical wacom configuration UI.
You'll need to ask mccann or jimmac (or hbons, aday, andreasn, who would certainly have experience of those devices) on #gnome-design on GIMPNet. It's unlikely for GNOME 3.0 though, unless this gets moving sharply.
Just an update (for me as much as anyone else), this is currently pushed to 3.2
Update with new code: http://cgit.freedesktop.org/~whot/gnome-control-center/log/?h=wacom Looks mostly like http://live.gnome.org/Design/SystemSettings/Tablet, calibration is still missing though.
(In reply to comment #6) > Update with new code: > http://cgit.freedesktop.org/~whot/gnome-control-center/log/?h=wacom If it's just a single patch, I'd rather you added it to bugzilla so it can be easily tracked and reviewed.
Created attachment 190419 [details] [review] wacom: Add a wacom control panel
Review of attachment 190419 [details] [review]: No signed off by needed. Please change the copyright accordingly. I don't think Intel can lay claim to the boilerplate necessary to write a panel :) ::: panels/wacom/Makefile.am @@ +36,3 @@ + +desktopdir = $(datadir)/applications +Desktop_in_files = gnome-wacom-panel.desktop.in lower-case (desktop_in_files, not Desktop_in_files) ::: panels/wacom/cc-wacom-panel.c @@ +1,2 @@ +/* + * Copyright (C) 2010 Intel, Inc Please change the copyright and authors. @@ +115,3 @@ + &error); + if (error != NULL) + { g_object_unref (priv->builder); ::: panels/wacom/gnome-wacom-panel.desktop.in.in @@ +15,3 @@ +X-GNOME-Settings-Panel=wacom +# Translators: those are keywords for the wacom tablet control-center panel +_X-GNOME-Keywords=Tablet;Wacom;Stylus; Eraser, mouse. Are there others? ::: panels/wacom/gnome-wacom-properties.c @@ +35,3 @@ +#include <sys/types.h> +#include <sys/stat.h> +#include <dirent.h> What are those for? If you're reading through a directory, GDir is probably good enough (or using the GIO functions instead). @@ +40,3 @@ +#include <X11/extensions/XInput.h> + +#define WID(x) (GtkWidget*) gtk_builder_get_object (dialog, x) "GtkWidget *" @@ +51,3 @@ + */ + +static GSettings *wacom_settings = NULL; Don't like global variables. Please cram all that in a structure. (The other panels do that because they're based on 10 years of code, not because it's a good idea :) @@ +65,3 @@ + +/* Tablet mode combo box storage columns + * MODEID_COLUMN is special, it's a string that double-functions as ID Is there any reason we can't use separate columns for that? @@ +99,3 @@ +}; + +/* Remove empty line. @@ +118,3 @@ + g_settings_set_value (settings, "pressurecurve", array); + + /* FIXME: why does this crash? The retval of "g_variant_new_int32" is a floating reference. g_variant_new_array will make the array take ownership as per docs. "If the children are floating references (see g_variant_ref_sink()), the new instance takes ownership of them as if via g_variant_ref_sink()." @@ +134,3 @@ +eraser_feel_value_changed_cb (GtkRange *range, gpointer user_data) +{ + set_pressurecurve (range, eraser_settings); 8 char indent please. @@ +161,3 @@ +/* Search the X input device list for wacom tablets. + * + * @return True if at least one compatible tablet was found, False otherwise. If you're going to document your internal functions, please use gtk-doc style. @@ +163,3 @@ + * @return True if at least one compatible tablet was found, False otherwise. + */ +static Bool gboolean please. @@ +164,3 @@ + */ +static Bool +find_tablets (void) The function is not actually run, is it? This really needs to be hotpluggable. I also think that GDK has enough API to implement this without using X directly. @@ +220,3 @@ + +static void +set_mode (GSettings *settings, int is_absolute) Why is this an int? @@ +225,3 @@ + + value = g_variant_new_boolean (!!is_absolute); + g_settings_set_value (settings, "is-absolute", value); g_settings_set_boolean (settings, "is-absolute", !!is_absolute); @@ +248,3 @@ + switch (mode) + { + case 0: /* Stylus absolute */ Add an enum for those magic values. @@ +268,3 @@ + int stylus_is_absolute, eraser_is_absolute; + + settings = stylus_settings; Is this actually useful? We don't have 72-char limits on line width. We have big screens, and text editors that can wrap lines.
sorry, I should have mentioned that this is a preliminary version of the patch that I hadn't cleaned up yet (hence just the link to the git repo). Thanks for the review though, I'll submit a patch when I've amended with your comments. also re: floating references: thanks, I read those docs but wasn't 100% sure
Created attachment 190553 [details] [review] 0001-wacom-add-a-wacom-control-panel.patch Updated, this time in more polished version. Still submitting as one giant patch because a lot has changed again, future versions can be as diffs it you prefer. A few things to note: - I can't seem to get the spacing to be like on the mockup. Does the CP force smallest-packing? - gnome-wacom-properties.c from the previous patch disappeared, it's now in cc-wacom-panel.c - no more globals - ditched the find_tablet(). IMO people should be able to set the values even if the tablet isn't currently connected. - Calibration part is still missing, I've simply set the button to invisible for now.
Review of attachment 190553 [details] [review]: Looking good, but my main concern is how you intend on configuring an absent tablet. It might have a different set of buttons, for example, and I don't think we should show options for hardware that doesn't exist, or doesn't match reality. I think that we should have "whole window" messages when a tablet is not present, for example: No tablets detected [ Set Up a Bluetooth Tablet] As for the spacing, that can be dealt with when the code is merged. ::: panels/wacom/cc-wacom-panel.c @@ +116,3 @@ + int i; + + variant = g_settings_get_value (settings, "pressurecurve"); You're leaking variant (values is const, and is only valid as long as variant exists). @@ +129,3 @@ + break; + } + } What happens if we can't find the value? @@ +144,3 @@ + return; + + liststore = GTK_LIST_STORE (WID ("liststore-tabletmode")); why not make it a GtkTreeModel straight away? You never use it as a GtkListStore. @@ +159,3 @@ + break; + default: + g_warning ("Ignoring unknown tablet mode %d.\n", mode); No need for a carriage return, it will already include one. @@ +168,3 @@ +{ + CcWacomPanelPrivate *priv = CC_WACOM_PANEL(panel)->priv; + gboolean stylus_is_absolute, Use separate lines please. ::: panels/wacom/wacom-module.c @@ +30,3 @@ +{ + bindtextdomain (GETTEXT_PACKAGE, GNOMELOCALEDIR); + bind_textdomain_codeset (GETTEXT_PACKAGE, "UTF-8"); 8 spaces (or 8-char tabs, up to you) for indent please.
(In reply to comment #12) > Review of attachment 190553 [details] [review]: > > Looking good, but my main concern is how you intend on configuring an absent > tablet. It might have a different set of buttons, for example, and I don't > think we should show options for hardware that doesn't exist, or doesn't match > reality. The mockup doesn't leave a lot of room for configuration anyway. So far, all but one device that I've seen has the three buttons that we allow configuration for. This would be different if we allowed for pad button configuration but that seems to not be catered for in the design. > I think that we should have "whole window" messages when a tablet is not > present, for example: > No tablets detected > [ Set Up a Bluetooth Tablet] I need a mockup for that then. I don't have the cycles to implement this twice again. > ::: panels/wacom/cc-wacom-panel.c > @@ +129,3 @@ > + break; > + } > + } > > What happens if we can't find the value? I don't know. gsettings actually allows for more flexible pressure curves than just those few in the GUI. If we call gtk_adjustment_set_value() for values not matching the default curves, we overwrite the custom setting whenever we fire up the GUI. > > @@ +144,3 @@ > + return; > + > + liststore = GTK_LIST_STORE (WID ("liststore-tabletmode")); > > why not make it a GtkTreeModel straight away? You never use it as a > GtkListStore. GtkListStore allows for the data to be in the xml file instead of in-code. Does the GtkTreeModel do the same? (doesn't do so in glade) other comments amended as requested, thanks
(In reply to comment #13) > (In reply to comment #12) > > Review of attachment 190553 [details] [review] [details]: > > > > Looking good, but my main concern is how you intend on configuring an absent > > tablet. It might have a different set of buttons, for example, and I don't > > think we should show options for hardware that doesn't exist, or doesn't match > > reality. > > The mockup doesn't leave a lot of room for configuration anyway. So far, all > but one device that I've seen has the three buttons that we allow configuration > for. This would be different if we allowed for pad button configuration but > that seems to not be catered for in the design. I'm guessing that the design might be extended in the future. Jakub can tell you more. > > I think that we should have "whole window" messages when a tablet is not > > present, for example: > > No tablets detected > > [ Set Up a Bluetooth Tablet] > > > I need a mockup for that then. I don't have the cycles to implement this twice > again. I'll leave that up to Jakub. > > ::: panels/wacom/cc-wacom-panel.c > > @@ +129,3 @@ > > + break; > > + } > > + } > > > > What happens if we can't find the value? > > I don't know. gsettings actually allows for more flexible pressure curves than > just those few in the GUI. If we call gtk_adjustment_set_value() for values not > matching the default curves, we overwrite the custom setting whenever we fire > up the GUI. We should probably show it's custom somewhere. Jakub? > > @@ +144,3 @@ > > + return; > > + > > + liststore = GTK_LIST_STORE (WID ("liststore-tabletmode")); > > > > why not make it a GtkTreeModel straight away? You never use it as a > > GtkListStore. > > GtkListStore allows for the data to be in the xml file instead of in-code. Does > the GtkTreeModel do the same? (doesn't do so in glade) What I meant was that you do: GtkListStore *liststore; liststore = GTK_LIST_STORE (WID ("liststore-tabletmode")); gtk_tree_model_get (GTK_TREE_MODEL (liststore), &iter, MODENUMBER_COLUMN, &mode, -1); instead of: GtkTreeModel *model; model = GTK_TREE_MODEL (WID("liststore-tabletmode")); gtk_tree_model_get (model, &iter, MODENUMBER_COLUMN, &mode, -1); > other comments amended as requested, thanks
I committed this. There are however a number of bugs. - I had to hide the calibrate button, as it doesn't do anything - It still doesn't detect whether a Wacom tablet is present (and you didn't get any updates from jimmac about the design) - The lines coming out of the image need to be dynamic, as it looks completely broken after my spacing changes - the GtkGrid seems to be adding bottom padding when you add border width. Will file those in separate bugs. You are now the owner of this panel, and need to fix the bugs above before 3.2, thanks!