GNOME Bugzilla – Bug 668547
Wacom g-s-d plugin does not take into account output rotation when applying 'display'
Last modified: 2012-07-03 07:28:42 UTC
If a display tablet has been mapped to a specific output through the use of the 'display' gsettings key, the rotation of the output is not taken into account. The 'rotation' key specifies the rotation of the tablet, and should be updated to match the output's rotation when the 'display' key is used to initialize the device, is updated, or the associated output is rotated through gnome-control-center's Display panel.
Created attachment 214186 [details] [review] Proposed patch Note: This patch goes on top of attachment 214071 [details] [review] from bug 668614 since it relies on GnomeRRScreen notifications to apply rotation.
Review of attachment 214186 [details] [review]: Can you please make sure that the code in rotate_touchscreens() doesn't get triggered too for wacom touch devices? Otherwise we'd end up with conflicting changes to the input device setup. Looks good otherwise.
Review of attachment 214186 [details] [review]: Thinking about it, is there any reason why we can't just use the GNOME_RR_ROTATION constants and names, instead of using our own wacom specific one?
(In reply to comment #3) > Thinking about it, is there any reason why we can't just use the > GNOME_RR_ROTATION constants and names, instead of using our own wacom specific > one? The GsdWacomRotation is used in the schema, I suspect the reason is mostly historical (as the rotation names come from xsetwacom(1) I guess). GNOME_RR_ROTATION constants are bit fields (as it also supports reflection which can be combined with rotation) whereas the Wacom equivalent are just plain enums. Other than that I don't see any particular reason.
Created attachment 215733 [details] [review] Updated patch Updated patch as per comment #2 To avoid wacom touch devices in rotate_touchscreens() which is in xrandr plugin, easiest (and safest) is to make xrandr plugin depend on libwacom as well, I hope it's OK. Also there's something quite unclear to me. It's not just Wacom touch devices which need to be avoided, but Wacom screen tablets as well, as the tablet (input) rotates along with the output (tablet/monitor). Am I right here (I don't have any device to test)? Assuming the above, the patch avoids bith libwacom_has_touch () and libwacom_is_builtin ()
Comment on attachment 215733 [details] [review] Updated patch Doesn't apply anymore after the changes in bug 677032.
Created attachment 216840 [details] [review] Updated patch Updated patch for current git head. Also, adds libwacom test to xrandr only for platforms with Wacom support.
Created attachment 217280 [details] [review] Updated patch Updated patch after the fix for bug #668547 (ie the patch for bug #668547 changes the parameters passed to find_output() in gsd-wacom-device.c so if we apply the proposed patch for bug #668547 we need to get this one updated as well).
Sorry I meant pdated patch after the fix for bug #678872 ...
Review of attachment 217280 [details] [review]: ::: plugins/xrandr/Makefile.am @@ +63,3 @@ +if HAVE_WACOM +libxrandr_la_CFLAGS += $(WACOM_CFLAGS) No need to have those in an if, they would be empty if wacom isn't built. ::: plugins/xrandr/gsd-xrandr-manager.c @@ +1518,3 @@ +#ifdef HAVE_WACOM +static gboolean +is_wacom_tablet_device (XDeviceInfo *device_info) I would ifdef inside is_wacom_tablet_device() instead, and return FALSE when unsupported. @@ +1522,3 @@ + gchar *device_node; + WacomDevice *wacom_device; + gboolean wacom_tablet_device = FALSE; is_wacom_tablet_device, or is_tablet would be enough. The variable name looks like it should be a WacomDevice. @@ +1531,3 @@ + return FALSE; + + wacom_device = libwacom_new_from_path (db, device_node, FALSE, NULL); free device_node here, you don't need it afterwards. @@ +1536,3 @@ + return FALSE; + } + wacom_tablet_device = libwacom_has_touch (wacom_device) || libwacom_is_builtin (wacom_device); This should work: is_tablet = libwacom_has_touch (wacom_device); is_tablet &= libwacom_is_builtin (wacom_device); @@ +1565,2 @@ for (i = 0; i < n_devices; i++) { +#ifdef HAVE_WACOM This saves you the ifdef here, and the hope that your platform will have support soon.
Created attachment 217356 [details] [review] Updated patch (In reply to comment #10) > Review of attachment 217280 [details] [review]: > No need to have those in an if, they would be empty if wacom isn't built. Oh right! > I would ifdef inside is_wacom_tablet_device() instead, and return FALSE when > unsupported. Agreed, it's better. > is_wacom_tablet_device, or is_tablet would be enough. The variable name looks > like it should be a WacomDevice. Agreed, it's better. > free device_node here, you don't need it afterwards. Agreed, there's a possible leak otherwise... > libwacom_is_builtin (wacom_device); > > This should work: > is_tablet = libwacom_has_touch (wacom_device); > is_tablet &= libwacom_is_builtin (wacom_device); It should yet I'm not entirely sure gcc does the same kind of optimizations with this though (but I am far from being a compiler expert), so I would rather keep "&&" (it would save a call to the second function) But you're right, we want to avoid changing built-in touch screens here so it should be an AND and not an OR there, my bad. > This saves you the ifdef here, and the hope that your platform will have > support soon. Yeap, agreed. I also spotted a bug in my initial version. If the device already has a rotation set (for example, rotation set to "half" for left-handed users) then that would have reset the rotation to match the output rotation once the output rotation is changed, which is not what we want. A much better approach is to compute the relative rotation between the device and the output so that initial rotation is /translated/ to the resulting output rotation. New patch attached with this added, tested with touch device, left-handed rotation is applied after the output rotation.
Review of attachment 217356 [details] [review]: ::: plugins/wacom/gsd-wacom-device.c @@ +1807,3 @@ + } + + /* Default */ Remove this. @@ +1821,3 @@ + } + + /* Default - not reached */ Remove that. ::: plugins/wacom/gsd-wacom-manager.c @@ +251,3 @@ + return device_rotation; + + n = G_N_ELEMENTS (rotations); This is a macro, use G_N_ELEMENTS directly everywhere. @@ +258,3 @@ + + if (output_rotation == GSD_WACOM_ROTATION_HALF) + return rotations[(i + n / 2) % n]; i + n - 2? ::: plugins/xrandr/gsd-xrandr-manager.c @@ +1569,2 @@ for (i = 0; i < n_devices; i++) { + if (is_wacom_tablet_device (&device_info[i])) Add a g_debug() here saying that the tablet has been skipped for rotation. (you might want to add such debug for other cases, in a separate commit).
Created attachment 217389 [details] [review] Updated patch (In reply to comment #12) > Review of attachment 217356 [details] [review]: All done, although I am not sure the following > This is a macro, use G_N_ELEMENTS directly everywhere. helps with clarity of the code.
Review of attachment 217389 [details] [review]: ::: plugins/xrandr/gsd-xrandr-manager.c @@ +105,3 @@ +#ifdef HAVE_WACOM +static WacomDeviceDatabase *db = NULL; I just realised this. You need to put this in the ManagerPrivate structure instead, and make sure to destroy it when shutting down.
Created attachment 217811 [details] [review] Updated patch Updated patch as per comment #14
Review of attachment 217811 [details] [review]: Looks good. ::: plugins/xrandr/gsd-xrandr-manager.c @@ +2099,3 @@ +#ifdef HAVE_WACOM + if (manager->priv->wacom_db != NULL) { + libwacom_database_destroy(manager->priv->wacom_db); Space needed between the function name and the bracket.
Committed with space added. commit ecb0e2d0b15c732007a611258fd3d916953c0b7c wacom: apply display rotation to device When an output is mapped to a device, match the device rotation with the output (bug #668547). https://bugzilla.gnome.org/show_bug.cgi?id=668547