[libinput] Add more support for touchpads in preparation for the new touchpad KCM
ClosedPublic

Authored by romangg on Nov 24 2016, 12:21 AM.

Details

Summary

This patch is made in preparation for the touchpad KCM for Wayland. Synopsis:

  • KWin has a Libinput version requirement bump to 1.5
  • new isTouchpad property to distinguish touchpads from mice
  • new lmrTapButtonMap property and its related default and support properties (since Libinput 1.5 - swap 2 finger tap: right click and 3 finger tap: middle click )
  • new disableWhileTyping property and its related default property to complement the existing support property
  • new pointerAccelerationProfile property and its related default and support properties (switch between flat and adaptive acceleration)
  • new defaultPointerAcceleration property to complement the existing pointerAcceleration property
  • save to config mechanism added for new propertys and pointerAcceleration
  • added new D-Bus interface org.kde.KWin.InputDeviceManager and method devicesSysNames in order to query all available devices
  • removed unnecessary additional D-Bus service name org.kde.KWin.InputDevice
  • changing mouse acceleration in the mouse KCM doesn't influence touchpads anymore
Test Plan

Wrote all autotests for new properties and did manual testing in Wayland and with a prototype of the new touchpad KCM.

Diff Detail

Repository
R108 KWin
Lint
Lint Skipped
Unit
Unit Tests Skipped
romangg updated this revision to Diff 8462.Nov 24 2016, 12:21 AM
romangg retitled this revision from to [libinput] Add more support for touchpads in preparation for the new touchpad KCM.
romangg updated this object.
romangg edited the test plan for this revision. (Show Details)
romangg added reviewers: KWin, Plasma.
romangg set the repository for this revision to R108 KWin.
romangg added a project: KWin.
romangg added subscribers: KWin, plasma-devel.
Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptNov 24 2016, 12:21 AM
Restricted Application added a subscriber: kwin. · View Herald Transcript
davidedmundson added inline comments.
libinput/connection.h
49

This isn't constant.
Connection::processEvents() will update m_devices

Rather than add a notify on here, common practice is generally to have a :

q_signals:

deviceAdded(QDBusObjectPath pathOfNewlyAddedObject)
deviceRemoved(QDBusObjectPath oldPath)   or some UUID

(remember to export the signals as well if you add them)

that way the KCM can avoid reparsing the list and work out what's changed.

Looks good to me

romangg updated this object.Nov 24 2016, 9:45 AM

@davidedmundson do you have any further comments on the DBus usage? if not, I would give it an "Accepted".

@davidedmundson do you have any further comments on the DBus usage? if not, I would give it an "Accepted".

At the moment @davidedmundson helps me with the dbus signals. I tried to implement them, but the problem was, that the processEvents() loop is in a different thread than the Connection instance. Because of that the signals weren't forwarded to D-Bus (hard to see).

He told me that he has an idea how to solve the problem. If this doesn't work though, I would suggest that we ignore the problem of the changing device list on plugging events. For my touchpad KCM it's in some way irrelevant anyway, because:

  1. Most touchpads aren't removable.
  2. On the rare occasion of a user plugging in or disconnecting a touchpad while being in the touchpad KCM, I would use the signal only to show a warning message to the user, that he should restart the KCM (I think it's over-engineered to implement logic to update the device list in real time in these very rare cases).

sure, don't commit the broken version, commit this and we can add the change signals afterwards

davidedmundson accepted this revision.Nov 30 2016, 4:44 PM
davidedmundson added a reviewer: davidedmundson.
davidedmundson added inline comments.
libinput/connection.cpp
159

only other comment I did have

We have the path

/org/kde/KWin/InputDevice/event0
/org/kde/KWin/InputDevice/event1
...

I would put the manager interface on the path
/org/kde/KWin/InputDevice

so it reads like a tree.

This revision is now accepted and ready to land.Nov 30 2016, 4:44 PM

To expand on the DBus problem:

The connection object resides in a thread; but emits stuff from the main thread.
DBus is generally thread safe, but QDBusAbstractAdaptor uses sender() in relaying so isn't.

add in a bouncer, things work nicely:

connect(this, &Connection::deviceRemoved, this, [this](Device* device) {
          emit deviceRemovedSysName(device->sysName());
  });

this gets implicitly queuedconnection as this is emitted in another thread and it all works. Huzzah

However, we still get a tonne of warnings because we're emitting a bunch of other signals, and internally in QDBusAbstractAdaptor we have the warning about checking for threads*before* we see if actually relays the signal.

The only solution I can see is to make an explicit adaptor.

(or we change the KCM side to use org.freedesktop.DBus.ObjectManager and forget putting anything in here)

romangg updated this revision to Diff 8696.Dec 2 2016, 1:36 PM
romangg edited edge metadata.
  • Changed D-Bus path to /org/kde/KWin/InputDevice
  • D-Bus signals for adding/removing devices

@davidedmundson: I changed my mind regarding the inclusion of the additional D-Bus signals already in this Diff because this way I can already use them in the upcoming diff of the Touchpad KCM. If you want to change their mechanism somehow in the future the names for the signals would probably still stay the same or we can then adapt to the new ones quite easily in the Touchpad KCM.

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptDec 2 2016, 1:36 PM
romangg edited projects, added Plasma; removed KWin.Dec 2 2016, 1:38 PM
romangg edited projects, added KWin; removed Plasma.
This revision was automatically updated to reflect the committed changes.
bcooksley edited projects, added Plasma; removed KWin.Dec 2 2016, 7:03 PM
bcooksley edited projects, added KWin; removed Plasma.