[Differential] [Changed Subscribers] D3617: [Touchpad KCM] New KWin Wayland version

[graesslin (Martin Gräßlin)]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20161209/573c9f26/attachment-0001.html>