Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1792623003:
Changed DeviceDataManagerX11 to use xinput1 for low resolution scroll devices (Closed)
|
Created:
4 years, 9 months ago by Will Shackleton Modified:
4 years, 8 months ago Reviewers:
sadrul CC:
chromium-reviews, sky, Daniel Erat, Elliot Glaysher, bokan, sadrul Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChanged DeviceDataManagerX11 to use xinput1 for low resolution scroll devices
I added functionality to DeviceDataManagerX11 that uses xinput1 for
devices that gain no extra precision from using xinput2. This means that
for devices that don't ouput two scroll events on focus of a window
(which seems to be the case for classic mice driven by evdev) we don't
experience a missed scroll event on focus.
BUG=593453
Committed: https://crrev.com/b794998819088f76b4cf44c8db6940240c563cf4
Cr-Commit-Position: refs/heads/master@{#389117}
Patch Set 1 #Patch Set 2 : Moved logic to UpdateScrollClassDevice #
Total comments: 1
Patch Set 3 : Made code more readable #
Total comments: 4
Patch Set 4 : Changed code comment #Messages
Total messages: 33 (12 generated)
Description was changed from ========== Changed DeviceDataManagerX11 to use xinput1 for low resolution scroll devices I added functionality to DeviceDataManagerX11 that uses xinput1 for devices that gain no extra precision from using xinput2. This means that for devices that don't ouput two scroll events on focus of a window (which seems to be the case for classic mice driven by evdev) we don't experience a missed scroll event on focus. BUG=593453 ========== to ========== Changed DeviceDataManagerX11 to use xinput1 for low resolution scroll devices I added functionality to DeviceDataManagerX11 that uses xinput1 for devices that gain no extra precision from using xinput2. This means that for devices that don't ouput two scroll events on focus of a window (which seems to be the case for classic mice driven by evdev) we don't experience a missed scroll event on focus. BUG=593453 ==========
w.shackleton@gmail.com changed reviewers: + bokan@chromium.org, sadrul@chromium.org
bokan@chromium.org changed reviewers: - sadrul@chromium.org
bokan@chromium.org changed reviewers: + sky@chromium.org
This looks fine to me but I'm a non-OWNER here and don't have any XInput experience. Over to sky@ for OWNER review.
On 2016/03/14 17:40:19, bokan wrote: > This looks fine to me but I'm a non-OWNER here and don't have any XInput > experience. Over to sky@ for OWNER review. (Since Sadrul is out for a while)
sky@chromium.org changed reviewers: + derat@chromium.org, erg@chromium.org
I'm not familiar with this code. +derat +erg Are either of you familiar with this?
nope, but cc-ing a few more people
On 2016/03/14 18:37:01, Daniel Erat (OOO to Mar 29) wrote: > nope, but cc-ing a few more people Is this just blocked on finding a reviewer? Ping erg / tdresser / kylechar - who is the best person for this code?
On 2016/03/21 23:12:08, skobes wrote: > On 2016/03/14 18:37:01, Daniel Erat (OOO to Mar 29) wrote: > > nope, but cc-ing a few more people > > Is this just blocked on finding a reviewer? > > Ping erg / tdresser / kylechar - who is the best person for this code? I've never touched this file and am unsure why I was added to the list of people who might know about it. Have you looked at git history?
On 2016/03/21 23:16:44, Elliot Glaysher wrote: > On 2016/03/21 23:12:08, skobes wrote: > > On 2016/03/14 18:37:01, Daniel Erat (OOO to Mar 29) wrote: > > > nope, but cc-ing a few more people > > > > Is this just blocked on finding a reviewer? > > > > Ping erg / tdresser / kylechar - who is the best person for this code? > > I've never touched this file and am unsure why I was added to the list of people > who might know about it. > > Have you looked at git history? Added some reviewers based on git history.
On 2016/03/21 at 23:26:27, skobes wrote: > On 2016/03/21 23:16:44, Elliot Glaysher wrote: > > On 2016/03/21 23:12:08, skobes wrote: > > > On 2016/03/14 18:37:01, Daniel Erat (OOO to Mar 29) wrote: > > > > nope, but cc-ing a few more people > > > > > > Is this just blocked on finding a reviewer? > > > > > > Ping erg / tdresser / kylechar - who is the best person for this code? > > > > I've never touched this file and am unsure why I was added to the list of people > > who might know about it. > > > > Have you looked at git history? > > Added some reviewers based on git history. I'm not super familiar with the scroll event code in DeviceDataManagerX11.
bokan@chromium.org changed reviewers: + bokan@chromium.org
I think Sadrul is the best reviewer but he's off until Thursday, which is why I tried to find another reviewer, but at this point we might as well wait.
dnicoara@chromium.org changed reviewers: + sadrul@chromium.org - dnicoara@chromium.org
I'm not familiar enough with X11 input to review this. +sadrul@ since he'll know better. You may need to wait until he's back.
Having read http://www.x.org/archive/X11R7.7/doc/man/man3/XIQueryDevice.3.xhtml a bit more I think I need an extra check in here---we need to check that the XIScrollFlagNoEmulation flag is not set before reverting to an old scroll device like this (or else no scroll events will come through). I'm currently away from my desktop / build server but I'll be back at it in two weeks. If this CL is needed more urgently than that (I could make this change in a second CL) I can clone and build on my laptop.
tdresser@chromium.org changed reviewers: - bokan@chromium.org, kylechar@chromium.org, sheckylin@chromium.org, tdresser@chromium.org
On 2016/03/30 10:48:12, Will Shackleton wrote: > Having read http://www.x.org/archive/X11R7.7/doc/man/man3/XIQueryDevice.3.xhtml > a bit more I think I need an extra check in here---we need to check that the > XIScrollFlagNoEmulation flag is not set before reverting to an old scroll device > like this (or else no scroll events will come through). I'm currently away from > my desktop / build server but I'll be back at it in two weeks. If this CL is > needed more urgently than that (I could make this change in a second CL) I can > clone and build on my laptop. I've done this now---there's now a check for XIScrollFlagNoEmulation, and I've moved the check to the code that actually enables smooth scrolling on a device. This fits in more with the idea that smooth scrolling is per-axis rather than per-device, as changed in https://codereview.chromium.org/1896483002/ and as fixed for devices with per-axis valuator configs in https://codereview.chromium.org/1891303002/
https://codereview.chromium.org/1792623003/diff/20001/ui/events/devices/x11/d... File ui/events/devices/x11/device_data_manager_x11.cc (right): https://codereview.chromium.org/1792623003/diff/20001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.cc:782: } Add a comment here. The code could be more readable like so: bool x11_sends_legacy_button_events = (scroll_class_info->flags & XIScrollFlagNoEmulation) == 0; if (x11_sends_legacy_button_events && std::abs(scroll_class_info->increment) <= 1.0) { return; }
On 2016/04/19 14:40:50, sadrul wrote: > https://codereview.chromium.org/1792623003/diff/20001/ui/events/devices/x11/d... > File ui/events/devices/x11/device_data_manager_x11.cc (right): > > https://codereview.chromium.org/1792623003/diff/20001/ui/events/devices/x11/d... > ui/events/devices/x11/device_data_manager_x11.cc:782: } > Add a comment here. The code could be more readable like so: > > bool x11_sends_legacy_button_events = > (scroll_class_info->flags & XIScrollFlagNoEmulation) == 0; > if (x11_sends_legacy_button_events && > std::abs(scroll_class_info->increment) <= 1.0) { > return; > } Will do. I'll check a couple of other things as well: * https://codereview.chromium.org/1883853002/ might have fixed one of the things this tried to fix. This CL is still useful as it doesn't "swap the scroll directions" for people with weird legacy X server configs and non smooth mice, but the problem with first scroll not being recognised may have been fixed. * Your comment on https://codereview.chromium.org/1891303002/ may have some implications here too.
On 2016/04/19 14:46:43, Will Shackleton wrote: > On 2016/04/19 14:40:50, sadrul wrote: > > > https://codereview.chromium.org/1792623003/diff/20001/ui/events/devices/x11/d... > > File ui/events/devices/x11/device_data_manager_x11.cc (right): > > > > > https://codereview.chromium.org/1792623003/diff/20001/ui/events/devices/x11/d... > > ui/events/devices/x11/device_data_manager_x11.cc:782: } > > Add a comment here. The code could be more readable like so: > > > > bool x11_sends_legacy_button_events = > > (scroll_class_info->flags & XIScrollFlagNoEmulation) == 0; > > if (x11_sends_legacy_button_events && > > std::abs(scroll_class_info->increment) <= 1.0) { > > return; > > } > > Will do. I'll check a couple of other things as well: > * https://codereview.chromium.org/1883853002/ might have fixed one of the things > this tried to fix. This CL is still useful as it doesn't "swap the scroll > directions" for people with weird legacy X server configs and non smooth mice, > but the problem with first scroll not being recognised may have been fixed. The mentioned CL didn't fix this, so this CL is still necessary to avoid the first scroll step being missed. > * Your comment on https://codereview.chromium.org/1891303002/ may have some > implications here too. See comments there---I don't think we should consider XISelectEvents in these CLs.
https://codereview.chromium.org/1792623003/diff/40001/ui/events/devices/x11/d... File ui/events/devices/x11/device_data_manager_x11.cc (right): https://codereview.chromium.org/1792623003/diff/40001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.cc:778: // then use that. s/then use that/then use the xinput1 resolution instead/ https://codereview.chromium.org/1792623003/diff/40001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.cc:781: return; Does this need to reset some state of |info|?
https://codereview.chromium.org/1792623003/diff/40001/ui/events/devices/x11/d... File ui/events/devices/x11/device_data_manager_x11.cc (right): https://codereview.chromium.org/1792623003/diff/40001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.cc:778: // then use that. On 2016/04/22 02:42:23, sadrul wrote: > s/then use that/then use the xinput1 resolution instead/ Done. https://codereview.chromium.org/1792623003/diff/40001/ui/events/devices/x11/d... ui/events/devices/x11/device_data_manager_x11.cc:781: return; On 2016/04/22 02:42:22, sadrul wrote: > Does this need to reset some state of |info|? No - the |info| fields are reset upon call to this method---it's only ever called to fill the |info| object in from a clean state.
lgtm
The CQ bit was checked by w.shackleton@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1792623003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1792623003/60001
Message was sent while issue was closed.
Description was changed from ========== Changed DeviceDataManagerX11 to use xinput1 for low resolution scroll devices I added functionality to DeviceDataManagerX11 that uses xinput1 for devices that gain no extra precision from using xinput2. This means that for devices that don't ouput two scroll events on focus of a window (which seems to be the case for classic mice driven by evdev) we don't experience a missed scroll event on focus. BUG=593453 ========== to ========== Changed DeviceDataManagerX11 to use xinput1 for low resolution scroll devices I added functionality to DeviceDataManagerX11 that uses xinput1 for devices that gain no extra precision from using xinput2. This means that for devices that don't ouput two scroll events on focus of a window (which seems to be the case for classic mice driven by evdev) we don't experience a missed scroll event on focus. BUG=593453 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Changed DeviceDataManagerX11 to use xinput1 for low resolution scroll devices I added functionality to DeviceDataManagerX11 that uses xinput1 for devices that gain no extra precision from using xinput2. This means that for devices that don't ouput two scroll events on focus of a window (which seems to be the case for classic mice driven by evdev) we don't experience a missed scroll event on focus. BUG=593453 ========== to ========== Changed DeviceDataManagerX11 to use xinput1 for low resolution scroll devices I added functionality to DeviceDataManagerX11 that uses xinput1 for devices that gain no extra precision from using xinput2. This means that for devices that don't ouput two scroll events on focus of a window (which seems to be the case for classic mice driven by evdev) we don't experience a missed scroll event on focus. BUG=593453 Committed: https://crrev.com/b794998819088f76b4cf44c8db6940240c563cf4 Cr-Commit-Position: refs/heads/master@{#389117} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/b794998819088f76b4cf44c8db6940240c563cf4 Cr-Commit-Position: refs/heads/master@{#389117} |
