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

[sebas (Sebastian Kügler)]

sebas added inline comments.

INLINE COMMENTS

> kwin_wayland.cmake:1
> +#find_package(X11 REQUIRED)
> +

remove commented out bits?

> kwinwaylandbackend.cpp:46
> +    m_deviceManager->connection().connect(QStringLiteral("org.kde.KWin"),
> +                                      QStringLiteral("/org/kde/KWin/InputDevice"),
> +                                      QStringLiteral("org.kde.KWin.InputDeviceManager"),

indenting four more spaces aligns it with the above

Yes, OCD. :)

> kwinwaylandbackend.cpp:89
> +            KWinWaylandTouchpad* tp = new KWinWaylandTouchpad(sn);
> +            if (! tp->init()) {
> +                qCCritical(KCM_TOUCHPAD) << "Error on creating touchpad object" << sn;

remove whitespace between ! and tp

> kwinwaylandbackend.h:2
> +/*
> + * Copyright (C) 2016 Roman Gilg <subdiff at gmail.com>
> + *

Bit of a nitpick, but the (C) doesn't really add anything. I usually leave it out.

> kwinwaylandtouchpad.h:36
> +    Q_PROPERTY(QString name READ name CONSTANT)
> +    Q_PROPERTY(bool supportsDisableEvents READ supportsDisableEvents CONSTANT)
> +    Q_PROPERTY(bool enabled READ isEnabled WRITE setEnabled NOTIFY enabledChanged)

I wonder if it wouldn't be neater if the features would be an enum, and you'd have methods to check for supported and enabled features. Have you thought of this?

> kwinwaylandtouchpad.h:352
> +    struct Prop {
> +        explicit Prop(QByteArray dbusName)
> +            : dbus(dbusName)

const & for the argument?

> ToolTip.qml:24
> +MouseArea {
> +    anchors.fill: parent
> +

whitespace looks weird here ... perhaps group the properties, the new properties and the signal handlers in blocks?

> ToolTip.qml:45
> +
> +        function killTooltip() {
> +            stop()

Could also be hidden by using a onRunningChanged handler, that way the stop() method would 'just work'

> main.qml:274
> +                                                }
> +                                                value = 4.5 * touchpad.pointerAcceleration + 5.5
> +                                            }

magic values, a comment would be useful where the 4.5 and 5.5 come from

> main.qml:279
> +                                                // return on initializing, otherwise ugly type error message by qml
> +                                                if (!initialized) {
> +                                                    initialized = true

is "value changed twice" a useful condition to set initalized here?

> main.qml:285
> +                                                if (enabled && !root.loading) {
> +                                                    touchpad.pointerAcceleration = Math.round( (value - 5.5) / 4.5 * 100 ) / 100
> +                                                    root.changeSignal()

same here, comment please

> main.qml:294
> +                                                hoverEnabled: true
> +                                                onPressed: mouse.accepted = false
> +                                                onWheel: { /* ignore. TODO: send event to ScrollView */ }

not so sure about that, it would introduce inconsistent special behaviour just in this KCM, as a slider always should react to wheel events.

> main.qml:545
> +                                        ToolTip {
> +                                            text: i18n("Touchscreen like scrolling")
> +                                        }

full stop is missing at the end

> touchpadconfiglibinput.cpp:100
> +{
> +    return QSize(650,800);
> +}

This will fall over with high dpi displays...

> touchpadconfiglibinput.cpp:105
> +{
> +    return QSize(100,200);
> +}

problematic for high dpi

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: sebas, luebking, graesslin, knambiar, kwin, plasma-devel, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, eliasp
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20161227/eab42402/attachment-0001.html>