graesslin added inline comments.
INLINE COMMENTS
> kwinwaylandbackend.cpp:59-63
> + foreach (QObject *d, m_devices) {
> + if (d) {
> + delete d;
> + }
> + }
check qDeleteAll
> kwinwaylandbackend.cpp:60-61
> + foreach (QObject *d, m_devices) {
> + if (d) {
> + delete d;
> + }
in general you can delete a nullptr, so the ifcheck is not needed
> kwinwaylandbackend.cpp:72
> + QStringList devicesSysNames;
> + QVariant reply = m_deviceManager->property("devicesSysNames");
> + if (reply.isValid()) {
const
> kwinwaylandbackend.cpp:79
> + qCCritical(KCM_TOUCHPAD) << "Error on receiving device list from
> KWin.";
> + m_errorString = i18n("Error on receiving device list from KWin.
> Please restart Touchpad KCM.");
> + return;
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".
> kwinwaylandbackend.cpp:84
> + bool touchpadFound = false;
> + foreach (QString sn, devicesSysNames) {
> + QDBusInterface deviceIface(QStringLiteral("org.kde.KWin"),
please don't use the Q_FOREACH in new code as Qt might deprecate it.
> kwinwaylandbackend.cpp:90
> + this);
> + QVariant reply = deviceIface.property("touchpad");
> + if (reply.isValid() && reply.toBool()) {
const
> kwinwaylandbackend.cpp:95
> + qCCritical(KCM_TOUCHPAD) << "Error on creating touchpad
> object" << sn;
> + m_errorString = i18n("Error on creating touchpad object.
> Please restart KCM.");
> + return;
I doubt a restart would help here. That should run in an error again.
> kwinwaylandbackend.cpp:107-113
> + bool success = true;
> + foreach(QObject *t, m_devices) {
> + if (!static_cast<KWinWaylandTouchpad*>(t)->applyConfig()) {
> + success = false;
> + }
> + }
> + return success;
return std::all_of(m_devices.constBegin(), m_devices.constEnd(),
[] (QObject *t) { return
static_cast<KWinWaylandTouchpad*>(t)->applyConfig(); });
> kwinwaylandbackend.cpp:118-124
> + bool success = true;
> + foreach(QObject *t, m_devices) {
> + if (!static_cast<KWinWaylandTouchpad*>(t)->getConfig()) {
> + success = false;
> + }
> + }
> + return success;
can also be simplified with algorithm
> kwinwaylandbackend.cpp:129-135
> + bool success = true;
> + foreach(QObject *t, m_devices) {
> + if (!static_cast<KWinWaylandTouchpad*>(t)->getDefaultConfig()) {
> + success = false;
> + }
> + }
> + return success;
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
> kwinwaylandbackend.cpp:140-144
> + bool ret = false;
> + foreach(QObject *q, m_devices) {
> + ret |= static_cast<KWinWaylandTouchpad*>(q)->isChangedConfig();
> + }
> + return ret;
std::any_of
> kwinwaylandbackend.cpp:149-154
> + foreach (QObject *d, m_devices) {
> + KWinWaylandTouchpad *t = static_cast<KWinWaylandTouchpad*>(d);
> + if (t->sysName() == sysName) {
> + return;
> + }
> + }
std::find_if
> kwinwaylandbackend.cpp:187-197
> +QList<QObject*>::iterator KWinWaylandBackend::getDeviceIterator(QString
> sysName)
> +{
> + QList<QObject*>::iterator it;
> + for (it = m_devices.begin(); it != m_devices.end(); ++it) {
> + KWinWaylandTouchpad *t = static_cast<KWinWaylandTouchpad*>(*it);
> + if (t->sysName() == sysName) {
> + return it;
std::find_if
> kwinwaylandbackend.h:55
> + QDBusInterface* m_deviceManager;
> + QList<QObject*> m_devices;
> +
why is m_devices a list of QObjects and not a list of KWinWaylandTouchpad?
Also: please don't use QList any more in new code, but if possible QVector.
REPOSITORY
R119 Plasma Desktop
REVISION DETAIL
https://phabricator.kde.org/D3617
EMAIL PREFERENCES
https://phabricator.kde.org/settings/panel/emailpreferences/
To: subdiff, #kwin, #plasma_on_wayland, #plasma, #vdg
Cc: graesslin, knambiar, kwin, plasma-devel, lesliezhai, ali-mohamed,
hardening, jensreuterberg, abetts, eliasp, sebas
