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

[Roman Gilg]

romangg requested changes to this revision.
romangg added a comment.
This revision now requires changes to proceed.


  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.

INLINE COMMENTS

> libinputcommon.h:1
> -/*
> - * Copyright 2017 Roman Gilg <subdiff at gmail.com>
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License as published by
> - * the Free Software Foundation; either version 2 of the License, or
> - * (at your option) any later version.
> - *
> - * This program is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> - * GNU General Public License for more details.
> - *
> - * You should have received a copy of the GNU General Public License
> - * along with this program; if not, write to the Free Software
> - * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
> - */
> -
> -#ifndef KWINWAYLANDTOUCHPAD_H
> -#define KWINWAYLANDTOUCHPAD_H
> +#ifndef LIBINPUTCOMMON_H
> +#define LIBINPUTCOMMON_H

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

> libinputtouchpad.h:54
> +    bool supportsDisableEvents() const override {
> +        return m_supportsDisableEvents.avail ? m_supportsDisableEvents.val : false;
> +    }

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).

> libinputtouchpad.h:57
> +    bool isEnabled() const override {
> +        return !m_enabled.val;
> +    }

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

> libinputtouchpad.h:65
> +    bool supportsLeftHanded() const override {
> +        return m_leftHanded.avail;
> +    }

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.

> xlibtouchpad.h:48
>  {
> +    Q_GADGET
> +

Necessary?

> touchpadconfiglibinput.h:35
>      explicit TouchpadConfigLibinput(TouchpadConfigContainer *parent,
> +                                    TouchpadBackend* backend,
>                              const QVariantList &args = QVariantList());

Pointer asterisk goes to the right.

> touchpadconfigplugin.h:32
>  public:
> -    explicit TouchpadConfigPlugin(QWidget *parent);
> +    explicit TouchpadConfigPlugin(QWidget *parent, TouchpadBackend* backend);
>      virtual ~TouchpadConfigPlugin() {}

No need for explicit anymore.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D20186

To: atulbi, ngraham, romangg, davidedmundson, #plasma
Cc: GB_2, jriddell, knambiar, plasma-devel, jraleigh, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20190510/408c86bf/attachment-0001.html>