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 <subd...@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
