D8460: Refactor kcm_input to enable having multiple backends.

[Roman Gilg]

subdiff added inline comments.

INLINE COMMENTS

> x11mousebackend.cpp:26
> +#include <evdev-properties.h>
> +#include <libinput-properties.h>
> +#include <KLocalizedString>

This include is not needed.

> x11mousebackend.cpp:42
> +static const char PROFILE_ADAPTIVE[] = I18N_NOOP("Adaptive");
> +static const char PROFILE_FLAT[] = I18N_NOOP("Flat");
> +

These 3 are never used.

> x11mousebackend.cpp:50
> +template<typename Callback>
> +static void XI2ForallPointerDevices(Display *dpy, const Callback &callback) {
> +    int ndevices_return;

Never used.

> x11mousebackend.cpp:139
> +    // get settings from X server
> +    unsigned char map[20];
> +    m_numButtons = XGetPointerMapping(m_dpy, map, 20);

Below you do it with 256 for reasons explained there.

> x11mousebackend.cpp:168
> +    unsigned char map[256];
> +    m_numButtons = XGetPointerMapping(m_dpy, map, 256);
> +

You set m_numButtons here and in load. This creates some ambiguity. Shouldn't be it enough to set it in the load() function?

> x11mousebackend.cpp:170
> +
> +    int remap=(m_numButtons>=1);
> +    if (settings.handedEnabled && (settings.m_handedNeedsApply || force)) {

nitpick: Spaces

> x11mousebackend.cpp:206
> +        int retval;
> +        if (remap) {
> +            while ((retval=XSetPointerMapping(m_dpy, map,

remap only used here -> Don't use local variable. Just check the condition here.

> x11mousebackend.cpp:207
> +        if (remap) {
> +            while ((retval=XSetPointerMapping(m_dpy, map,
> +                                              m_numButtons)) == MappingBusy)

Spaces

> x11mousebackend.cpp:215
> +        // are belong to kcm touchpad.
> +        XIForallPointerDevices(m_dpy, [&] (XDeviceInfo *info) {
> +            int deviceid = info->id;

Capture "this" pointer and settings reference directly.

> main.cpp:71
>                                         QDBusConnection::sessionBus());
>      if(!theme.isEmpty())
>          klauncher.setLaunchEnv(QStringLiteral("XCURSOR_THEME"), theme);

I know it's copy paste. But so we can directly clean it up: Coding style -> always use braces.

> mousebackend.h:26
> +
> +enum class MouseHanded {
> +    Right = 0,

Put this enum class in a separate file, such that you don't have to include the full mousebackend.h in other header files like mouse.h

> mousesettings.cpp:96
> +    kcminputGroup.writeEntry("Threshold",thresholdMove);
> +    if (handed == MouseHanded::Right)
> +        kcminputGroup.writeEntry("MouseButtonMapping",QString("RightHanded"));

coding style

> mousesettings.h:27
> +
> +class MouseSettings
> +{

class -> struct

And remove the public keywords.

> mousesettings.h:36
> +    bool handedEnabled;
> +    bool m_handedNeedsApply;
> +    MouseHanded handed;

-> handedNeedsApply

like the others.

> mousesettings.h:46
> +    bool reverseScrollPolarity;
> +    QString m_currentAccelProfile;
> +};

-> currentAccelProfile

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D8460

To: xuetianweng, subdiff, davidedmundson, ngraham
Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20171029/ef482710/attachment-0001.html>