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

Wip/libinput support #131

Merged
merged 14 commits into from
Sep 26, 2016
Merged

Wip/libinput support #131

merged 14 commits into from
Sep 26, 2016

Conversation

whot
Copy link
Contributor

@whot whot commented Sep 13, 2016

version 2 of the pull request.

Fixes #78

Add libinput support. Done cooperatively with other drivers, so most of the bits toggle the bit that exists on the device. This should in theory even support two touchpads, one with synaptics and one with libinput, and just DTRT.

The keys don't all map into libinput config options, so I took a best guess where needed.

I wouldn't mind if someone could do a bit more testing though, happy to merge/squash any patches.

libinput doesn't use device types, what is considered a touchpad is up to the
caller. We consider anything with tapping a touchpad.
Easier to read and removes the current bug where some checks for a property
would create it, negating the check to begin with.
Move it up too so we can re-use it from elsewhere and don't have to move it
later. And while we're moving this code anyway, fix the indentation to match
the rest of the code.

No functional changes.
Use one with the extra device_is_touchpad check, one for generic boolean
property
Note that libinput does not allow for both edge and twofinger scrolling to be
enabled simultaneously. We prefer twofinger scrolling. The same goes for
horizontal scrolling, it picks the setting for whatever scroll method we
applied.
libinput does not allow for configuration of the click actions, the options
are just "software buttons" or "clickfinger". If any clickfinger actions are
set, we enable clickfinger, otherwise software buttons.
We ignore the "sensitivity" setting in libinput, we only have an accel setting
here
Excluding touchpad devices since the old code only applied to evdev devices.
@whot
Copy link
Contributor Author

whot commented Sep 13, 2016

For testing: xinput watch-props "" and then toggle the various UI elements. If the property on the driver changes in the correct manner, then the bits work correctly. Doesn't work for all options though, e.g. disable-while-typing, left-handed and accel settings aren't properties on the legacy drivers

@clefebvre
Copy link
Member

First of all, many thanks for this. I really like the way you supported both synaptics and libinput here.

I tested as much as I could and I couldn't find regressions.

In Linux Mint, libinput takes over from synaptics for all touchpad devices as soon as it is installed.

When libinput isn't installed, all options in Cinnamon work properly and affect the synaptics driver. This is still true after the changes brought by this PR.

When libinput is installed, the touchpad continues to work but the options in Cinnamon no longer work and the touchpad is no longer configurable. This was due to the lack of libinput support which this PR addresses. With the changes brought by this PR, most options work. Those which don't are probably failing due to limitations inherent to the libinput driver itself.

This is therefore ready to merge. I hope we'll be able to improve the mouse module in cinnamon settings so that it can show/hide relevant configuration options based on the driver being used.

@monsta
Copy link
Contributor

monsta commented Dec 22, 2016

@whot: I started adapting this to mate-settings-daemon, and there are some things I have some questions about... can you please clarify these things?

  • 8551023: setting tap to click for libinput doesn't take "left handed" setting into account... is that ok?

  • c35f952: well, this is related to previous question... setting "left handed" property on a Synaptics touchpad also includes calling set_tap_to_click for it to swap left and right buttons if necessary, but it's not done for libinput.

  • c35f952 and 1e7d504: here property_exists_on_device is used instead of property_from_name to check if a device is managed by libinput - wouldn't using property_from_name be enough?

@whot
Copy link
Contributor Author

whot commented Dec 22, 2016

sure.

  • in libinput tapping is handled differently so you don't have to worry about left-handed, there is no crazy button mapping needed like in synaptics, libinput will handle this for you, same applies to your second comment
  • properties names are global and device independent, once initialised by any device it exists until the end of the session. so if you have (had) one device with libinput, property_from_name always returns a valid property, even if there is no device currently present that actually uses that property. that's why you need to check for the property on the device itself

@monsta
Copy link
Contributor

monsta commented Dec 23, 2016

Ok, got it. But then... shouldn't property_exists_on_device be used in other functions instead of property_from_name as well?

@monsta
Copy link
Contributor

monsta commented Dec 23, 2016

Side note: I've noticed that property_exists_on_device doesn't close the device in the end...

@whot
Copy link
Contributor Author

whot commented Dec 30, 2016

oops, yeah, the missing xdevice_close() is a bug that needs to be fixed.

A quick check of the property_from_name() usage: yes, ideally they'll all use property_exists_on_device() everywhere including in the old code. right now, I think the only issues that could arise is when you have evdev + libinput simultaneously active or synaptics + libinput. But because changing properties is always "get property -> change property" the "get" part will fail and we bail, so no harm done. So right now, it's a cosmetic issue.

monsta added a commit that referenced this pull request Jan 2, 2017
@monsta
Copy link
Contributor

monsta commented Jan 2, 2017

Ok, thanks.
I've submitted a fix for device closing in #147.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants