GNOME Bugzilla – Bug 783797
Nautilus file selection (rubberband) has one corner stuck at 0,0 when using Wacom pen under Wayland
Last modified: 2017-06-15 16:41:44 UTC
The rubberband selection effect in Nautilus does not work properly when used with a Wacom tool (pen, puck, etc.) under Wayland. Instead of drawing a selection rectangle from the point the pen went down to the current pen position, the rectangle is drawn from the top-left corner of the window to the original click position. The shape of the rectangle does not change in response to pen movement.
Created attachment 353768 [details] [review] Fix issue by storing the triggering GdkDevice
Created attachment 353769 [details] [review] Fix graphical glitch caused by simultaneous rubberband actions
Review of attachment 353768 [details] [review]: ::: src/nautilus-canvas-container.c @@ +2758,3 @@ signals[BAND_SELECT_STARTED], 0); + band_info->device = event->device; Couldn't this device go away on the timeout? Probably not gonna happen in practicity... Still, would be good to "clean it up" when the rubberbanding is done, instead of having a pointer to device that could have gone away later on.
Review of attachment 353769 [details] [review]: ::: src/nautilus-canvas-container.c @@ +2759,3 @@ band_info = &details->rubberband_info; + if (band_info->active) { brackets go on new line @@ +2760,3 @@ + if (band_info->active) { + g_warning ("Canceling active rubberband by device %s", gdk_device_get_name (band_info->device)); you mean g_debug? Or do you really want to give a warning because this should never happen? @@ +2831,3 @@ + if (event != NULL && event->device != band_info->device) + return; missing brackets
Created attachment 353845 [details] [review] Fix issue by storing the triggering GdkDevice (v2) (In reply to Carlos Soriano from comment #3) > Review of attachment 353768 [details] [review] [review]: > > ::: src/nautilus-canvas-container.c > @@ +2758,3 @@ > signals[BAND_SELECT_STARTED], 0); > > + band_info->device = event->device; > > Couldn't this device go away on the timeout? > Probably not gonna happen in practicity... > For the Wayland case that motivated this bug report, libinput is supposed to send button-release events prior to removing devices. I think X input drivers are expected to act similarly. If those events aren't sent (i.e., due to a bug) then you could potentially have a disconnected device disappear in the middle of rubberbanding. We could potentially listen for a device-removed signal and cancel the rubberband if its a big enough concern. > Still, would be good to "clean it up" when the rubberbanding is done, > instead of having a pointer to device that could have gone away later on. Patch amended.
Created attachment 353846 [details] [review] Fix graphical glitch caused by simultaneous rubberband actions (v2) (In reply to Carlos Soriano from comment #4) > Review of attachment 353769 [details] [review] [review]: > > ::: src/nautilus-canvas-container.c > @@ +2759,3 @@ > band_info = &details->rubberband_info; > > + if (band_info->active) { > > brackets go on new line > Patch amended. > @@ +2760,3 @@ > > + if (band_info->active) { > + g_warning ("Canceling active rubberband by device %s", > gdk_device_get_name (band_info->device)); > > you mean g_debug? Or do you really want to give a warning because this > should never happen? > Replacing an active rubberband be a very rare / exceptional event, but I suppose there's no particular reason to print a warning since it "should" be expected to work. Patch amended. > @@ +2831,3 @@ > > + if (event != NULL && event->device != band_info->device) > + return; > > missing brackets Patch amended.
(In reply to Jason Gerecke from comment #5) > Created attachment 353845 [details] [review] [review] > Fix issue by storing the triggering GdkDevice (v2) > > (In reply to Carlos Soriano from comment #3) > > Review of attachment 353768 [details] [review] [review] [review]: > > > > ::: src/nautilus-canvas-container.c > > @@ +2758,3 @@ > > signals[BAND_SELECT_STARTED], 0); > > > > + band_info->device = event->device; > > > > Couldn't this device go away on the timeout? > > Probably not gonna happen in practicity... > > > > For the Wayland case that motivated this bug report, libinput is supposed to > send button-release events prior to removing devices. I think X input > drivers are expected to act similarly. If those events aren't sent (i.e., > due to a bug) then you could potentially have a disconnected device > disappear in the middle of rubberbanding. > > We could potentially listen for a device-removed signal and cancel the > rubberband if its a big enough concern. Yeah I was thinking about that, but no worries, it's not a concern big enough (and even more for this code which eventually will go away with the new views) > > > Still, would be good to "clean it up" when the rubberbanding is done, > > instead of having a pointer to device that could have gone away later on. > > Patch amended.
Review of attachment 353845 [details] [review]: Excellent, thanks!
Review of attachment 353846 [details] [review]: LGTM, thanks for this work!
Pushed to master and 3.24
git bz failed, the commits are: https://git.gnome.org/browse/nautilus/commit/?id=ee67066c99605035eb8311e7a2ed8c2be4fcbd04 https://git.gnome.org/browse/nautilus/commit/?id=5bca20de257c0a3724a21dcc5e0972aee1058617