[libinput-touchpad-kcm] Use wayland specific touchpad KCM UI when libinput is used on X11
ClosedPublic

Authored by atulbi on Apr 1 2019, 8:23 PM.

Details

Summary

Earlier when libinput was used on X11, XInput specific UI was shown, due to which most of the options were grayed out.
Now we show libinput specific UI that is already used on wayland if libinput driver is used on X11.

  • loaded all libinput properties that are available on X11.
  • exposed all Q_PROPERTY to Libinput specific UI.

FEATURE: 387153
FEATURE: 392709
FIXED-IN: 5.16.0

Diff Detail

Repository
R119 Plasma Desktop
Branch
arcpatch-D20186
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 11766
Build 11784: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
atulbi updated this revision to Diff 55251.Apr 2 2019, 12:05 AM
  • Avoid Setup Backend two times

Got to Know a lot about how X works with input handling. XD

I happen to be the ‘accidental’ maintainer of the Touchpad KCM. I’d be more than happy to hand that hat over if you wish to maintain this code. :-D

atulbi updated this revision to Diff 55277.Apr 2 2019, 1:29 PM
  • Code Cleanup and resolved unexpected behaviour
atulbi added a comment.Apr 3 2019, 9:03 PM

Got to Know a lot about how X works with input handling. XD

I happen to be the ‘accidental’ maintainer of the Touchpad KCM. I’d be more than happy to hand that hat over if you wish to maintain this code. :-D

Hey, I would be happy to take that responsibility.

I happen to be the ‘accidental’ maintainer of the Touchpad KCM. I’d be more than happy to hand that hat over if you wish to maintain this code. :-D

Hey, I would be happy to take that responsibility.

Fantastic.

@ngraham @jriddell Would you let me know the technicalities to make Atul maintainer of Touchpad KCM? I couldn't find a guideline in the wiki/techbase.

I happen to be the ‘accidental’ maintainer of the Touchpad KCM. I’d be more than happy to hand that hat over if you wish to maintain this code. :-D

Hey, I would be happy to take that responsibility.

Fantastic.

@ngraham @jriddell Would you let me know the technicalities to make Atul maintainer of Touchpad KCM? I couldn't find a guideline in the wiki/techbase.

Wherever you're currently listed, remove your name and replace it with Atul's. :) The only place I can find it on Bugzilla. I've gone ahead and made @atulbi the default assignee for bugs to the Touchpad-KCM product.

@atulbi is anything special needed to compile the KCM? After compiling plasma-desktop with your patch, no touchpad shared library was produced.

romangg requested changes to this revision.Apr 4 2019, 11:25 PM

Nice work. Most of the stuff in libinputtouchpad.cpp and its header file are copy-paste from the Wayland backend. It would make sense to have a new abstract parent class such that the code is shared.

kcms/touchpad/src/backends/x11/xlibbackend.h
76

Put the definition in the cpp file.

kcms/touchpad/src/kcm/libinput/touchpadconfiglibinput.cpp
54–55

Put it in the constructor initializer list.

kcms/touchpad/src/touchpadbackend.h
44

Such a member varialbe shouldn't be public.

55

Unnecessary changes. Revert these pls to keep the diff size in check.

This revision now requires changes to proceed.Apr 4 2019, 11:25 PM

Wherever you're currently listed, remove your name and replace it with Atul's. :) The only place I can find it on Bugzilla. I've gone ahead and made @atulbi the default assignee for bugs to the Touchpad-KCM product.

@atulbi Congratulations, you’re now the maintainer of Touchpad KCM. In certain countries, there’s something called “inheritance tax” and you just inherited a few dozen bugs. Don’t despair, many of them should be fixed by now and some will be fixed by this commit.

ngraham edited the summary of this revision. (Show Details)Apr 5 2019, 12:55 PM
atulbi added a comment.Apr 5 2019, 2:54 PM

I happen to be the ‘accidental’ maintainer of the Touchpad KCM. I’d be more than happy to hand that hat over if you wish to maintain this code. :-D

Hey, I would be happy to take that responsibility.

Fantastic.

@ngraham @jriddell Would you let me know the technicalities to make Atul maintainer of Touchpad KCM? I couldn't find a guideline in the wiki/techbase.

