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
