A rewrite of the Virtual Desktops KCM using the new DBus
API.
Depends on D13887.
Details
Details
- Reviewers
mart davidedmundson ltoscano zzag ngraham - Group Reviewers
Documentation - Maniphest Tasks
- T4457: [kwayland] Virtual Desktop protocol
- Commits
- R108:cee5ea7819b2: [kcmkwin/desktop] KCM using new virtual desktops DBus interface
Diff Detail
Diff Detail
- Repository
- R108 KWin
- Branch
- master
- Lint
Lint Skipped Excuse: Weird errors - Unit
No Unit Test Coverage - Build Status
Buildable 5537 Build 5555: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
Please wait for others.
When you're about to push, please:
- re-title this patch to "[kcmkwin/desktop] Use new ...";
- shift access modifiers and everything that goes below 4 spaces to the left (that's one of my inline comments).
Comment Actions
I understand what you're doing with the syncing now, it makes sense in principle.
Can you copy-paste what you typed to me into the code.
| kcmkwin/kwindesktop/desktopsmodel.cpp | ||
|---|---|---|
| 203 | this is setting it to the value it already is | |
| 252 | If I have 3 desktops with 3 IDs and I delete desktop2 Here I delete 3 and then sync the names, leaving me with: it looks fine within the confines of this KCM, but as soon as we rely on those IDs for external use (even just the fact that a user might have his windows on id3 and none on id2) it'll fall apart. When you make the change sending insert/remove instead of renaming we'll need to make sure we do removal before insertion. desktopCreated relies on the index of the newly created desktop to be in the same place as m_desktops has it. If we insert first, the position will be off as the server will still have the about-to-be-deleted entries | |
| kcmkwin/kwindesktop/desktopsmodel.cpp | ||
|---|---|---|
| 414 | Is this right? If I remove a desktop in the KCM(without applying settings) and create a new one using the d-bus interface, the KCM will crash. | |
| kcmkwin/kwindesktop/desktopsmodel.cpp | ||
|---|---|---|
| 254 | Is m_desktops.last() the right one? It can be already deleted. For example, If user has two virtual desktops:
and he or she wants to delete virtual desktop called "Pizza", then Desktop 1 will be removed instead. | |
Comment Actions
- Sync ids before syncing names.
- Remove the dummy id swap, it's no longer needed given the above.
- Add a class description comment that illuminates the sync/detach behavior.
- Reindent headers.
Comment Actions
(it will try to reach the maximum number of virtual desktops)
Other issues:
- if I remove a virtual desktop and apply settings, it's not possible anymore to add or remove virtual desktops;
- if I create a virtual desktop, then for some reason the KCM will try to create more than one:
(it will try to reach the maximum number of virtual desktops)
Comment Actions
Fix syncing to server.
Fixes take various shapes:
- Fix bug on our side.
- Handle weird KWin behavior, such as emiting desktopRowsChanged with an unchanged value.
- s/onAccepted/onEditingFinished on the rename TextField, otherwise focus loss keeps the changed text but doesn't light up Apply.
Comment Actions
Revamp KWin restart handling
The way a KWin restart is handled is now the same as the general
"stay in sync with server if the user didn't make changes, other-
wise stick to the user state and notify about the server-side
change" approach:
- User changes are now kept and not thrown away
- When KWin restarts the old and the new server state are compared, and if the user had made any changes, the model notifies about any difference
Comment Actions
Some issues that I saw while testing this patch:
- with 2 rows and 6 virtual desktop, I get the following desktop layout (is it a bug in KWin core?) (EDIT: nvm, without pager it's pretty hard to keep track of what current virtual desktop is)
+---+---+---+---+---+ | | | | | | +---+---+---+---+---+ | | +---+
- if any virtual desktop is removed, then System Settings window will be sent to the last virtual desktop.
| kcmkwin/kwindesktop/desktopsmodel.h | ||
|---|---|---|
| 52 ↗ | (On Diff #44673) | *KWin-side |
| 120–125 ↗ | (On Diff #44673) | For better readability, you could create a struct to represent the state on both sides, e.g.: struct State { QStringList desktops; QHash<QString, QString> names; int rows; }; State m_clientState; State m_serverState; |
Comment Actions
Further fixes to sync & co
- Adding a desktop could emit wrong model transactions (wrong container count was used to calculate append index).
- updateModifiedState (previously checkModifiedState) now handles cases where desktop counts and names remained the same despite the user triggering with remove/add actions. This can happen when removing a desktop retaining the default name and adding a desktop back, for example. In this case the method will replace dummy with server ids so that the data structures match again, then avoid doing a sync to the server and disable the Apply button.
- During sync, when syncing ids replace any dummy ids with server ids in the data structures. For cases similar to above - a desktop was replaced with an identically-named one, and is not handled by a remove/create -, otherwise the following block will emit setDesktopName D-Bus calls with invalid ids.
Comment Actions
I've just done a round of testing of the latest revision together my last kwin patch, including:
- adding and/or removinf desktops
- changing the number of rows
- renaming some desktops
- restarting kwin randomly after any of the above to see settings are kept, without restarting the kcm which keeps showing coherent data
so, definitely lgtm (adding ship it as my personal one, still needs David's)
Comment Actions
It turns out we all collectively forgot about the "Switching" tab in the original KCM.
Together with @ngraham we came up with a plan:
- Drop the Shortcuts settings for now. They're duplicated from the Shortcuts KCM.
- Drop the Animation setting for now. Its duplicated from the Desktop Effects KCM.
- Rearrange the layout of the new KCM a bit, and add the remaining settings to the footer.
Here's a screenshot:
@ngraham would prefer to have the Animation setting in there, and I might add it back later in a seperate review before we go into freeze. For now, however, I want this reviewed and merged along with the other pending virtual desktop patches, so it gets testing in master. Please review.
Comment Actions
- if any virtual desktop is removed, then System Settings window will be sent to the last virtual desktop.
This seems to be a bug in KWin core.
"Navigation wraps around" and the other check boxes initially don't represent the actual state, e.g. RollOverDesktops is set to false, but the corresponding checkbox is checked.
Comment Actions
I can confirm @zzag's bug.
Overall it works very well! UI-wise, I found myself confused by the unpredictability of the Add button. With multiple rows, it was not clear which row the new desktop would be added to. Perhaps instead we could give each row an inline Add button, kind of like this:
Second issue: when adding new rows, the existing virtual desktops are unpredictably re-assigned. I would expect new empty rows to be created, without re-assigning the rows of the existing desktops.
Comment Actions
I'm afraid that in order to do that, we have to change KWin core (and also some effects) first.
Comment Actions
@ngraham From a UX perspective your comments make perfect sense, but unfortunately KWin currently has a flat list of desktops divided by number of rows so I need to decline that for now. It's out of scope for 5.15 for sure.
Comment Actions
Hmm, I'm no longer able to save new settings, i.e. if any option has been changed, the Apply button is still not enabled.
Comment Actions
Darn. Everything else looks and feels good to me. It would be nice to get the animation setting back in here for 5.15 though.
| kcmkwin/kwindesktop/kcm_kwin_virtualdesktops.desktop | ||
|---|---|---|
| 6 ↗ | (On Diff #44673) | please make sure the docs people know about this |
| kcmkwin/kwindesktop/package/contents/ui/main.qml | ||
|---|---|---|
| 223 ↗ | (On Diff #44673) | Can this be just "ms"-symbol as in Wikipedia for milliseconds to avoid confusion with plural forms? Thanks. |
Comment Actions
- Add missing upper bound to the Rows spinbox (cf. 06a9a2a468df).
- Change 'msec' to 'ms'.