Port the "Switch Desktop" containment action to libtaskmanager
ClosedPublic

Authored by hein on Sep 19 2018, 9:53 AM.

Details

Summary

This:

  • Makes it work on Wayland. Depends on D13745.
  • Cleans up includes and linkage.
  • Fixes some pre-existing bugs like cached actions not updating to changed desktop names.

Diff Detail

Repository
R120 Plasma Workspace
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
hein created this revision.Sep 19 2018, 9:53 AM
Restricted Application added a project: Plasma. · View Herald TranscriptSep 19 2018, 9:53 AM
hein requested review of this revision.Sep 19 2018, 9:53 AM
hein updated this revision to Diff 41931.Sep 19 2018, 9:58 AM

Remove another unused include.

hein updated this revision to Diff 41932.Sep 19 2018, 10:02 AM

Kill more unused stuff.

broulik added inline comments.
containmentactions/switchdesktop/desktop.cpp
39

!s_instanceCount

50

This codes doesn't take into account when the virtual desktop names change etc. I think it should re-create the actions every time, and not try to be "smart" by only changing the ones that are "superfluous" or "missing" as the names and order inbetween could have changed

hein added inline comments.Sep 20 2018, 3:41 PM
containmentactions/switchdesktop/desktop.cpp
50

You misread the diff. This was indeed a bug the old code has, but the new code sets the text and data on all actions every time (below).

hein updated this revision to Diff 41997.Sep 20 2018, 3:45 PM

Fix wrong conditional.

davidedmundson requested changes to this revision.Sep 28 2018, 8:54 AM
davidedmundson added inline comments.
containmentactions/switchdesktop/desktop.cpp
39

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.

44

Use of the & is weird.

91

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)

This revision now requires changes to proceed.Sep 28 2018, 8:54 AM
hein updated this revision to Diff 42597.Sep 30 2018, 1:43 PM

Fix list access out of bounds

hein updated this revision to Diff 42600.Sep 30 2018, 2:01 PM

Fix calculating next/prev desktop index

zzag added a subscriber: zzag.Sep 30 2018, 2:22 PM
zzag added inline comments.
containmentactions/switchdesktop/desktop.cpp
58–59

Will this loop ever be executed?

Also, why not delete m_actions.take(i - 1);?

davidedmundson requested changes to this revision.Oct 16 2018, 1:36 PM
davidedmundson added inline comments.
containmentactions/switchdesktop/desktop.cpp
39

This comment wasn't addressed.

It goes for the other containment actions patch too.

This revision now requires changes to proceed.Oct 16 2018, 1:36 PM
hein updated this revision to Diff 43753.Oct 16 2018, 7:21 PM

Fix two more uses of arg()

hein updated this revision to Diff 43754.Oct 16 2018, 7:28 PM

Don't keep VirtualDesktopInfo as a static member to make d_ed happy

davidedmundson accepted this revision.Oct 16 2018, 9:47 PM

to make d_ed happy

It worked

This revision is now accepted and ready to land.Oct 16 2018, 9:47 PM
zzag added inline comments.Oct 16 2018, 10:15 PM
containmentactions/switchdesktop/desktop.cpp
58–59

Could someone please explain why this part is fine?

zzag added inline comments.Oct 16 2018, 10:30 PM
containmentactions/switchdesktop/desktop.cpp
58–59

I didn't test it, but I think this one would be better:

qDeleteAll(m_actions.begin() + numDesktops, m_actions.end());
m_actions.resize(numDesktops);
davidedmundson requested changes to this revision.Oct 16 2018, 11:06 PM
davidedmundson added inline comments.
containmentactions/switchdesktop/desktop.cpp
58–59

There's a bigger question.

What if you have 5 desktops, and you delete desktop 2?

We don't want the action for desktop 5 to be the one that's removed.

This either needs better change tracking or to just rebuild the list every time.

This revision now requires changes to proceed.Oct 16 2018, 11:06 PM
hein updated this revision to Diff 43810.Oct 17 2018, 6:11 PM
  • Fix another arg() call
  • Do delete+remove in one step with take()
  • Fix loop condition

Addressing other review comments:

  • More change tracking is not needed because of the later loop that indiscriminately calls setText/setData on all actions
  • The suggested qDeleteAll+resize method doesn't work because QHash has no resize()
hein updated this revision to Diff 43811.Oct 17 2018, 6:14 PM

Remove two arg() fixes in the diff that should be in the other containment action diff

hein added a comment.Oct 23 2018, 5:58 AM

Ping?

containmentactions/switchdesktop/desktop.cpp
44

I know the containers are implicitly shared, but I consider it better coding style to do the correct C++ thing out of habit rather than rely on that.

zzag added inline comments.Oct 23 2018, 8:33 AM
containmentactions/switchdesktop/desktop.cpp
58–59

It should probably be ">".

If i == numDesktops, we delete numDesktops - 1, which we then access down below.

hein updated this revision to Diff 44096.Oct 23 2018, 8:51 AM

Fix off-by-one.

davidedmundson accepted this revision.Nov 1 2018, 4:47 PM
This revision is now accepted and ready to land.Nov 1 2018, 4:47 PM
This revision was automatically updated to reflect the committed changes.