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