Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

libinput: ensure that we only apply touchpad options to touchpads #90635

Merged
merged 1 commit into from Jun 20, 2020
Merged

libinput: ensure that we only apply touchpad options to touchpads #90635

merged 1 commit into from Jun 20, 2020

Conversation

Twey
Copy link
Contributor

@Twey Twey commented Jun 16, 2020

Motivation for this change

It seems that the default libinputConfiguration InputClass section generated by the libinput module is intended for touchpads, and include touchpad-specific options such as TappingDragLock and (by default) ScrollMethod twofinger. These are fine when applied to touchpads, but if we wish to set them on something else they may fail, causing the option to be ignored entirely. For example, I have a Kensington Expert Mouse, one of whose four buttons I like to dedicate to being a scroll button, to allow me to use the trackball for scrolling rather than the scrolling ring, so I have an entry in services.xserver.inputClassSections like this:

    services.xserver.inputClassSections =
    [ ''
        Identifier "Kensington Expert Mouse"
        MatchProduct "Expert Mouse"
        Driver "libinput"
        Option "ScrollMethod" "button"
        Option "ScrollButton" "8"
      '' ];

Unfortunately, when Xorg loads the generated configuration file, it will first attempt to apply the libinputConfiguration section to my trackball, which states amongst other things that Option "ScrollMethod" "twofinger", generating an error like this:

Jun 16 09:59:13 ingwaz xserver-wrapper[1745]: (EE) libinput: Kensington      Kensington Expert Mouse: Failed to set scroll to twofinger

Then, my own Option "ScrollMethod" "button" is ignored.

This PR adds a condition to the libinputConfiguration block that causes it to only match trackpads (as identified and tagged by udev).

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

pwetzel
pwetzel approved these changes Jun 18, 2020
View reviewed changes
Copy link
Contributor

@pwetzel pwetzel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested with libinput enabled on:

  • computer with only mouse
  • laptop with touchpad
  • laptop with touchpad and mouse

Did the right thing in all cases, and the "Failed to set scroll to twofinger" errors are now gone. Nice!

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Jun 20, 2020

Merging based on the review of @pwetzel. Thank you.

@rnhmjoj rnhmjoj merged commit 41e1bd6 into NixOS:master Jun 20, 2020
@eadwu
Copy link
Member

eadwu commented Jun 21, 2020 •
edited

This breaks the application of the libinput configuration for touchpads (at least for me). It doesn't break libinput since that is handled separately by another configuration file I believe. So the touchpad would still work but any custom options wouldn't be applied (basically rendering the module useless).

I believe this was a configuration error on the part of the user. The ScrollMethod can be configured with services.xserver.libinput.scrollMethod and there are other ways as well to override the configuration (xorg.conf.d or figure out the order of application for the rules in the generated xserver.conf) which probably should've been tested as well beforehand.

Anyway, the proper way of identifying only touchpads would probably be MatchIsTouchpad, I'm not sure where MatchTag came from or how the tag is actually set (first few results from google didn't pull anything up).

Source: My libinput settings weren't applied and thus broke my tap-to-click, button layout, etc.

@eadwu eadwu mentioned this pull request Jun 21, 2020
Merged
10 tasks
@pwetzel
Copy link
Contributor

pwetzel commented Jun 21, 2020

I screwed up. I thought I saw good news in the X log and scrolling worked and I didn't push it any further. Sorry. I confirm that this breaks touchpad function like tap-to-click.

The approach in the PR seems like it should have worked. MatchTag is described here[1], and my laptop has a log entry like this:
xserver-wrapper[854]: (II) event5 - Elan Touchpad: is tagged by udev as: Touchpad

I just tried lowercase touchpad as the tag, still didn't work. I tried MatchIsTouchpad "on" as suggested above and that did work.

@eadwu, I think you're suggesting there's a better way to go about solving this problem? It seems like good UX to have the default libinput snippet (very much targeted at trackpads) only apply to trackpads. This was obviously the wrong way to do it, but an implementation with MatchIsTouchpad might be?

[1] https://fedoraproject.org/wiki/Input_device_configuration#InputClasses

@eadwu
Copy link
Member

eadwu commented Jun 21, 2020 •
edited

If it only applies to trackpads then it wouldn't be generic enough for libinput (and wouldn't be changeable), which covers all input devices. This may have also introduced some unintended breakages since the original mapped to all input devices and it doesn't seem like Xorg just crashes from an invalid configuration from libinput.

But yeah, I think this should've just been settled through an override, which should be possible (I'm pretty sure) with a config file like 99-custom-rules.conf in xorg.conf.d. Or if it isn't, then we should probably allow all the options to be null so that we don't set any defaults for the overrides.

For the particular issue of the custom InputClass not overriding, it might just be that it was overrided by the libinput configuration because it appears before it. Something like xserver.config = mkAfter "" might work (untested).

@Twey
Copy link
Contributor Author

Twey commented Jun 21, 2020

It's not clear to me whether the default libinput options are intended to be for general libinput or for trackpads. The configuration structure would seem to suggest it's for everything, but the actual InputClass section seems to be trackpad-specific. This means that the configuration at the moment will always produce an error if you try to configure something other than a trackpad to use libinput. Unless we have a clear goal for this config to be trackpad-specific or not, it's hard to say what should go here. Using ordering to override the InputClass might make sense (but in that case shouldn't custom InputClasses always go after the generic stuff?) but it's still not good practice to set nonsensical options for non-trackpads — if this section is meant to be generic, we should probably remove all trackpad-specific stuff from it and add a new trackpad-specific InputClass with its own config options.

@thiagokokada thiagokokada mentioned this pull request Jan 12, 2021
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pwetzel pwetzel pwetzel approved these changes

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Twey @rnhmjoj @eadwu @pwetzel