GNOME Bugzilla – Bug 692816
Wacom: add command line arguments to select the device and action
Last modified: 2013-06-05 16:33:21 UTC
to be able to chose which device to show in control-center Wacom panel when invoked from the command line, and possibly trigger an action like configure buttons, run calibration, etc. command line parsing for each panel is done in the panel itself (via the "argv" property, see the network panel for example).
Maintainer change
Created attachment 241747 [details] [review] Adds command line arguments support and allows to choose the tablet from them Here is the patch that introduces support for CLI arguments. The first thing that is possible to do is to choose the device to be shown by giving its name as the first argument.
Created attachment 241748 [details] [review] CLI support for calibration This patch can be applied on top of the previous one and allows to call the calibration area from CLI. I could have sent it in the same patch as the previous one but, besides being more atomic, I could have accomplished this functionality in other ways so here it goes.
Review of attachment 241747 [details] [review]: ::: panels/wacom/cc-wacom-panel.c @@ +69,3 @@ +/* borrowed from cc-network-panel */ +static GPtrArray * +variant_av_to_string_array (GVariant *array) Why would you unwind the whole array, just to stuff it in a strv and then only use a single parameter? Even if you were to use 2 parameters, there's still no need to go through those steps. @@ +149,3 @@ + + if (parameters) { + array = variant_av_to_string_array (parameters); Indentation.
Review of attachment 241748 [details] [review]: ::: panels/wacom/cc-wacom-panel.c @@ +187,3 @@ priv->arg_device = g_strdup (args[0]); + + if (args[1]) { This really need to be better designed. $ gnome-control-center wacom 12 run-calibration doesn't exactly roll of the tongue. See the online-accounts panel for something more modern (rather than ported from handling "argv").
In my defense, I took that from the network panel but I agree it's not exactly beautiful (just as spawning a command to open the g-c-c in some tab) so I'll look into the online-accounts panel.
Created attachment 244142 [details] [review] wacom: Add command line argument support for choosing the tablet
Created attachment 244148 [details] [review] Adds command line arguments support and allows to choose the tablet from them New version addressing Bastien's comments.
Created attachment 244149 [details] [review] CLI support for calibration Support for calibrating the device.
Review of attachment 244148 [details] [review]: ::: panels/wacom/cc-wacom-panel.c @@ +79,3 @@ + + /* Choose correct device */ + page = g_hash_table_lookup (priv->pages, One line. @@ +83,3 @@ + + if (page == NULL) { + g_warning ("Failed to find device \"%s\", supplied " One line. @@ +88,3 @@ + } else { + current = gtk_notebook_page_num (GTK_NOTEBOOK (priv->notebook), + GTK_WIDGET (page)); This isn't aligned with the opening bracket above. @@ +110,3 @@ + switch (g_variant_n_children (parameters)) { + case 2: + set_device_page (self, device_name); As mentioned earlier, I would expect the actions to be specified before the device it pertains to. gnome-control-center wacom calibrate 12 not: gnome-control-center wacom 12 calibrate @@ +117,3 @@ + break; + default: + g_warning ("Unexpected parameters found. " See below about using one line. @@ +159,3 @@ + g_variant_get_child (parameters, 1, "v", &v); + if (!g_variant_is_of_type (v, G_VARIANT_TYPE_STRING)) { + g_warning ("Wrong type for the second " Can you put that on a single line please? If you think it's too nested, then it should probably be in a separate function.
Review of attachment 244149 [details] [review]: ::: panels/wacom/cc-wacom-panel.c @@ +117,3 @@ + + if (!g_variant_is_of_type (v, G_VARIANT_TYPE_STRING)) { + g_warning ("Wrong type for the operation " One line. @@ +128,3 @@ + cc_wacom_page_calibrate (page); + else + g_warning ("The device %s cannot be " One line. @@ +132,3 @@ + device_name); + } else { + g_warning ("Ignoring unrecognized operation " One line. @@ +133,3 @@ + } else { + g_warning ("Ignoring unrecognized operation " + "\"%s\"", operation); Use '%s' so you don't need to escape it.
(In reply to comment #10) > gnome-control-center wacom calibrate 12 > not: > gnome-control-center wacom 12 calibrate I hadn't understood that. I will change that.
Created attachment 244933 [details] [review] Adds command line arguments support and allows to choose the tablet from them New version addressing Bastien's comments.
Created attachment 244934 [details] [review] CLI support for calibration New version.
Review of attachment 244933 [details] [review]: ::: panels/wacom/cc-wacom-panel.c @@ +82,3 @@ + + if (page == NULL) { + g_warning ("Failed to find device '%s', supplied in the command line.", device_name); return page; here. @@ +83,3 @@ + if (page == NULL) { + g_warning ("Failed to find device '%s', supplied in the command line.", device_name); + } else { So you can avoid using an else block here. @@ +145,3 @@ + GVariant *parameters, *v; + + parameters = g_value_get_variant (value); Move all those checks to the run_operation_from_params(). @@ +150,3 @@ + return; + + if (g_variant_n_children (parameters) > 1) { Or do is g_variant_n_children (params) <= 1 above?
Review of attachment 244934 [details] [review]: You should split the cc-wacom-page.[ch] additions from the wacom panel args handling. ::: panels/wacom/cc-wacom-page.c @@ +324,3 @@ + CcWacomPage *page) +{ + calibrate (page); Indentation. ::: panels/wacom/cc-wacom-page.h @@ +84,3 @@ gboolean ignore_first_page); +void cc_wacom_page_calibrate (CcWacomPage *page); Can you line up the function names/parameters with the above calls? ::: panels/wacom/cc-wacom-panel.c @@ +98,3 @@ const gchar *device_name = NULL; const gchar *operation = NULL; + gint nr_parameters; n_params;
Created attachment 245939 [details] [review] wacom: Add command line argument support for choosing the tablet
Created attachment 245940 [details] [review] wacom: Add cc_wacom_page_calibrate function
Created attachment 245941 [details] [review] wacom: Add command line support for calibration
Patches updated according to comments except: (In reply to comment #16) > ::: panels/wacom/cc-wacom-page.h > @@ +84,3 @@ > gboolean ignore_first_page); > > +void cc_wacom_page_calibrate (CcWacomPage *page); > > Can you line up the function names/parameters with the above calls? As far as I can see, each of the above functions aren't aligned with each other and have just a space between the return type and the arguments, just as these do. If you still want me to align it differently, please let me know how I should do it because I think aligning just this function will make the declarations inconsistent.
Created attachment 245946 [details] [review] wacom: Add cc_wacom_page_calibrate and cc_wacom_page_can_calibrate functions
Sorry, had to re-upload the patch above because the description was incomplete.
Review of attachment 245939 [details] [review]: ::: panels/wacom/cc-wacom-panel.c @@ +83,3 @@ + if (page == NULL) { + g_warning ("Failed to find device '%s', supplied in the command line.", device_name); + No need for the carriage return here. @@ +106,3 @@ + if (!g_variant_is_of_type (v, G_VARIANT_TYPE_STRING)) { + g_warning ("Wrong type for the second argument GVariant, expected 's' but got '%s'", + (gchar *) g_variant_get_type (v)); g_variant_get_type_string(). @@ +118,3 @@ + set_device_page (self, device_name); + break; + case 1: g_assert_not_reached(); You can't get here, as there's at least 2 parameters. @@ +122,3 @@ + the Wacom panel to be shown */ + break; + default: Ditto. @@ +158,3 @@ + parameters = g_value_get_variant (value); + if (parameters == NULL || g_variant_n_children (parameters) == 0 || + g_variant_n_children (parameters) <= 1) Why check for <= 1, *and* for == 0?
Review of attachment 245941 [details] [review]: Fine.
Review of attachment 245946 [details] [review]: ::: panels/wacom/cc-wacom-page.h @@ +84,3 @@ gboolean ignore_first_page); +void cc_wacom_page_calibrate (CcWacomPage *page); As per bug 692816 comment 16: Can you line up the function names/parameters with the above calls?
Created attachment 246049 [details] [review] wacom: Add command line argument support for choosing the tablet New version after Bastien's comments and I also removed some variables declarations which belonged to the "command line support for calibration" so I need to re-upload that one again.
Created attachment 246050 [details] [review] wacom: Add cc_wacom_page_calibrate and cc_wacom_page_can_calibrate functions Aligned the functions' declarations in the header as Bastien requested.
Created attachment 246051 [details] [review] wacom: Add command line support for calibration Need to re-upload this one because the base patch it seats on changed but it is essencially the same that we had before which had already the "go-ahead" from Bastien.
Review of attachment 246049 [details] [review]: Looks good.
Review of attachment 246050 [details] [review]: Looks fine to commit after those changes. ::: panels/wacom/cc-wacom-page.c @@ +1347,3 @@ +static gboolean +has_monitor (CcWacomPagePrivate *priv) Why take a priv? @@ +1349,3 @@ +has_monitor (CcWacomPagePrivate *priv) +{ + return gsd_wacom_device_get_display_monitor (priv->stylus) >= 0; return gsd_wacom_device_get_display_monitor (page->priv->stylus) >= 0; @@ +1510,3 @@ + priv = page->priv; + + return has_monitor (priv); return has_monitor(page); directly No need for the temp variable.
Review of attachment 246051 [details] [review]: ::: panels/wacom/cc-wacom-panel.c @@ +118,3 @@ switch (n_params) { + case 3: + page = set_device_page (self, device_name); The only thing I find a bit bizarre is that if calibrating were to not be available, you would still have switched to the page for that device. I guess this wouldn't happen if called from g-s-d because you already do a check there.
Created attachment 246099 [details] [review] wacom: Add cc_wacom_page_calibrate and cc_wacom_page_can_calibrate functions Attaching this again just for referrence (it's already committed with Bastien's suggested changes).
All pushed. Thanks. commit 366fd4ba9406107ea082f3762b59140432368493 commit ed3c9badff52768a6b3f924868b6bfb549f53f75 commit e45bafaacbbcd5a915e306c64fbb391aa50a3e5e