[kcmkwin/desktop] KCM using new virtual desktops DBus interface
ClosedPublic

Authored by hein on Aug 1 2018, 6:58 PM.

Details

Summary

A rewrite of the Virtual Desktops KCM using the new DBus
API.

Depends on D13887.

Diff Detail

Repository
R108 KWin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes
hein updated this revision to Diff 43814.Oct 17 2018, 6:22 PM
hein edited the summary of this revision. (Show Details)

Drop the TODO note from the message.

zzag added inline comments.Oct 23 2018, 8:20 AM
kcmkwin/kwindesktop/kcm_kwin_virtualdesktops.desktop
2

Is it a typo?

kcmkwin/kwindesktop/package/contents/ui/main.qml
31

I suppose we don't need NG anymore. :-)

kcmkwin/kwindesktop/package/metadata.desktop
3

Same here, we probably don't need "NG" anymore.

hein updated this revision to Diff 44095.Oct 23 2018, 8:48 AM
  • Fix Exec
  • Clean up KCM titles
zzag added a comment.Oct 23 2018, 9:46 AM

It looks like this revision now also includes Marco's patch.

hein updated this revision to Diff 44498.Oct 30 2018, 3:54 PM

Try to remove Marco's revision

hein added a comment.Oct 30 2018, 3:55 PM

Should be OK now

zzag accepted this revision.Nov 1 2018, 4:37 PM

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).
hein added a comment.Nov 1 2018, 4:43 PM
In D14542#352260, @zzag wrote:

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).

Will do

davidedmundson requested changes to this revision.Nov 1 2018, 6:15 PM

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
id1 -> desktop1
id2 -> desktop2
id3 -> desktop3

and I delete desktop2

Here I delete 3 and then sync the names, leaving me with:
id1 -> desktop1
id2 -> desktop3

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

This revision now requires changes to proceed.Nov 1 2018, 6:15 PM
zzag requested changes to this revision.Nov 1 2018, 8:28 PM
zzag added inline comments.
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.

zzag added inline comments.Nov 1 2018, 8:42 PM
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:

  • Desktop 1
  • Pizza

and he or she wants to delete virtual desktop called "Pizza", then Desktop 1 will be removed instead.

hein updated this revision to Diff 44671.Nov 1 2018, 8:43 PM
  • 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.
hein updated this revision to Diff 44672.Nov 1 2018, 8:56 PM
hein retitled this revision from KCM using new virtual desktops DBus interface to [kcmkwin/desktop] KCM using new virtual desktops DBus interface.

Update the title.

hein updated this revision to Diff 44673.Nov 1 2018, 8:59 PM

Fix build error.

zzag added a comment.Nov 1 2018, 9:03 PM

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)

hein updated this revision to Diff 46545.Nov 30 2018, 11:24 AM

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.
hein updated this revision to Diff 46551.Nov 30 2018, 12:08 PM

Rebase on master for good measure

hein updated this revision to Diff 46554.Nov 30 2018, 1:19 PM

Handle KWin restarts

hein updated this revision to Diff 46895.Dec 5 2018, 12:00 PM

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
zzag added a comment.EditedDec 5 2018, 2:48 PM

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

*KWin-side

120–125

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;
hein updated this revision to Diff 46916.Dec 5 2018, 8:21 PM

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.
hein added a comment.Dec 5 2018, 8:22 PM

I'm no longer aware of bugs in this, please re-review it.

Please have the latest version of @mart's D17265 applied or you may encounter weirdness from KWin's side.

mart accepted this revision.Dec 6 2018, 3:24 PM

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)

hein updated this revision to Diff 47240.Dec 10 2018, 7:21 AM

Add back nav wrap and OSD settings.

hein added a subscriber: ngraham.Dec 10 2018, 7:24 AM

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.

zzag added a comment.Dec 10 2018, 9:28 AM
  • 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.

ngraham requested changes to this revision.Dec 11 2018, 11:45 PM

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.

This revision now requires changes to proceed.Dec 11 2018, 11:45 PM
zzag added a comment.Dec 12 2018, 7:32 AM

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:

I'm afraid that in order to do that, we have to change KWin core (and also some effects) first.

hein added a comment.EditedDec 14 2018, 5:44 AM

@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.

hein updated this revision to Diff 47546.Dec 14 2018, 5:57 AM

Fix initial state

zzag added a comment.Dec 14 2018, 11:01 AM

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.

zzag added a comment.Dec 14 2018, 12:12 PM
In D14542#371677, @zzag wrote:
  • if any virtual desktop is removed, then System Settings window will be sent to the last virtual desktop.

Should be addressed by D17576.

hein added a comment.Dec 15 2018, 6:39 AM
In D14542#376846, @zzag wrote:

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.

Can you give me more specific steps? Because I can't reproduce here ...

hein added a comment.Dec 15 2018, 6:41 AM

Apply button working for me: https://youtu.be/LvMhpCLxdWY

ngraham accepted this revision.Dec 16 2018, 12:01 AM
In D14542#376755, @hein wrote:

@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.

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.

zzag accepted this revision.Dec 17 2018, 3:03 PM

Please wait for David's review. :-)

davidedmundson accepted this revision.Dec 17 2018, 3:42 PM
davidedmundson added inline comments.
kcmkwin/kwindesktop/kcm_kwin_virtualdesktops.desktop
6

please make sure the docs people know about this

This revision is now accepted and ready to land.Dec 17 2018, 3:42 PM
yurchor added inline comments.
kcmkwin/kwindesktop/package/contents/ui/main.qml
223

Can this be just "ms"-symbol as in Wikipedia for milliseconds to avoid confusion with plural forms? Thanks.

hein added inline comments.Dec 18 2018, 7:23 AM
kcmkwin/kwindesktop/kcm_kwin_virtualdesktops.desktop
6

Told Luigi about it weeks ago, no worries :)

kcmkwin/kwindesktop/package/contents/ui/main.qml
223

Sure, I just copied "msec" from the old KCM. Will adjust before pushing.

hein updated this revision to Diff 47749.Dec 18 2018, 7:43 AM
  • Add missing upper bound to the Rows spinbox (cf. 06a9a2a468df).
  • Change 'msec' to 'ms'.
This revision was automatically updated to reflect the committed changes.