Rationalise input configuration options #3432

AlanGriffiths · 2024-06-19T17:08:15Z

Let's tidy up before we release 2.18 with them

codecov · 2024-06-19T17:25:40Z

Codecov Report

Attention: Patch coverage is 54.40000% with 57 lines in your changes missing coverage. Please review.

Project coverage is 77.23%. Comparing base (688d231) to head (ded4f92).
Report is 13 commits behind head on main.

Files	Patch %	Lines
src/miral/input_device_config.cpp	63.33%	33 Missing ⚠️
src/server/input/default_input_device_hub.cpp	31.57%	13 Missing ⚠️
src/common/input/mir_touchpad_config.cpp	26.66%	11 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3432      +/-   ##
==========================================
- Coverage   77.24%   77.23%   -0.01%     
==========================================
  Files        1071     1071              
  Lines       68467    68483      +16     
==========================================
+ Hits        52885    52893       +8     
- Misses      15582    15590       +8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Saviq

While here, how about a reference page for input configuration?

Saviq

I believe we're missing:

include/common/mir/input/mir_touchpad_config.h

AlanGriffiths · 2024-06-20T11:40:33Z

While here, how about a reference page for input configuration?

I first want consensus on the naming

Saviq

  --mouse-scroll-speed-scale arg        Scales mice scroll events, use negative
                                        values for natural scrolling
  --touchpad-scroll-speed-scale arg     Scales touchpad scroll events, use 
                                        negative values for natural scrolling

-speed-scale is a mouthful… -speed alone? -factor? Could also use a note on accepted values.

Saviq · 2024-06-20T12:18:34Z

For reference, this is the full current --help output:

  --mouse-cursor-acceleration arg       Select acceleration profile for mice   
                                        and trackballs [none, adaptive]
  --mouse-cursor-acceleration-bias arg  Constant factor (+1) to velocity or 
                                        bias to the acceleration curve within 
                                        the range [-1.0, 1.0] for mice
  --mouse-scroll-speed-scale arg        Scales mice scroll events, use negative
                                        values for natural scrolling
  --touchpad-disable-while-typing arg   Disable touchpad while typing on    
                                        keyboard configuration [true, false]   
  --touchpad-tap-to-click arg           Enable or disable tap-to-click on this 
                                        device, with a default mapping of 1, 2,
                                        3 finger tap mapping to left, right, 
                                        middle click, respectively [true,      
                                        false]     
  --touchpad-cursor-acceleration arg    Select acceleration profile for 
                                        touchpads [none, adaptive]             
  --touchpad-cursor-acceleration-bias arg                                                                                  
                                        Constant factor (+1) to velocity or 
                                        bias to the acceleration curve within 
                                        the range [-1.0, 1.0] for touchpads
  --touchpad-scroll-speed-scale arg     Scales touchpad scroll events, use 
                                        negative values for natural scrolling
  --touchpad-scroll-mode arg            Select scroll mode for touchpads: 
                                        [edge, two-finger, button-down]
  --touchpad-click-mode arg             Select click mode for touchpads: 
                                        [{area, finger-count}]
  --touchpad-middle-mouse-button-emulation arg
                                        Converts a simultaneous left and right 
                                        button click into a middle button

Saviq · 2024-06-20T12:20:28Z

  --touchpad-tap-to-click arg           Enable or disable tap-to-click on this 
                                        device, with a default mapping of 1, 2,

Should we say default, if there's no way to change that?

Saviq · 2024-06-20T12:30:31Z

And the corresponding logs:

Apple Inc. Apple Internal Keyboard / Trackpad, capabilities={keyboard|alpha_numeric}
bcm5974, capabilities={pointer|touchpad}, pointer_config={handedness=right,cursor-acceleration=adaptive,cursor-acceleration-bias=0.00,scroll-hscale=1.00,scroll-vscale=1.00}, touchpad_config={click-mode=finger-count|scroll-mode=two-finger|disable-while-typing}
Logitech Wireless Mouse, capabilities={pointer}, pointer_config={handedness=right,cursor-acceleration=none,cursor-acceleration-bias=1.00,scroll-hscale=1.00,scroll-vscale=1.00}
Logitech Wireless Keyboard PID:4023, capabilities={keyboard|alpha_numeric}

