davidedmundson requested changes to this revision.
davidedmundson added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> desktop.cpp:59
> +    const QVariantList &desktopIds = s_virtualDesktopInfo->desktopIds();
> +    const QStringList &desktopNames = s_virtualDesktopInfo->desktopNames();
> +    const QVariant &currentDesktop = s_virtualDesktopInfo->currentDesktop();

Use of the & is weird.

> desktop.cpp:108
> +
> +    const int nextDesktopIndex = (currentDesktopIndex % desktopIds.count() + 1);
> +

I /think/ this is wrong. Not 100% sure.

We used to want a number wrapped between 1 and numDesktops.

The old
 KWindowSystem::setCurrentDesktop(currentDesktop % numDesktops + 1);

is probably more understandable as
 KWindowSystem::setCurrentDesktop((currentDesktop +1 ) -1 % numDesktops + 1);

In the new code as we have the IDs in a lookup table so we want our final number to be between 0 and numDesktops-1

Therefore should this be
currentDesktopIndex +1 % desktopIds.count();

?

(same for perform previous)

> broulik wrote in desktop.cpp:46
> `!s_instanceCount`

In the other patch VirtualDesktopInfo has a shared d ptr across all instances.

There's no point making VirtualDesktopInfo shared when it internally does it itself anyway. You save practically nothing.

REPOSITORY
  R120 Plasma Workspace

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

To: hein, mart, davidedmundson
Cc: broulik, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart