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.
Details
Details
- Reviewers
mart davidedmundson - Maniphest Tasks
- T4457: [kwayland] Virtual Desktop protocol
- Commits
- R120:6e41b8d98e06: Port the "Switch Desktop" containment action to libtaskmanager
Diff Detail
Diff Detail
- Repository
- R120 Plasma Workspace
- Branch
- arcpatch-D13745
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 3005 Build 3023: arc lint + arc unit
| containmentactions/switchdesktop/desktop.cpp | ||
|---|---|---|
| 49 ↗ | (On Diff #41930) | !s_instanceCount |
| 68 ↗ | (On Diff #41930) | 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 |
| containmentactions/switchdesktop/desktop.cpp | ||
|---|---|---|
| 68 ↗ | (On Diff #41930) | 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). |
| containmentactions/switchdesktop/desktop.cpp | ||
|---|---|---|
| 49 ↗ | (On Diff #41930) | 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. |
| 62 ↗ | (On Diff #41930) | Use of the & is weird. |
| 111 ↗ | (On Diff #41930) | I /think/ this is wrong. Not 100% sure. We used to want a number wrapped between 1 and numDesktops. The old is probably more understandable as 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 ? (same for perform previous) |
| containmentactions/switchdesktop/desktop.cpp | ||
|---|---|---|
| 79 ↗ | (On Diff #41930) | Will this loop ever be executed? Also, why not delete m_actions.take(i - 1);? |
| containmentactions/switchdesktop/desktop.cpp | ||
|---|---|---|
| 49 ↗ | (On Diff #41930) | This comment wasn't addressed. It goes for the other containment actions patch too. |
| containmentactions/switchdesktop/desktop.cpp | ||
|---|---|---|
| 79 ↗ | (On Diff #41930) | Could someone please explain why this part is fine? |
| containmentactions/switchdesktop/desktop.cpp | ||
|---|---|---|
| 79 ↗ | (On Diff #41930) | 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); |
| containmentactions/switchdesktop/desktop.cpp | ||
|---|---|---|
| 79 ↗ | (On Diff #41930) | 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. |
Comment Actions
- 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()
Comment Actions
Remove two arg() fixes in the diff that should be in the other containment action diff
Comment Actions
Ping?
| containmentactions/switchdesktop/desktop.cpp | ||
|---|---|---|
| 62 ↗ | (On Diff #41930) | 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. |
| containmentactions/switchdesktop/desktop.cpp | ||
|---|---|---|
| 79 ↗ | (On Diff #41930) | It should probably be ">". If i == numDesktops, we delete numDesktops - 1, which we then access down below. |