Saviq · 2024-06-20T12:32:28Z

We have scroll scale split into vertical and horizontal in the log (and calling it just scroll-scale, no -speed), but we only allow a single value input. Do we want to do anything about that?

Saviq · 2024-06-20T12:34:46Z

  --touchpad-click-mode arg             Select click mode for touchpads: 
                                        [{area, finger-count}]

Why the curly braces?

Also, libinput calls this "clickfinger" rather than "finger-count", we probably should stay close to their nomenclature?

AlanGriffiths · 2024-06-21T09:29:15Z

We have scroll scale split into vertical and horizontal in the log (and calling it just scroll-scale, no -speed), but we only allow a single value input. Do we want to do anything about that?

Do you have a preference? I'd go for specifying vspeed and hspeed but...

AlanGriffiths · 2024-06-21T11:59:39Z

handedness

disable-while-trackpointing

disable-while-trackpointing would mean changing mir::input::TouchpadSettings - which is a (badly designed for stability) part of the platform ABI. Not sure we want that now

Saviq

Do you have a preference? I'd go for specifying vspeed and hspeed but...

I suppose there could be devices that have different default scroll speeds on the two axes, so we should keep it?

And we could accept a float or a tuple of floats?

src/miral/input_device_config.cpp

Saviq · 2024-06-26T17:10:02Z

src/miral/input_device_config.cpp

-char const* const mouse_scroll_speed_scale_opt = "mouse-scroll-speed-scale";
+char const* const mouse_vscroll_speed_opt = "mouse-scroll-vspeed";
+char const* const mouse_hscroll_speed_opt = "mouse-scroll-hspeed";
 char const* const touchpad_cursor_acceleration_opt = "touchpad-cursor-acceleration";
 char const* const touchpad_cursor_acceleration_bias_opt = "touchpad-cursor-acceleration-bias";
-char const* const touchpad_scroll_speed_scale_opt = "touchpad-scroll-speed-scale";
+char const* const touchpad_vscroll_speed_opt = "touchpad-vscroll-speed";
+char const* const touchpad_hscroll_speed_opt = "touchpad-hscroll-speed";


Oh no, don't like that… 99% just need the same scale for both h/v, I'd rather stick to a single float, or maybe:

--mouse-scroll-speed {<hvscale>|<hscale>,<vscale>}

Maybe: use --mouse-scroll-speed for both unless --mouse-horizontal-scroll-speed-override is also set?

But then… what if you only need vscroll changed? I suppose my proposal also forced you to give both, but in a more obvious way…

Maybe: use --mouse-scroll-speed for both unless --mouse-horizontal-scroll-speed-override and/or --mouse-vertical-scroll-speed-override is also set?

And what if you set both -overrides, but not -speed? XD

My vote is on the above, but maybe with "x" as separator.

--mouse-scroll-speed {<hvscale>|<hscale>x<vscale>} # e.g. --mouse-scroll-speed 1.2 --mouse-scroll-speed -2 --mouse-scroll-speed 1x-1 --mouse-scroll-speed -1.2x2

"But then… what if you only need vscroll changed?"

Still nicer IMO than the -vertical-override option…

--mouse-scroll-speed 1x2 # or, but I don't think so, certainly not with `x` as separator --mouse-scroll-speed x2 --mouse-scroll-speed 2x

Actually, there's one thing that needs consideration - in theory devices could have different defaults? So it may be important to have a way to only specify one axis? Or will the default always be 1.0?

The default is always 1F, it isn't something libinput implements

Saviq · 2024-06-26T17:16:33Z