Wherever you're currently listed, remove your name and replace it with Atul's. :) The only place I can find it on Bugzilla. I've gone ahead and made @atulbi the default assignee for bugs to the Touchpad-KCM product.

@atulbi is anything special needed to compile the KCM? After compiling plasma-desktop with your patch, no touchpad shared library was produced.

I've only made changes to the existing files.

atulbi added a comment.Apr 5 2019, 2:58 PM

Wherever you're currently listed, remove your name and replace it with Atul's. :) The only place I can find it on Bugzilla. I've gone ahead and made @atulbi the default assignee for bugs to the Touchpad-KCM product.

@atulbi Congratulations, you’re now the maintainer of Touchpad KCM. In certain countries, there’s something called “inheritance tax” and you just inherited a few dozen bugs. Don’t despair, many of them should be fixed by now and some will be fixed by this commit.

Many of the bugs were created during my school days XD

atulbi updated this revision to Diff 55532.Apr 6 2019, 9:33 AM
  • Resolved issues

Nice work. Most of the stuff in libinputtouchpad.cpp and its header file are copy-paste from the Wayland backend. It would make sense to have a new abstract parent class such that the code is shared.

Initially, I also wanted to implement it that way, but it'll mess up existing code, and especially because LibinputTouchpad class already extends XlibTouchpad class. I wanted to have minimal changes to existing code.

romangg added a comment.EditedApr 12 2019, 10:13 AM

Nice work. Most of the stuff in libinputtouchpad.cpp and its header file are copy-paste from the Wayland backend. It would make sense to have a new abstract parent class such that the code is shared.

Initially, I also wanted to implement it that way, but it'll mess up existing code, and especially because LibinputTouchpad class already extends XlibTouchpad class.

Can it multi-inherit? Or split out the common parts into a helper-class.

I wanted to have minimal changes to existing code.

Make a two patch series: first patch would be creating the parent class and let current Wayland backend inherit it. Second patch would be creating X11/libinput child class.

@atulbi ping! Does Roman's request make sense? In general I think it does make sense to do code clean-up in a separate patch first.

@atulbi ping! Does Roman's request make sense? In general I think it does make sense to do code clean-up in a separate patch first.

Totally!! I'm working on it... Currently facing some error. I'll contact Roman for help

@atulbi ping! Does Roman's request make sense? In general I think it does make sense to do code clean-up in a separate patch first.

Totally!! I'm working on it... Currently facing some error. I'll contact Roman for help

You can ping me on IRC. Handle/nick there same as here.

@atulbi were you able to get in touch with @romangg? Can I help in any way?

