Differential D3617 Diff 10034 kcms/touchpad/src/backends/kwin_wayland/kwinwaylandbackend.cpp

Changeset View

Standalone View

View Options

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

  • This file was added.
1​/*
2​ * Copyright 2017 Roman Gilg <subdiff@gmail.com>
3​ *
4​ * This program is free software; you can redistribute it and/or modify
5​ * it under the terms of the GNU General Public License as published by
6​ * the Free Software Foundation; either version 2 of the License, or
7​ * (at your option) any later version.
8​ *
9​ * This program is distributed in the hope that it will be useful,
10​ * but WITHOUT ANY WARRANTY; without even the implied warranty of
11​ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
12​ * GNU General Public License for more details.
13​ *
14​ * You should have received a copy of the GNU General Public License
15​ * along with this program; if not, write to the Free Software
16​ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
17​ */
18​
19​#include "kwinwaylandbackend.h"
20​#include "kwinwaylandtouchpad.h"
21​
22​#include <algorithm>
23​
24​#include <KLocalizedString>
25​
26​#include <QDBusMessage>
27​#include <QDBusInterface>
28​#include <QDBusReply>
29​#include <QStringList>
30​
31​#include "logging.h"
32​
33​KWinWaylandBackend::KWinWaylandBackend(QObject *parent) :
34​ TouchpadBackend(parent)
35​{
36​
37​ m_deviceManager = new QDBusInterface (QStringLiteral("org.kde.KWin"),
38​ QStringLiteral("/org/kde/KWin/InputDevice"),
39​ QStringLiteral("org.kde.KWin.InputDeviceManager"),
40​ QDBusConnection::sessionBus(),
41​ this);
42​
43​ findTouchpads();
44​
45​ m_deviceManager->connection().connect(QStringLiteral("org.kde.KWin"),
46​ QStringLiteral("/org/kde/KWin/InputDevice"),
47​ QStringLiteral("org.kde.KWin.InputDeviceManager"),
sebasUnsubmitted
Done

indenting four more spaces aligns it with the above

Yes, OCD. :)

sebas: indenting four more spaces aligns it with the above Yes, OCD. :)
48​ QStringLiteral("deviceAdded"),
49​ this,
50​ SLOT(onDeviceAdded(QString)));
51​ m_deviceManager->connection().connect(QStringLiteral("org.kde.KWin"),
52​ QStringLiteral("/org/kde/KWin/InputDevice"),
53​ QStringLiteral("org.kde.KWin.InputDeviceManager"),
54​ QStringLiteral("deviceRemoved"),
55​ this,
56​ SLOT(onDeviceRemoved(QString)));
57​}
58​
59​KWinWaylandBackend::~KWinWaylandBackend()
60​{
61​ qDeleteAll(m_devices);
62​ delete m_deviceManager;
graesslinUnsubmitted
Done

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

graesslin: in general you can delete a nullptr, so the ifcheck is not needed
63​}
64​
graesslinUnsubmitted
Done

check qDeleteAll