This is how things look now:

  --mouse-handedness arg                Select mouse laterality: [right, left]
  --mouse-cursor-acceleration arg       Select acceleration profile for mice 
                                        and trackballs [none, adaptive]   
  --mouse-cursor-acceleration-bias arg  Constant factor (+1) to velocity or 
                                        bias to the acceleration curve within 
                                        the range [-1.0, 1.0] for mice
  --mouse-scroll-vspeed arg             Scales mouse vertical scroll, use 
                                        negative values for natural scrolling  
  --mouse-scroll-hspeed arg             Scales mouse horizontal scroll, use 
                                        negative values for natural scrolling
  --touchpad-disable-while-typing arg   Disable touchpad while typing on 
                                        keyboard configuration [true, false]
  --touchpad-disable-with-external-mouse arg                                                                                       
                                        Disable touchpad if an external pointer
                                        device is plugged in [true, false]  
  --touchpad-tap-to-click arg           Enable or disable tap-to-click on this 
                                        device, with 1, 2, 3 finger tap mapping
                                        to left, right, middle click,  
                                        respectively [true, false]  
  --touchpad-cursor-acceleration arg    Select acceleration profile for        
                                        touchpads [none, adaptive]
  --touchpad-cursor-acceleration-bias arg                
                                        Constant factor (+1) to velocity or    
                                        bias to the acceleration curve within 
                                        the range [-1.0, 1.0] for touchpads
  --touchpad-vscroll-speed arg          Scales touchpad vertical scroll, use 
                                        negative values for natural scrolling
  --touchpad-hscroll-speed arg          Scales touchpad horizontal scroll, use 
                                        negative values for natural scrolling
  --touchpad-scroll-mode arg            Select scroll mode for touchpads: 
                                        [edge, two-finger, button-down]
  --touchpad-click-mode arg             Select click mode for touchpads: [none,
                                        area, clickfinger]
  --touchpad-middle-mouse-button-emulation arg
                                        Converts a simultaneous left and right 
                                        button click into a middle button

… and text

…ical

RAOF

I think there are a couple of options that could be improved with the grammatical suggestions, and a couple of other things, but in the broad strokes this option scheme looks sensible to me.

src/miral/input_device_config.cpp

AlanGriffiths · 2024-06-28T10:22:21Z

This is how these options now look:

  --mouse-handedness arg                Select mouse laterality: [right, left]
  --mouse-cursor-acceleration arg       Select acceleration profile for mice 
                                        and trackballs [none, adaptive]
  --mouse-cursor-acceleration-bias arg  Set the pointer acceleration speed of 
                                        mice within a range of [-1.0, 1.0]
  --mouse-scroll-speed arg              Scales mouse scroll. Use negative 
                                        values for natural scrolling
  --mouse-horizontal-scroll-speed-override arg
                                        Scales mouse horizontal scroll. Use 
                                        negative values for natural scrolling
  --mouse-vertical-scroll-speed-override arg
                                        Scales mouse vertical scroll. Use 
                                        negative values for natural scrolling
  --touchpad-disable-while-typing arg   Disable touchpad while typing on 
                                        keyboard configuration [true, false]
  --touchpad-disable-with-external-mouse arg
                                        Disable touchpad if an external pointer
                                        device is plugged in [true, false]
  --touchpad-tap-to-click arg           Enable or disable tap-to-click on this 
                                        device, with 1, 2, 3 finger tap mapping
                                        to left, right, middle click, 
                                        respectively [true, false]
  --touchpad-cursor-acceleration arg    Select acceleration profile for 
                                        touchpads [none, adaptive]
  --touchpad-cursor-acceleration-bias arg
                                        Set the pointer acceleration speed of 
                                        touchpads within a range of [-1.0, 1.0]
  --touchpad-scroll-speed arg           Scales touchpad scroll. Use negative 
                                        values for natural scrolling
  --touchpad-horizontal-scroll-speed-override arg
                                        Scales touchpad horizontal scroll. Use 
                                        negative values for natural scrolling
  --touchpad-vertical-scroll-speed-override arg
                                        Scales touchpad vertical scroll. Use 
                                        negative values for natural scrolling
  --touchpad-scroll-mode arg            Select scroll mode for touchpads: 
                                        [edge, two-finger, button-down]
  --touchpad-click-mode arg             Select click mode for touchpads: [none,
                                        area, clickfinger]
  --touchpad-middle-mouse-button-emulation arg
                                        Converts a simultaneous left and right 
                                        button click into a middle button

AlanGriffiths requested a review from a team as a code owner June 19, 2024 17:08