The feature freeze for Plasma 5.16 is in a little over two weeks (https://community.kde.org/Schedules/Plasma_5) and it would really nice to finally have full Libinput support in our KCM in that release. Right now we'd still have enough time for testing because of the beta period, but we'll need to get it in within the next two weeks.

@atulbi were you able to get in touch with @romangg? Can I help in any way?

The feature freeze for Plasma 5.16 is in a little over two weeks (https://community.kde.org/Schedules/Plasma_5) and it would really nice to finally have full Libinput support in our KCM in that release. Right now we'd still have enough time for testing because of the beta period, but we'll need to get it in within the next two weeks.

Sorry for the delay. My pre-university exams will end in just 2 days. I'll continue the work as soon as exams are over. I'm already halfway through.
I've found the solution to the problem though 😄 . Now it should not take much time to complete.

Wonderful! I'll stop bugging you now. :)

atulbi updated this revision to Diff 57508.May 3 2019, 7:07 PM
  • Added new abstract class LibinputCommon
  • Add overriding destructor
  • Added support for disable horizontal scrolling on Libinput On X11.
atulbi updated this revision to Diff 57519.May 4 2019, 6:01 AM
  • Removed last commit
atulbi updated this revision to Diff 57520.EditedMay 4 2019, 6:19 AM
  • Added abstract class LibinputCommon.
  • All the Q_PROPERTY are in LibinputCommon.
  • Added Disable Horizontal Support on X11 on Libinput.
atulbi updated this revision to Diff 57521.May 4 2019, 6:27 AM
  • Remove CMakeLists.txt.user
atulbi added a comment.May 9 2019, 2:19 PM
In D20186#462883, @GB_2 wrote:

Ping

Hey... Have you tried this patch ?

I'm afraid I don't think I have the technical chops to adequately review this, but hopefully @romangg does! :) Or any of the other Plasma people.

romangg requested changes to this revision.May 10 2019, 2:43 PM

Looks really nice. I have yet to try it out, but following a code review.

In general it is good practice to make class member variables private and have protected getters/setters if child classes need to interact with them. For example TouchpadBackend::m_mode. Other cases of that are all the properties in the child classes of LibinputCommon. Fixing that might be difficult though because of the template usage. And it might induce regressions in Wayland case, so I would wait with that for after next release. Some more ideas I wrote in the inline comments for that. But the m_mode thing can be changed already in this diff.

kcms/touchpad/src/backends/libinputcommon.h
19–20

Needs copyright notice (you should of course add yourself to the notice for files with non-marginal changes).

kcms/touchpad/src/backends/x11/libinputtouchpad.h
54

Equivalent and more concise is:
return m_supportsDisableEvents.avail && m_supportsDisableEvents.val ;
For other getters below as well.

Long term try to not access the member variables from child class though. You could just have these functions in the parent class and just always check if avail is true before accessing the value. This will change the behavior on Wayland backend as well, but it should be fine (let's do this after release though to not risk regressions).

57

Why is there the negation? Put the Enabled-related function in the parent class.

65

I get why you query the avail field of this and other properties in the X case and not the special property as on Wayland, but maybe there is a way to find a common approach for both?

Anyways stuff for after the release. It's fine this way at the moment.

kcms/touchpad/src/backends/x11/xlibtouchpad.h
48

Necessary?

kcms/touchpad/src/kcm/libinput/touchpadconfiglibinput.h
35

Pointer asterisk goes to the right.

kcms/touchpad/src/kcm/touchpadconfigplugin.h
32

No need for explicit anymore.

This revision now requires changes to proceed.May 10 2019, 2:43 PM
atulbi added inline comments.May 12 2019, 8:31 AM
kcms/touchpad/src/backends/x11/libinputtouchpad.h
57

X provides property for the disabled, while Wayland provides property for enabled.

atulbi updated this revision to Diff 57923.May 12 2019, 10:24 AM
  • Added Copyright notice
  • Corrected asterisk placement
atulbi updated this revision to Diff 57933.May 12 2019, 2:54 PM
  • lmr mapping working now
ngraham edited the summary of this revision. (Show Details)May 13 2019, 5:35 PM
romangg accepted this revision.May 13 2019, 8:18 PM

I tested it now. I think it's working robustly and should still go in the upcoming release. We have the beta phase to further test then.

For the next release some pointers where still work could be done:

  • Touchpads do never feature acceleration profiles I believe. Just never show the all the time disabled controls. Same holds for scroll button. The respective backend interfaces can then be cleaned out as well.
  • There is some weirdness going on if no touchpad is connected and one opens the touchpad KCM. Normally all controls should be greyed out. But at least on Wayland they are not but instead some of them are not shown.

But for now let's celebrate finally shipping a fitting libinput KCM on X as well! Great job Atul! Thank you for your hard work and for being patient in the review process.

kcms/touchpad/src/backends/libinputcommon.h
2

Most of the code in this file was written by me and is now copy-pasted. Please keep in my copyright and add your own above/below.

54

rm these comment markers

This revision is now accepted and ready to land.May 13 2019, 8:18 PM

For the next release some pointers where still work could be done:

  • Touchpads do never feature acceleration profiles I believe.

That's true for the moment, but this is planned for a future Libinput release. So maybe keep support in the backend just in case it ever materializes.

But for now let's celebrate finally shipping a fitting libinput KCM on X as well! Great job Atul! Thank you for your hard work and for being patient in the review process.

Indeed. Party time for sure!!! Also @atulbi if you don't already have commit access you should seriously consider applying--especially given that you're now the maintainer of the touchpad KCM. See https://techbase.kde.org/Contribute/Get_a_Contributor_Account

For now I can land this for you once you've adderssed the last of Roman's inline comments.

atulbi updated this revision to Diff 58082.May 14 2019, 2:20 PM

Finally no more greyed settings there \o/

Work I've planned for future releases :

  • Adding support to disable horizontal scrolling on wayland.
  • Provide settings to Disable touchpad on external mouse.

Regarding acceleration profile, maybe we should set visible to false for now.

If no touchpad is connected, we could also show a large icon in middle with "No touchpad Detected" written below it.

Regarding acceleration profile, maybe we should set visible to false for now.

+1, let's do that in this patch

If no touchpad is connected, we could also show a large icon in middle with "No touchpad Detected" written below it.

+1, though maybe material for a follow-up patch if you don't think you can get that in today. I'd really like to get this landed today or tomorrow before Plasma 5.16 branches so it's in for the beta.

atulbi updated this revision to Diff 58083.May 14 2019, 2:37 PM

Set visible to false, until accel profiles are not supported.

ngraham accepted this revision.May 14 2019, 2:41 PM

Are you ready for me to land this?

Are you ready for me to land this?

Yeah \o/
I'll take that for the next patch.

This revision was automatically updated to reflect the committed changes.
ngraham added a subscriber: fvogt.EditedMay 14 2019, 6:22 PM

@atulbi This has caused a failure in the CI:

https://build.kde.org/view/Failing/job/Plasma/job/plasma-desktop/job/kf5-qt5%20SUSEQt5.12/204/console

09:12:41  /home/jenkins/workspace/Plasma/plasma-desktop/kf5-qt5 SUSEQt5.12/kcms/touchpad/src/backends/x11/libinputtouchpad.cpp:28:10: fatal error: libinput-properties.h: No such file or directory
09:12:41   #include <libinput-properties.h>
09:12:41            ^~~~~~~~~~~~~~~~~~~~~~~
09:12:41  compilation terminated.

Looks like we need to make the libinput xorg dev package a mandatory dep (boo) or else not build the x11 Libinput KCM if it isn't found (preferred). I know it's short notice but we branch in two days; do you think you could fix this? Thanks!

romangg added a comment.EditedMay 14 2019, 8:42 PM

@atulbi This has caused a failure in the CI:

https://build.kde.org/view/Failing/job/Plasma/job/plasma-desktop/job/kf5-qt5%20SUSEQt5.12/204/console

09:12:41  /home/jenkins/workspace/Plasma/plasma-desktop/kf5-qt5 SUSEQt5.12/kcms/touchpad/src/backends/x11/libinputtouchpad.cpp:28:10: fatal error: libinput-properties.h: No such file or directory
09:12:41   #include <libinput-properties.h>
09:12:41            ^~~~~~~~~~~~~~~~~~~~~~~
09:12:41  compilation terminated.

Looks like we need to make the libinput xorg dev package a mandatory dep (boo) or else not build the x11 Libinput KCM if it isn't found (preferred). I know it's short notice but we branch in two days; do you think you could fix this? Thanks!

Of course we miss out again on these compilation corner cases... For that see D21220.

I found a grave issue now when testing though: Saved values are not restored on Reboot. Are they saved at all to a config file?

EDIT: No, they are not. In X11 + evdev the KCM does this, not the backend. We need to:

  1. add config file write and load functions to the X11-libinput backend
  2. add a hook to kcm_init such that the load function is called on startup.

@atulbi: Do you have time to try this tomorrow?

@atulbi This has caused a failure in the CI:

https://build.kde.org/view/Failing/job/Plasma/job/plasma-desktop/job/kf5-qt5%20SUSEQt5.12/204/console

09:12:41  /home/jenkins/workspace/Plasma/plasma-desktop/kf5-qt5 SUSEQt5.12/kcms/touchpad/src/backends/x11/libinputtouchpad.cpp:28:10: fatal error: libinput-properties.h: No such file or directory
09:12:41   #include <libinput-properties.h>
09:12:41            ^~~~~~~~~~~~~~~~~~~~~~~
09:12:41  compilation terminated.

Looks like we need to make the libinput xorg dev package a mandatory dep (boo) or else not build the x11 Libinput KCM if it isn't found (preferred). I know it's short notice but we branch in two days; do you think you could fix this? Thanks!

Whoops...
If it's just because of "libinput-properties.h", then we could replace all macros with their property name, then there is no need for libinput-properties.h .

@atulbi This has caused a failure in the CI:
[...]

Looks like we need to make the libinput xorg dev package a mandatory dep (boo) or else not build the x11 Libinput KCM if it isn't found (preferred). I know it's short notice but we branch in two days; do you think you could fix this? Thanks!

Whoops...
If it's just because of "libinput-properties.h", then we could replace all macros with their property name, then there is no need for libinput-properties.h .

This was already fixed in 8b1c894ef. What still needs to be done is:

I found a grave issue now when testing though: Saved values are not restored on Reboot. Are they saved at all to a config file?

EDIT: No, they are not. In X11 + evdev the KCM does this, not the backend. We need to:

  1. add config file write and load functions to the X11-libinput backend
  2. add a hook to kcm_init such that the load function is called on startup.

    @atulbi: Do you have time to try this tomorrow?

I found a grave issue now when testing though: Saved values are not restored on Reboot. Are they saved at all to a config file?

EDIT: No, they are not. In X11 + evdev the KCM does this, not the backend. We need to:

  1. add config file write and load functions to the X11-libinput backend
  2. add a hook to kcm_init such that the load function is called on startup.

    @atulbi: Do you have time to try this tomorrow?

Yeah just starting to work on it. I'll try to complete it by today.

Also keep in mind that it's possible to have global config files in /etc/X11/xorg.conf.d/ I have one such file there myself that I made to work around the lack of this feature being implemented yet:

$  cat /etc/X11/xorg.conf.d/99-libinput.conf
Section "InputClass"
        Identifier "libinput touchpad catchall"
        MatchIsTouchpad "on"
        MatchDevicePath "/dev/input/event*"
        Driver "libinput"
        Option "ClickMethod" "clickfinger" # other option is "buttonareas"
        Option "DisableWhileTyping" "false"
        Option "AccelSpeed" "0.2"
        #Option "MiddleEmulation" "true" # Only use this with the "buttonareas" clickmethod
EndSection

Others may have similar settings. If possible, it might be nice to read these config files to determine the default option state in the KCM. Then once the user modifies anything, a new user-specific config file gets written that takes priority.

Also keep in mind that it's possible to have global config files in /etc/X11/xorg.conf.d/ I have one such file there myself that I made to work around the lack of this feature being implemented yet:

$  cat /etc/X11/xorg.conf.d/99-libinput.conf
Section "InputClass"
        Identifier "libinput touchpad catchall"
        MatchIsTouchpad "on"
        MatchDevicePath "/dev/input/event*"
        Driver "libinput"
        Option "ClickMethod" "clickfinger" # other option is "buttonareas"
        Option "DisableWhileTyping" "false"
        Option "AccelSpeed" "0.2"
        #Option "MiddleEmulation" "true" # Only use this with the "buttonareas" clickmethod
EndSection

Others may have similar settings. If possible, it might be nice to read these config files to determine the default option state in the KCM. Then once the user modifies anything, a new user-specific config file gets written that takes priority.

Is there any specific library which could read these files ??

fvogt added a comment.EditedMay 15 2019, 4:59 PM

Also keep in mind that it's possible to have global config files in /etc/X11/xorg.conf.d/ I have one such file there myself that I made to work around the lack of this feature being implemented yet:

$  cat /etc/X11/xorg.conf.d/99-libinput.conf
Section "InputClass"
        Identifier "libinput touchpad catchall"
        MatchIsTouchpad "on"
        MatchDevicePath "/dev/input/event*"
        Driver "libinput"
        Option "ClickMethod" "clickfinger" # other option is "buttonareas"
        Option "DisableWhileTyping" "false"
        Option "AccelSpeed" "0.2"
        #Option "MiddleEmulation" "true" # Only use this with the "buttonareas" clickmethod
EndSection

Others may have similar settings. If possible, it might be nice to read these config files to determine the default option state in the KCM. Then once the user modifies anything, a new user-specific config file gets written that takes priority.

Is there any specific library which could read these files ??

AFAIK this is not necessary. The configuration here simply specifies the default values for the properties used until overwritten during runtime.
So storing it in whatever format and applying them on device discovery is fine.