graesslin: check qDeleteAll
65​void KWinWaylandBackend::findTouchpads()
66​{
67​ QStringList devicesSysNames;
68​ const QVariant reply = m_deviceManager->property("devicesSysNames");
69​ if (reply.isValid()) {
70​ qCDebug(KCM_TOUCHPAD) << "Devices list received successfully from KWin.";
71​ devicesSysNames = reply.toStringList();
72​ }
73​ else {
knambiarUnsubmitted
Not Done

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

knambiar: Just wondering whether these calls are synchronous and may cause delay in startup.
romanggAuthorUnsubmitted
Not Done

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.

romangg: > Just wondering whether these calls are synchronous and may cause delay in startup. I assume…
graesslinUnsubmitted
Done

const

graesslin: const
74​ qCCritical(KCM_TOUCHPAD) << "Error on receiving device list from KWin.";
75​ m_errorString = i18n("Querying input devices failed. Please reopen this settings module.");
76​ return;
77​ }
78​
79​ bool touchpadFound = false;
80​ for (QString sn : devicesSysNames) {
graesslinUnsubmitted
Done

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

graesslin: We normally don't use the name "KWin" in user facing messages. Also I don't expect users to…
81​ QDBusInterface deviceIface(QStringLiteral("org.kde.KWin"),
82​ QStringLiteral("/org/kde/KWin/InputDevice/") + sn,
83​ QStringLiteral("org.kde.KWin.InputDevice"),
84​ QDBusConnection::sessionBus(),
85​ this);
graesslinUnsubmitted
Done

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

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

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?

romangg: Are you sure? The foreach keyword is still listed in the official Qt docu. What's the best…
luebkingUnsubmitted
Done

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

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

luebking: "might deprecate it" - magic eightball wisdom ;-) The reason is that there's now http://en.
graesslinUnsubmitted
Done

https://www.kdab.com/goodbye-q_foreach/

graesslin: https://www.kdab.com/goodbye-q_foreach/
86​ const QVariant reply = deviceIface.property("touchpad");
87​ if (reply.isValid() && reply.toBool()) {
88​ KWinWaylandTouchpad* tp = new KWinWaylandTouchpad(sn);
89​ if (!tp->init()) {
90​ qCCritical(KCM_TOUCHPAD) << "Error on creating touchpad object" << sn;
sebasUnsubmitted
Done

remove whitespace between ! and tp

sebas: remove whitespace between ! and tp
91​ m_errorString = i18n("Critical error on reading fundamental device infos for touchpad %1.", sn);
graesslinUnsubmitted
Done

const

graesslin: const
92​ return;
93​ }
94​ m_devices.append(tp);
95​ touchpadFound = true;
96​ qCDebug(KCM_TOUCHPAD).nospace() << "Touchpad found: " << tp->name() << " (" << tp->sysName() << ")";
graesslinUnsubmitted
Done

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

graesslin: I doubt a restart would help here. That should run in an error again.
97​ }
98​ }
99​}
100​
101​bool KWinWaylandBackend::applyConfig()
102​{
103​ return std::all_of(m_devices.constBegin(), m_devices.constEnd(),
104​ [] (QObject *t) { return static_cast<KWinWaylandTouchpad*>(t)->applyConfig(); });
105​}
106​
107​bool KWinWaylandBackend::getConfig()
108​{
109​ return std::all_of(m_devices.constBegin(), m_devices.constEnd(),
110​ [] (QObject *t) { return static_cast<KWinWaylandTouchpad*>(t)->getConfig(); });
111​}
112​
113​bool KWinWaylandBackend::getDefaultConfig()
114​{
graesslinUnsubmitted
Done
return std::all_of(m_devices.constBegin(), m_devices.constEnd(),
    [] (QObject *t) { return static_cast<KWinWaylandTouchpad*>(t)->applyConfig(); });
graesslin: return std::all_of(m_devices.constBegin(), m_devices.constEnd(), [] (QObject *t) {…
115​ return std::all_of(m_devices.constBegin(), m_devices.constEnd(),
116​ [] (QObject *t) { return static_cast<KWinWaylandTouchpad*>(t)->getDefaultConfig(); });
117​}
118​
119​bool KWinWaylandBackend::isChangedConfig() const
120​{
121​ return std::any_of(m_devices.constBegin(), m_devices.constEnd(),
122​ [] (QObject *t) { return static_cast<KWinWaylandTouchpad*>(t)->isChangedConfig(); });
123​}
124​
125​void KWinWaylandBackend::onDeviceAdded(QString sysName)
graesslinUnsubmitted
Done

can also be simplified with algorithm

graesslin: can also be simplified with algorithm
126​{
127​ if (std::any_of(m_devices.constBegin(), m_devices.constEnd(),
128​ [sysName] (QObject *t) { return static_cast<KWinWaylandTouchpad*>(t)->sysName() == sysName; })) {
129​ return;
130​ }
131​
132​ QDBusInterface deviceIface(QStringLiteral("org.kde.KWin"),
133​ QStringLiteral("/org/kde/KWin/InputDevice/") + sysName,
134​ QStringLiteral("org.kde.KWin.InputDevice"),
135​ QDBusConnection::sessionBus(),
136​ this);
graesslinUnsubmitted
Done

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

graesslin: and here as well. Given that you have three times the same method with just one smallish…
137​ QVariant reply = deviceIface.property("touchpad");
138​ if (reply.isValid() && reply.toBool()) {
139​ KWinWaylandTouchpad* tp = new KWinWaylandTouchpad(sysName);
140​ if (!tp->init() || !tp->getConfig()) {
141​ emit touchpadAdded(false);
142​ return;
143​ }
144​ m_devices.append(tp);
145​ qCDebug(KCM_TOUCHPAD).nospace() << "Touchpad connected: " << tp->name() << " (" << tp->sysName() << ")";
graesslinUnsubmitted
Done

std::any_of

graesslin: std::any_of
146​ emit touchpadAdded(true);
147​ }
148​}
149​
150​void KWinWaylandBackend::onDeviceRemoved(QString sysName)
151​{
152​ QVector<QObject*>::const_iterator it = std::find_if(m_devices.constBegin(), m_devices.constEnd(),
153​ [sysName] (QObject *t) { return static_cast<KWinWaylandTouchpad*>(t)->sysName() == sysName; });
154​ if (it == m_devices.cend()) {
155​ return;
graesslinUnsubmitted
Done

std::find_if

graesslin: std::find_if
156​ }
157​
158​ KWinWaylandTouchpad *tp = static_cast<KWinWaylandTouchpad*>(*it);
159​ qCDebug(KCM_TOUCHPAD).nospace() << "Touchpad disconnected: " << tp->name() << " (" << tp->sysName() << ")";
160​
161​ int index = it - m_devices.cbegin();
162​ m_devices.removeAt(index);
163​ emit touchpadRemoved(index);
164​}
graesslinUnsubmitted
Done

std::find_if

graesslin: std::find_if