Saviq force-pushed the log-input-devices branch from 7966439 to a65965e Compare June 20, 2024 11:09

Saviq reviewed Jun 20, 2024

View reviewed changes

include/common/mir/input/mir_touchpad_config.h Outdated Show resolved Hide resolved

Saviq reviewed Jun 20, 2024

View reviewed changes

Base automatically changed from log-input-devices to main June 20, 2024 12:52

Saviq reviewed Jun 21, 2024

View reviewed changes

src/miral/input_device_config.cpp Show resolved Hide resolved

Saviq reviewed Jun 26, 2024

View reviewed changes

src/miral/input_device_config.cpp Outdated Show resolved Hide resolved

Saviq reviewed Jun 26, 2024

View reviewed changes

Saviq requested a review from RAOF June 26, 2024 17:16

AlanGriffiths added 7 commits June 27, 2024 09:35

Rationalise naming of input configuration options, variables, methods…

f16c725

… and text

Adopt names & help documentation suggested in review

2341713

Add "mouse-handedness"

4061259

Rework InputDeviceConfig::InputDeviceConfig to make changes easier

d3d4cb6

vscroll-speed and hscroll-speed options

18b1ffd

Wire up touchpad-disable-with-external-mouse

3997873

Allow generic "scroll speed" setting in addition to horizontal & vert…

102c89f

…ical

AlanGriffiths force-pushed the MIRENG-577/rationalise-input-config-options branch from 83f8650 to 102c89f Compare June 27, 2024 09:25

Fix typo

b9448b2

RAOF approved these changes Jun 28, 2024

View reviewed changes

AlanGriffiths added 3 commits June 28, 2024 09:25

Clearer descriptions

2883b96

Clearer clamping

a42ad8e

Clearer descriptions

ded4f92

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rationalise input configuration options #3432

Rationalise input configuration options #3432

AlanGriffiths commented Jun 19, 2024

codecov bot commented Jun 19, 2024 •

edited

Loading

Saviq left a comment

Saviq left a comment •

edited

Loading

AlanGriffiths commented Jun 20, 2024

Saviq left a comment

Saviq commented Jun 20, 2024

Saviq commented Jun 20, 2024

Saviq commented Jun 20, 2024

Saviq commented Jun 20, 2024

Saviq commented Jun 20, 2024 •

edited

Loading

AlanGriffiths commented Jun 21, 2024

AlanGriffiths commented Jun 21, 2024 •

edited by Saviq

Loading

Saviq left a comment

Saviq Jun 26, 2024

AlanGriffiths Jun 27, 2024

Saviq Jun 27, 2024

AlanGriffiths Jun 27, 2024

Saviq Jun 27, 2024

AlanGriffiths Jun 27, 2024

Saviq Jun 27, 2024

AlanGriffiths Jun 27, 2024

Saviq commented Jun 26, 2024

RAOF left a comment

AlanGriffiths commented Jun 28, 2024

Rationalise input configuration options #3432

Are you sure you want to change the base?

Rationalise input configuration options #3432

Conversation

AlanGriffiths commented Jun 19, 2024

codecov bot commented Jun 19, 2024 • edited Loading

Codecov Report

Saviq left a comment

Choose a reason for hiding this comment

Saviq left a comment • edited Loading

Choose a reason for hiding this comment

AlanGriffiths commented Jun 20, 2024

Saviq left a comment

Choose a reason for hiding this comment

Saviq commented Jun 20, 2024

Saviq commented Jun 20, 2024

Saviq commented Jun 20, 2024

Saviq commented Jun 20, 2024

Saviq commented Jun 20, 2024 • edited Loading

AlanGriffiths commented Jun 21, 2024

AlanGriffiths commented Jun 21, 2024 • edited by Saviq Loading

Saviq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Saviq commented Jun 26, 2024

RAOF left a comment

Choose a reason for hiding this comment

AlanGriffiths commented Jun 28, 2024

codecov bot commented Jun 19, 2024 •

edited

Loading

Saviq left a comment •

edited

Loading

Saviq commented Jun 20, 2024 •

edited

Loading

AlanGriffiths commented Jun 21, 2024 •

edited by Saviq

Loading