GNOME Bugzilla – Bug 677095
modifying display layout affects calibration on dualhead
Last modified: 2013-05-16 13:49:39 UTC
If would be nice if screen tablet calibration was linked to the particular display. Modifying display layout breaks calibration for me: - I have a two display setup, Regular LCD + a display tablet (Cintiq UX2 in my case) - calibrate a screen tablet - go to Displays, move tablet to the opposite side of the other display - tablet area now on the other display and I am unable to perform calibration as the first tap doesn't register. I used to use xsetwacom to set the appropriate display with `xsetwacom set "Wacom Cintiq 21UX2 stylus" MapToOutput DVI-I-2` which still seems to do the trick. Also seems to be affected by resolution changes, which works fine if I use the wacom as the only screen. Tested on F17. libwacom-0.5-3.fc17.x86_64, xorg-x11-drv-wacom-0.14.0-2.fc17.x86_64
technical explanation: mapping to a display is handled in the server and simply maps the tablet to some subsection of the display width/height (e.g. 50% of width, 100% of height for a identical dual-monitor setup) calibration is handled in the driver and remaps tablet inpu to the axis ranges reported by the device. so after calibration, input 100/100 may be sent as 90/90, which is then remapped to whatever 90/90 is on 50% of the display. solutions we need here: - if the monitor configuration changes, re-apply the mapping. this requires some cooperation between the randr and the wacom plugins/panels. - if the resolution changes, warn that re-calibration may be needed. this needs some UI design, possibly a notification.
The current code in git already does the first point, ie re-apply the mapping when XRandR configuration changes are detected. This was added as part of commit 49c02b7e to fix bug 668614 (change display mapping on "monitors-changed" event) There's no notification though when resolution changes.
Created attachment 219016 [details] [review] Proposed patch Re-apply calibration and are on screen change so that either calibration or aspect ratio (both being handled by the "area" parameter) is restored after a screen change (just like we do with mapping).
Patch pushed to git. Bug remains open for the notification in case of resolution change that needs to be implemented.
Notification should be added in gnome-settings-daemon, not gnome-control-center (since it's the daemon that will trigger the warning when it detects a change in resolution, not gnome-control-center). It would allow for redoing calibration of the device by invoking the calibration process from gnome-control-center for that given screen tablet. This requires command-line arguments to be parsed in control-center Wacom panel, see bug 692816
Maintainer change
Working on it.
Created attachment 241914 [details] [review] g-s-d patch This is the g-s-d patch that, when the monitor size changes, shows a notification with the calibration action on it. Another patch for g-c-c will be uploaded next.
Created attachment 241915 [details] [review] g-c-c patch This is the g-c-c patch which sets the last-calibrated-resolution key so g-s-d will know whether a notification is needed or not.
Created attachment 243042 [details] [review] g-s-d patch Updating the g-s-d patch because there was an unfreed string.
Review of attachment 243042 [details] [review]: Please attach a screenshot of the "UI" so that designers don't need to build gnome-settings-daemon and can check the design. ::: plugins/wacom/Makefile.am @@ +26,3 @@ $(WACOM_CFLAGS) \ + $(AM_CFLAGS) \ + -DBINDIR=\"$(bindir)\" Remove this. ::: plugins/wacom/gsd-wacom-manager.c @@ +1408,3 @@ + settings = gsd_wacom_device_get_settings (device); + + variant = g_settings_get_value (settings, No need for line feeds here, we have wide screens. @@ +1445,3 @@ +remove_notification (GsdWacomManager *manager) +{ + g_object_unref (manager->priv->calibration_notification); g_clear_object(); @@ +1452,3 @@ +static void +on_notification_closed (NotifyNotification *notification, + GsdWacomManager *manager) indentation. @@ +1459,3 @@ +static void +on_notification_action (NotifyNotification *notification, + gchar *action, indentation. @@ +1468,3 @@ + GsdWacomManager *manager = GSD_WACOM_MANAGER (data); + + device_name = No linefeeds after "=" please. @@ +1472,3 @@ + + if (g_strcmp0 (action, "run-calibration") == 0) { + command = g_strdup_printf (BINDIR "/gnome-control-center wacom " Why bindir? Look for it in the $PATH. @@ +1522,3 @@ + _("Wacom Settings")); + notify_notification_set_timeout (priv->calibration_notification, + 15000); No magic numbers please. @@ +1527,3 @@ + notify_notification_add_action (priv->calibration_notification, + "run-calibration", + /* TRANSLATORS: launches the Can you put this above the call, rather than inline in the parameters? It's not helping readability. @@ +1558,3 @@ +{ + manager->priv->calibration_device = device; + manager->priv->notification_timeout_src_id = linefeed. @@ +1597,3 @@ set_keep_aspect (device, g_settings_get_boolean (settings, KEY_KEEP_ASPECT)); + else if (type == WACOM_TYPE_STYLUS) { + g_source_remove ( eek. linefeed.
Review of attachment 241915 [details] [review]: ::: panels/wacom/cc-wacom-page.c @@ +181,3 @@ + + screen = gdk_screen_get_default (); + monitor = gsd_wacom_device_get_display_monitor (device); Can't all this information come from the calibration process, rather than having to get it back? @@ +190,3 @@ + } + + tmp = g_malloc (2 * sizeof (GVariant*)); This is certainly an ugly way to do things. See the docs for g_variant_builder_init() for example (though something link g_variant_new ("(ii)", width, height) probably works).
Created attachment 244176 [details] [review] g-c-c patch New version hopefully fixing the last issues pointed out.
Created attachment 244178 [details] [review] g-s-d patch New version for g-s-d.
Review of attachment 244178 [details] [review]: Mostly the same style problems as the previous patch. ::: plugins/wacom/gsd-wacom-manager.c @@ +1401,3 @@ +static gboolean +check_need_for_calibration (GsdWacomManager *manager, + GsdWacomDevice *device) Indentation. @@ +1464,3 @@ + + if (g_strcmp0 (action, "run-calibration") == 0) { + command = g_strdup_printf ("gnome-control-center wacom " Does this need to be on multiple lines? @@ +1498,3 @@ + g_warning ("Failed to close notification: %s", + error->message); + g_error_free (error); g_clear_error(). @@ +1501,3 @@ + error = NULL; + } + g_object_unref (priv->calibration_notification); g_clear_object(); @@ +1505,3 @@ + } + msg_body = g_strdup_printf (_("Tablet %s needs to be calibrated."), + gsd_wacom_device_get_name (priv->calibration_device)); Indentation. @@ +1506,3 @@ + msg_body = g_strdup_printf (_("Tablet %s needs to be calibrated."), + gsd_wacom_device_get_name (priv->calibration_device)); + priv->calibration_notification = Linefeed. @@ +1549,3 @@ +{ + manager->priv->calibration_device = device; + manager->priv->notification_timeout_src_id = g_timeout_add (2000, Magic number. @@ +1589,3 @@ set_keep_aspect (device, g_settings_get_boolean (settings, KEY_KEEP_ASPECT)); + else if (type == WACOM_TYPE_STYLUS) { + g_source_remove (manager->priv->notification_timeout_src_id); notification_id = 0?
Review of attachment 244176 [details] [review]: ::: panels/wacom/cc-wacom-page.c @@ +171,3 @@ tmp = g_malloc (nvalues * sizeof (GVariant*)); for (i = 0; i < ncal; i++) + tmp[i] = g_variant_new_int32 (cal[i]); Why did you change the indentation on this line? @@ +184,3 @@ + "last-calibrated-resolution", + last_resolution); + g_clear_object (&last_resolution); g_clear_pointer(&last_resolution, g_variant_unref); GVariant isn't a GObject.
(In reply to comment #16) > Review of attachment 244176 [details] [review]: > > ::: panels/wacom/cc-wacom-page.c > @@ +171,3 @@ > tmp = g_malloc (nvalues * sizeof (GVariant*)); > for (i = 0; i < ncal; i++) > + tmp[i] = g_variant_new_int32 (cal[i]); > > Why did you change the indentation on this line? My Emacs doesn't really like tabs indentation and I didn't notice it in this and other cases. I will fix it. > > @@ +184,3 @@ > + "last-calibrated-resolution", > + last_resolution); > + g_clear_object (&last_resolution); > > g_clear_pointer(&last_resolution, g_variant_unref); > > GVariant isn't a GObject. Sorry, I haven't used GVariants that much and I thought you had mentioned g_clear_object. I'll change it too. Regarding breaking the lines. I usually go for 80 char max lines because while I do have a big resolution screen, 1) we should keep consistency and 2) I (and many people) divide the editor in 2, making it not so wide even for big screens. Since we're using tabs, it's true that 80 char lines will require more line breaks than usual so I'll just do as you say. It would be very nice if we could be more consistent with the Glib/GNOME coding style.
Created attachment 244281 [details] [review] g-s-d patch New version.
Created attachment 244282 [details] [review] g-c-c patch New version.
Review of attachment 244281 [details] [review]: The code also only seems to support a single tablet not being calibrated for the right resolution. If I had 2 tablets, this code would probably do unexpected things. File a separate bug about this please. ::: plugins/wacom/gsd-wacom-manager.c @@ +1421,3 @@ +{ + g_clear_object (&manager->priv->calibration_notification); + manager->priv->calibration_notification = NULL; Why are you setting it to NULL even though you're using g_clear_object() already? @@ +1511,3 @@ + manager); + + No need for the double-linefeed here.
Review of attachment 244282 [details] [review]: Fine to commit after those. ::: panels/wacom/cc-wacom-page.c @@ +184,3 @@ + "last-calibrated-resolution", + last_resolution); + g_clear_pointer(&last_resolution, g_variant_unref); space before the bracket. @@ +205,3 @@ + calib_area_get_display_size (area, &display_width, &display_height); + + set_calibration(page->priv->stylus, Space before the bracket.
Created attachment 244332 [details] [review] g-c-c patch New version.
Created attachment 244333 [details] [review] g-s-d patch New version.
Review of attachment 244333 [details] [review]: ::: plugins/wacom/gsd-wacom-manager.c @@ +1350,3 @@ + manager->priv->notification_timeout_src_id = 0; + manager->priv->calibration_notification = NULL; + manager->priv->calibration_device = NULL; private struct members are automatically initialised to NULL when instantiated. @@ +1472,3 @@ + if (priv->calibration_notification) { + gboolean success; + success = notify_notification_close (priv->calibration_notification, We don't care if this fails. @@ +1566,3 @@ set_keep_aspect (device, g_settings_get_boolean (settings, KEY_KEEP_ASPECT)); + else if (type == WACOM_TYPE_STYLUS) { + g_source_remove (manager->priv->notification_timeout_src_id); You need to check for the id being > 0 which is probably easier done in a separate call. (the notification also needs to be removed, otherwise you'll call it with ->device = NULL when the user clicks on "calibrate") @@ +1703,3 @@ + if (wacom_manager->priv->notification_timeout_src_id != 0) + g_source_remove (wacom_manager->priv->notification_timeout_src_id); The notification removal is supposed to have happened when you stop the plugin.
Comment on attachment 244333 [details] [review] g-s-d patch Committed the g-s-d patch with fixes mentioned in the review above.
Review of attachment 244332 [details] [review]: Looks good.
All pushed. Closing.