[Touchpad KCM] New KWin Wayland version
ClosedPublic

Authored by romangg on Dec 7 2016, 3:18 PM.

Details

Summary

In order to work under KWin Wayland the Touchpad KCM needed to be adjusted. In theory this should have meant only the creation of an additional backend. But since a libinput based touchpad configuration utility needs to be in many ways different than the legacy X version the decision was made to redesign the user interface aswell.

Feature summary:

  • Changes the configuration of each connected touchpad individually
  • Saves changes per device (through the KWin libinput backend) in a config file.
  • Quick switch between devices via drop-down list.
  • Loads and removes devices on the fly on hotplugging events and informs the user with animated messages.
  • Visual indication if a property is not supported by a device
  • Tooltips give additional informations about available options

Technical overview:

The KCModule was redesigned as a container, which loads on opening a X or Wayland plugin dependent on the active windowing system. In the X case it loads the legacy QWidget, which was untouched besides integration work in the new plugin structure. In the Wayland case, it loads the new QML based user interface, which was written from scratch.

The user interface then instantiates appropriate backend. In the X case it loads the old backend, which was also not changed. The Wayland backend was created aswell from scratch.

Backend:
KWinWaylandBackend queries available devices from KWin via D-Bus and creates KWinWaylandTouchpad instances for each available touchpad.

KWinWaylandTouchpad gets via D-Bus all properties from KWin, which are relevant for a touchpad and saves them in property structs. This includes: Boolean if the property is supported by the specific touchpad. Default and reset values for the generic KCModule actions. And the D-Bus name in order to write changes back later on

When requested by the KCModule, the backend sends all changed values back to KWin, which then forwards them to libinput and saves them in its config file. The backend also connect to KWin signals over D-Bus, which emit in case a touchpad gets disconnected or another one additionally connected.

Frontend:
Loads the device list from the backend and all properties of the selected touchpad. If a value gets changed in the ui, it writes directly back to the KWinWaylandTouchpad instance.

If there is no touchpad available an info message is shown and the control elements get disabled. If a touchpad is then connected, it is automatically loaded and the message vanishes. A similar message is shown when disconnecting the touchpad with currently opened configuration.

Requests for help / future work:
I need in particular feedback on my QML coding. I hope it's fine in general but there is most certainly stuff to be improved. Also the option and tooltip texts need someone else to take a look on.

The touchpad in my laptop doesn't support "Scroll-on-Button-Down". Since I would've needed to test the functionality somehow while coding, I didn't implement it yet and only did some ground work.

The hot plug handling seems a bit overengineered for touchpads, which are normally permanently installed. But most functionality of this KCM in general could be transferred to a new Wayland based Mouse KCM, which also could need an overhaul. Maybe this could be a project for a just starting contributer, since it would involve different areas of Plasma/KWin while there is already a similar example with the Touchpad KCM.

This change implements T4460.

Test Plan

Manually tested with a single touchpad in my laptop. In order to test the hot plugging I changed the reply value in KWinWaylandBackend to also gather other pointer devices. This way I could test it with multiple mice over USB.

Diff Detail

Repository
R119 Plasma Desktop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
romangg updated this revision to Diff 8848.Dec 7 2016, 3:18 PM
romangg retitled this revision from to [Touchpad KCM] New KWin Wayland version.
romangg updated this object.
romangg edited the test plan for this revision. (Show Details)
romangg added reviewers: KWin, Plasma on Wayland, Plasma.
romangg set the repository for this revision to R119 Plasma Desktop.
romangg added a project: Plasma.
romangg added a subscriber: plasma-devel.
Restricted Application edited projects, added Plasma on Wayland, KWin; removed Plasma. · View Herald TranscriptDec 7 2016, 3:18 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
romangg added a reviewer: VDG.Dec 7 2016, 3:21 PM
romangg updated this object.Dec 7 2016, 3:26 PM

A query about the dbus calls in findTouchpads().

kcms/touchpad/src/backends/kwin_wayland/kwinwaylandbackend.cpp
73

Just wondering whether these calls are synchronous and may cause delay in startup.

graesslin added inline comments.
kcms/touchpad/src/backends/kwin_wayland/kwinwaylandbackend.cpp
60–64

check qDeleteAll

61–62

in general you can delete a nullptr, so the ifcheck is not needed

73

const

80

We normally don't use the name "KWin" in user facing messages. Also I don't expect users to know what a "KCM" is ;-)

Suggestion: "Querying input devices failed. Please reopen this settings module".

85

please don't use the Q_FOREACH in new code as Qt might deprecate it.

91

const

96

I doubt a restart would help here. That should run in an error again.

108–114
return std::all_of(m_devices.constBegin(), m_devices.constEnd(),
    [] (QObject *t) { return static_cast<KWinWaylandTouchpad*>(t)->applyConfig(); });
119–125

can also be simplified with algorithm

130–136

and here as well.

Given that you have three times the same method with just one smallish difference I would even suggest to use a std::function and use one method to rule them all

141–145

std::any_of

150–155

std::find_if

188–198

std::find_if

kcms/touchpad/src/backends/kwin_wayland/kwinwaylandbackend.h
56

why is m_devices a list of QObjects and not a list of KWinWaylandTouchpad?

Also: please don't use QList any more in new code, but if possible QVector.

romangg updated this revision to Diff 9359.Dec 25 2016, 9:33 PM
romangg marked 12 inline comments as done.
Restricted Application edited projects, added Plasma; removed KWin, Plasma on Wayland. · View Herald TranscriptDec 25 2016, 9:33 PM
romangg added inline comments.Dec 25 2016, 9:35 PM
kcms/touchpad/src/backends/kwin_wayland/kwinwaylandbackend.cpp
73

Just wondering whether these calls are synchronous and may cause delay in startup.

