D14542: WIP: Basic KCM using new virtual desktops DBus interface

zzag added a comment.


  Some minor nitpicks.

INLINE COMMENTS

> desktopsmodel.cpp:37
> +
> +namespace KWin {
> +

namespace KWin
  {

> desktopsmodel.cpp:288-291
> +        s_serviceName,
> +        s_virtDesktopsPath,
> +        s_virtualDesktopsInterface,
> +        QStringLiteral("createDesktop"));

Please indent it.

> desktopsmodel.cpp:317
> +
> +        const QDBusPendingCallWatcher *watcher = new QDBusPendingCallWatcher(pending, this);
> +        QObject::connect(watcher, &QDBusPendingCallWatcher::finished, this, callFinished);

You could also use `auto` here. ;-)

> desktopsmodel.cpp:378
> +    const KWin::DBusDesktopDataVector &desktops = qdbus_cast<KWin::DBusDesktopDataVector>(
> +        data.value(QLatin1String("desktops")).value<QDBusArgument>()
> +    );

Minor nitpick: In order to construct QString, we'll copy the QLatin1String (source <https://code.woboq.org/qt5/qtbase/src/corelib/tools/qstring.cpp.html#_ZN7QString17fromLatin1_helperEPKci>). Maybe, use QStringLiteral instead? (Same with the QLatin1String down below)

> desktopsmodel.cpp:384
> +
> +    foreach(const KWin::DBusDesktopDataStruct &d, desktops) {
> +        m_serverSideDesktops.append(d.id);

Minor nitpick: because that's a new code, maybe use range based for loop instead?

> desktopsmodel.h:30
> +
> +namespace KWin {
> +

Coding style nitpick: namespaces have the opening brace on a new line.

> desktopsmodel.h:41
> +
> +    public:
> +        enum AdditionalRoles {

Coding style nitpick: the kdelibs coding style doesn't say anything about indenting access modifiers, but in KWin, we usually put access modifiers on the start of a line (i.e. they are not indented).

> desktopsmodel.h:54
> +        QVariant data(const QModelIndex &index, int role = Qt::DisplayRole) const override;
> +        int rowCount(const QModelIndex &parent = QModelIndex()) const override;
> +

Minor nitpick: you could also use uniform initialization, e.g.

  int rowCount(const QModelIndex &parent = {}) const override;

It saves typing extra characters, also looks neater, IMHO. I didn't test it, so take my words with a grain of salt.

> desktopsmodel.h:85
> +        void desktopRowsChanged(uint rows);
> +        void checkModifiedState(bool server = false);
> +        void handleCallError();

Feel free to ignore this one: as an alternative, we could use an enum instead of the boolean trap.

> virtualdesktops.cpp:29
> +
> +namespace KWin {
> +

namespace KWin
  {

> virtualdesktops.cpp:56
> +void VirtualDesktops::load()
> +{
> +}

It looks like it does nothing. If we need it to be empty, can you please add an explanatory comment why?

> virtualdesktops.h:23
> +
> +namespace KWin {
> +

namespace KWin
  {

> virtualdesktops.h:34
> +    public:
> +        explicit VirtualDesktops(QObject* parent = nullptr, const QVariantList &list = QVariantList());
> +        ~VirtualDesktops() override;

Coding style nitpick: `QObject *parent` -> `QObject *parent`.

> virtualdesktops.h:45
> +    private:
> +        KWin::DesktopsModel *m_desktopsModel;
> +};

Minor nitpick: `KWin::` is redundant.

REPOSITORY
  R108 KWin

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

To: hein, mart, davidedmundson, ltoscano
Cc: zzag, davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart