[Touchpad KCM] Do not crash in case there is no touchpad
ClosedPublic

Authored by beischer on Jun 18 2019, 6:00 PM.

Details

Diff Detail

Repository
R119 Plasma Desktop
Branch
bug-408325 (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 12948
Build 12966: arc lint + arc unit
beischer created this revision.Jun 18 2019, 6:00 PM
Restricted Application added a project: Plasma. · View Herald TranscriptJun 18 2019, 6:00 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
beischer requested review of this revision.Jun 18 2019, 6:00 PM
ngraham retitled this revision from [Touchpad KCM] Do not crash in case there is no touchpad. to [Touchpad KCM] Do not crash in case there is no touchpad.Jun 18 2019, 6:02 PM
ngraham edited the summary of this revision. (Show Details)
beischer edited the summary of this revision. (Show Details)Jun 18 2019, 6:09 PM
davidedmundson accepted this revision.Jun 18 2019, 6:26 PM
davidedmundson added a subscriber: davidedmundson.

Heh, so we show a backend that will do nothing instead? That doesn't sound like the right long term approach.

Will do for backporting into stable.

This revision is now accepted and ready to land.Jun 18 2019, 6:26 PM

On Wayland it shows a message that no touchpad is connected. Would it be possible on X to determine if a to-be-connected touchpad would use libinput or Xlib (although the use case is pretty rare of a detechable touchpad)?

Roman, with this patch the box with 'No touchpad found' is shown under X11, too:

Since I'm not the author of the code my intention was to fix the bug with as little side effects as possible. In Plasma 5.15 the code created a TouchpadConfigXlib instance in case there was no touchpad, so that's what I went with here. If the user plugs in a device later, it looks like that should trigger XlibBackend::devicePlugged() which will reset the mode to the correct mode (see findTouchpad()). The next time the KCM touchpad is opened it should then show the correct set of (activated) controls.

Roman, with this patch the box with 'No touchpad found' is shown under X11, too:

Since I'm not the author of the code my intention was to fix the bug with as little side effects as possible. In Plasma 5.15 the code created a TouchpadConfigXlib instance in case there was no touchpad, so that's what I went with here.

Yea, didn't mean this patch shouldn't go in. Was just some contemplating on what's the strategy moving forward.

If the user plugs in a device later, it looks like that should trigger XlibBackend::devicePlugged() which will reset the mode to the correct mode (see findTouchpad()). The next time the KCM touchpad is opened it should then show the correct set of (activated) controls.

Ok, that would be even better. Only drawback is then that it shows the "wrong" interface in case libinput is used until a touchpad is plugged, but that's such an uncommon case that I don't think we should invest much more time into it. On the other side since libinput is the default backend in the future it might make more sense to show the libinput one per default if no touchpad is plugged in. Would this also be possible or is the current X libinput backend missing this funcitonality? At least from user perspective it could be confusing to see the non-default backend on the workstation without touchpad and something completely different on his laptop.

Ok, that would be even better. Only drawback is then that it shows the "wrong" interface in case libinput is used until a touchpad is plugged, but that's such an uncommon case that I don't think we should invest much more time into it. On the other side since libinput is the default backend in the future it might make more sense to show the libinput one per default if no touchpad is plugged in. Would this also be possible or is the current X libinput backend missing this funcitonality? At least from user perspective it could be confusing to see the non-default backend on the workstation without touchpad and something completely different on his laptop.

I think one could set m_plugin to "TouchpadConfigLibinput(this, backend)" (as would be the case with a libinput touchpad). I just tried that, this produces the following result:

That doesn't look right to me? Maybe because TouchpadConfigContainer::kcmInit() is a no-op (first because the mode is Unset, so neither of the two if branches triggers, but even if that is fixed it's a noop because m_device is null in the xlibbackend).

That doesn't look right to me? Maybe because TouchpadConfigContainer::kcmInit() is a no-op (first because the mode is Unset, so neither of the two if branches triggers, but even if that is fixed it's a noop because m_device is null in the xlibbackend).

What does not look right to you there? Are the controls greyed out?

What does not look right to you there? Are the controls greyed out?

  1. Controls not disabled
  2. "Two-finger-tap" radio buttons are missing their labels
  3. Middle-click label missing its radio buttons

And then of course, unrelated to this patch or bug, is https://bugs.kde.org/show_bug.cgi?id=407464

  1. Controls not disabled
  2. "Two-finger-tap" radio buttons are missing their labels
  3. Middle-click label missing its radio buttons

    And then of course, unrelated to this patch or bug, is https://bugs.kde.org/show_bug.cgi?id=407464

Yes, thats it. I think the libinput widget depends on the specifics of the device (of which there is none)? I would go with the version I initially proposed personally (at least for now).

ngraham accepted this revision.Jun 19 2019, 10:24 AM

Shall we land this now and refine the behavior later?

romangg accepted this revision.Jun 19 2019, 4:29 PM

Yes, let's do that. Add a comment to the code that we fallback to evdev for now if no touchpad is connected and we want to show instead libinput in the future when the issues have been resolved.

This revision was automatically updated to reflect the committed changes.