I assume so. Should be no problem though. We need the reply values anyway.

85

Are you sure? The foreach keyword is still listed in the official Qt docu. What's the best alternative? A normal for-loop? With upcounting integer or iterator?

kcms/touchpad/src/backends/kwin_wayland/kwinwaylandbackend.h
56

getDevices() is a implementation independent virtual method of TouchpadBackend and therefore KWinWaylandBackend also needs to return a container of QObjects in its getDevices() function. Implicit conversion for the whole container doesn't work and I don't wanted to iterate over the container just to return QObjects, or is there another trick?

The reason for QList is, that we later in TouchpadConfigLibinput() set it as a model context property of the QML context, which only accepts QList. But I've done it anyway now as QVector and convert it there back to a QList with toList().

luebking added inline comments.
kcms/touchpad/src/backends/kwin_wayland/kwinwaylandbackend.cpp
85

"might deprecate it" - magic eightball wisdom ;-)

The reason is that there's now
http://en.cppreference.com/w/cpp/language/range-for

graesslin added inline comments.Dec 26 2016, 8:07 AM
kcms/touchpad/src/backends/kwin_wayland/kwinwaylandbackend.cpp
85
romangg updated this revision to Diff 9389.Dec 27 2016, 3:45 PM
romangg marked 4 inline comments as done.

Replace foreach loops.

Restricted Application edited projects, added Plasma on Wayland, KWin; removed Plasma. · View Herald TranscriptDec 27 2016, 3:45 PM
sebas added a subscriber: sebas.Dec 27 2016, 4:28 PM
sebas added inline comments.
kcms/touchpad/src/backends/kwin_wayland.cmake
2

remove commented out bits?

kcms/touchpad/src/backends/kwin_wayland/kwinwaylandbackend.cpp
47

indenting four more spaces aligns it with the above

Yes, OCD. :)

90

remove whitespace between ! and tp

kcms/touchpad/src/backends/kwin_wayland/kwinwaylandbackend.h
3

Bit of a nitpick, but the (C) doesn't really add anything. I usually leave it out.

kcms/touchpad/src/backends/kwin_wayland/kwinwaylandtouchpad.h
37

I wonder if it wouldn't be neater if the features would be an enum, and you'd have methods to check for supported and enabled features. Have you thought of this?

353

const & for the argument?

kcms/touchpad/src/kcm/libinput/components/ToolTip.qml
24

whitespace looks weird here ... perhaps group the properties, the new properties and the signal handlers in blocks?

45

Could also be hidden by using a onRunningChanged handler, that way the stop() method would 'just work'

kcms/touchpad/src/kcm/libinput/main.qml
275

magic values, a comment would be useful where the 4.5 and 5.5 come from

280

is "value changed twice" a useful condition to set initalized here?

286

same here, comment please

295

not so sure about that, it would introduce inconsistent special behaviour just in this KCM, as a slider always should react to wheel events.

546

full stop is missing at the end

kcms/touchpad/src/kcm/libinput/touchpadconfiglibinput.cpp
101

This will fall over with high dpi displays...

106

problematic for high dpi

romangg updated this revision to Diff 9407.Dec 28 2016, 2:45 AM
romangg marked 12 inline comments as done.
Restricted Application edited projects, added Plasma; removed KWin, Plasma on Wayland. · View Herald TranscriptDec 28 2016, 2:45 AM
romangg added inline comments.Dec 28 2016, 2:47 AM
kcms/touchpad/src/backends/kwin_wayland/kwinwaylandtouchpad.h
37

Haven't thought about it. The structure here is analogous to the structure in KWin's libinput counter part.

kcms/touchpad/src/kcm/libinput/components/ToolTip.qml
45

Whenever the running property is changed to false, it automatically should hide the tooltip? But what happens, when it changes to false after a normal enter or position change event and the timer triggered after its interval? Then the tooltip would get hidden again directly.

kcms/touchpad/src/kcm/libinput/main.qml
280

Sorry, I don't understand. What does "value changed twice" mean?

295

Yea, I'll remove it. I really don't like this though since you can change by accident the value, when you have the mouse cursor by chance in vertical alignment above or below the slider and then you begin scrolling with the mouse wheel and the slider moves under the cursor.

Happened to me quite often, when I was testing different parts of the ui.

Clarified your questions.

My remaining comments are all non-critical so as long as you consider them, it's up to you if you change things or not. Just wanted to make sure it's not an oversight there.

kcms/touchpad/src/kcm/libinput/components/ExclGroupBox.qml
54

default should already be empty, so you should be able to leave this line out.

kcms/touchpad/src/kcm/libinput/components/ToolTip.qml
45

Right, that could lead to glitches. Good thinking!

kcms/touchpad/src/kcm/libinput/main.qml
280

What I mean is that this only prevents the rest of the codepath to run after the value has at least changed once. What if someone sets this value again before everything is properly initialized?

romangg updated this revision to Diff 9495.Dec 30 2016, 3:46 PM
romangg marked 8 inline comments as done.
Restricted Application edited projects, added Plasma on Wayland, KWin; removed Plasma. · View Herald TranscriptDec 30 2016, 3:46 PM
graesslin accepted this revision.Jan 10 2017, 4:18 PM
graesslin added a reviewer: graesslin.

code looks good to me modulo the one issue. I didn't check the qml side, though and hope the Plasma Qml experts did so.

kcms/touchpad/src/backends/kwin_wayland/kwinwaylandtouchpad.h
23

why an include of libinput.h? If you use that you also have to find libinput in CMake. Otherwise it won't compile on all systems.

This revision is now accepted and ready to land.Jan 10 2017, 4:18 PM
This revision was automatically updated to reflect the committed changes.
romangg marked an inline comment as done.