D14542: WIP: Basic KCM using new virtual desktops DBus interface

[Eike Hein]

hein created this revision.
hein added a reviewer: mart.
Restricted Application added a project: KWin.
hein requested review of this revision.

REVISION SUMMARY
This is a basic proof of concept KCM using Marco's DBus API from
D13887 <https://phabricator.kde.org/D13887>, as he requested as a form of review.

The KCM currently installs alongside the old one with no
file conflicts.

The UI is based on ScrollViewKCM. I'll attach a screenshot
seperately. It's OK-ish, I guess, although only the "Rows"
spinbox correctly manages the "Apply" button; adding, removing
and renaming desktops results in direct calls to the compositor.
Renaming is done with an inline text field on the row delegate.

More importantly, here's the problems I found:

- The `desktopCreated` signal doesn't have an index parameter, which means I had to resort to appending new desktops I'm told about through the protocol, even though `createDesktop` takes a `position` parameter. The struct has an x11DesktopNumber, but I don't want to use that.
- The `position` parameter in `createDesktop` seems to be handled incorrectly on the KWin side. I expected it to be zero-indexed, but I have to add one to append a new desktop.
- To do the custom type demarshalling for the structs and vector of structs I copied the type definitions and the functions for now, but it would be good to put this stuff into a shared header considering it's the same repo.
- The KCM for some reason catches Return presses and closes when anything in the Qt Quick view has focus. To do the inline editing using a TextField in the delegate, I had to implement `Keys.on(Return|Enter)Pressed` instead of using `onAccepted`.
- A small spacing conundrum with the RowLayout in the footer I was too lazy to work out for now since this is a WIP UI anyway.

REPOSITORY
R108 KWin

BRANCH
mart/plasmavirtualdesktop

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

AFFECTED FILES
kcmkwin/CMakeLists.txt
kcmkwin/kwindesktopng/CMakeLists.txt
kcmkwin/kwindesktopng/Messages.sh
kcmkwin/kwindesktopng/desktopsmodel.cpp
kcmkwin/kwindesktopng/desktopsmodel.h
kcmkwin/kwindesktopng/kcm_kwin_virtualdesktops.desktop
kcmkwin/kwindesktopng/org.kde.kwin.virtualdesktopmanager.xml
kcmkwin/kwindesktopng/package/contents/ui/main.qml
kcmkwin/kwindesktopng/package/metadata.desktop
kcmkwin/kwindesktopng/virtualdesktops.cpp
kcmkwin/kwindesktopng/virtualdesktops.h

To: hein, mart
Cc: plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Eike Hein]

hein added a comment.


Screenshot:

F6170928: Screenshot_20180802_035857.png <https://phabricator.kde.org/F6170928>

REPOSITORY
R108 KWin

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

To: hein, mart
Cc: plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Eike Hein]

hein added a comment.


I just noticed my logic for the `DesktopRow` role is pretty borked, but out of time for today.

REPOSITORY
R108 KWin

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

To: hein, mart
Cc: plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Kai Uwe Broulik]

broulik added a comment.


That DBus stuff looks like it was painful to write :/

It seems the KCM does auto-apply of everything (desktop names, adding, removing them) which is not what we usually do, and neither did the old KCM

INLINE COMMENTS

desktopsmodel.cpp:92
+ QStringLiteral("org.kde.KWin.VirtualDesktopManager"),
+ QStringLiteral("rowsChanged"),
+ this,

Architectural question: why don't you go the standard `org.freedesktop.DBus.Properties.PropertiesChanged` way?

desktopsmodel.cpp:105
+ QStringLiteral("/VirtualDesktopManager"),
+ QStringLiteral("org.freedesktop.DBus.Properties"),
+ QStringLiteral("Get"));

Can you put a couple of static `QString`s somewhere to avoid copy-pasting this all over?

desktopsmodel.cpp:165
+
+ const int perRow = std::ceil((qreal)m_desktops.count() / (qreal)m_rows);
+ return (index.row() / perRow) + 1;

`m_rows` can potentially be `0` here

main.qml:160
+
+ QtControls.Button {
+ Layout.alignment: Qt.AlignRight

Add `list-add` icon

virtualdesktops.cpp:40
+ i18n("Configure Virtual Desktops"),
+ QStringLiteral("2.0"), QString(), KAboutLicense::GPL);
+ setAboutData(about);

Copyright headers in the code say LGPL

REPOSITORY
R108 KWin

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

To: hein, mart
Cc: broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Marco Martin]

mart added inline comments.

INLINE COMMENTS

desktopsmodel.cpp:78
+ QStringLiteral("/VirtualDesktopManager"),
+ QStringLiteral("org.kde.KWin.VirtualDesktopManager"),
+ QStringLiteral("desktopDataChanged"),

all of this, static values

desktopsmodel.cpp:106
+ QStringLiteral("org.freedesktop.DBus.Properties"),
+ QStringLiteral("Get"));
+

one way that I would love this thing to work (and how i designed it to be used as)
is with GetAll, which minimizes absolutely the roundtrips (the amount of data, even if useless passing is not an issue at all, the number of calls is)

It's very type unsafe, which sn't great, tough you are sure you get the whole initial state in one go.

Look at plasma-workspace/dataengines/statusnotifieritem/statusnotifieritemsource.cpp performrefresh() and refreshcallback()

In this case the whole getall would be needed only at start (and eventually when the service goes down/gets back up) as all the signals carry all the needed data to keep it in sync.

REPOSITORY
R108 KWin

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

To: hein, mart
Cc: broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Eike Hein]

hein added inline comments.

INLINE COMMENTS

broulik wrote in desktopsmodel.cpp:92
Architectural question: why don't you go the standard `org.freedesktop.DBus.Properties.PropertiesChanged` way?

No deeper reason. Marco added an additional rowsChanged I guess.

REPOSITORY
R108 KWin

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

To: hein, mart
Cc: broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Eike Hein]

hein updated this revision to Diff 39008.
hein added a comment.


- Use GetAll to initialize m_desktops and m_rows at the same time.
- Reuse QStrings for DBus connection stuff.
- Fix the DesktopRow role.
- Add list-add icon to button.
- Fix license.

REPOSITORY
R108 KWin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D14542?vs=38914&id=39008

BRANCH
mart/plasmavirtualdesktop

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

AFFECTED FILES
kcmkwin/CMakeLists.txt
kcmkwin/kwindesktopng/CMakeLists.txt
kcmkwin/kwindesktopng/Messages.sh
kcmkwin/kwindesktopng/desktopsmodel.cpp
kcmkwin/kwindesktopng/desktopsmodel.h
kcmkwin/kwindesktopng/kcm_kwin_virtualdesktops.desktop
kcmkwin/kwindesktopng/org.kde.kwin.virtualdesktopmanager.xml
kcmkwin/kwindesktopng/package/contents/ui/main.qml
kcmkwin/kwindesktopng/package/metadata.desktop
kcmkwin/kwindesktopng/virtualdesktops.cpp
kcmkwin/kwindesktopng/virtualdesktops.h

To: hein, mart
Cc: broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Eike Hein]

hein updated this revision to Diff 39009.
hein added a comment.


Relicense to GPL.

So it fits the code temporarily copied from dbusinterface.(h|cpp).

REPOSITORY
R108 KWin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D14542?vs=39008&id=39009

BRANCH
mart/plasmavirtualdesktop

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

AFFECTED FILES
kcmkwin/CMakeLists.txt
kcmkwin/kwindesktopng/CMakeLists.txt
kcmkwin/kwindesktopng/Messages.sh
kcmkwin/kwindesktopng/desktopsmodel.cpp
kcmkwin/kwindesktopng/desktopsmodel.h
kcmkwin/kwindesktopng/kcm_kwin_virtualdesktops.desktop
kcmkwin/kwindesktopng/org.kde.kwin.virtualdesktopmanager.xml
kcmkwin/kwindesktopng/package/contents/ui/main.qml
kcmkwin/kwindesktopng/package/metadata.desktop
kcmkwin/kwindesktopng/virtualdesktops.cpp
kcmkwin/kwindesktopng/virtualdesktops.h

To: hein, mart
Cc: broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[David Edmundson]

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

INLINE COMMENTS

desktopsmodel.cpp:53
+
+ bool connected = QDBusConnection::sessionBus().connect(
+ s_serviceName,

We can just generate the interface from the XML and use the generated class instead of using low level classes.

desktopsmodel.cpp:361
+
+const QDBusArgument &operator>>(const QDBusArgument &argument, KWin::DBusDesktopDataVector &deskVector)
+{

All this is already in the kwin repo for Marco's export. We should share code here even if it means including a C++ file from outside the kcmkwin dir.

REPOSITORY
R108 KWin

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

To: hein, mart, davidedmundson
Cc: davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[David Edmundson]

davidedmundson added a comment.


Also, I know this is a WIP, but so it's noted it's not going to get merged without some unit test of the new API and checking all current stuff passes.

REPOSITORY
R108 KWin

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

To: hein, mart, davidedmundson
Cc: davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[David Edmundson]

davidedmundson added a comment.


Edit. That unit test comment was intended for Marcos patch not this.

REPOSITORY
R108 KWin

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

To: hein, mart, davidedmundson
Cc: davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Eike Hein]

hein added inline comments.

INLINE COMMENTS

davidedmundson wrote in desktopsmodel.cpp:53
We can just generate the interface from the XML and use the generated class instead of using low level classes.

I have no preference here. Marco's patch currently doesn't provide an XML file, though.

davidedmundson wrote in desktopsmodel.cpp:361
All this is already in the kwin repo for Marco's export. We should share code here even if it means including a C++ file from outside the kcmkwin dir.

This was mentioned in the commit message, too. I want Marco to do this refactoring.

REPOSITORY
R108 KWin

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

To: hein, mart, davidedmundson
Cc: davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Eike Hein]

hein updated this revision to Diff 42592.
hein edited the summary of this revision.
hein added a comment.


Fully implement delayed-apply.

Conflicts between user edits and server-side changes are resolved
as follows:

- If the user hasn't made any edits vs. the server state, the KCM keeps in sync transparently.
- If the user has made edits and the server signals a change, an InlineMessage is used to inform the user that saving will over- write the externally-made changes.
- Saving user edits is done via a synchronization loop until the states are in sync again.

Call errors while saving user edits are now handled. If a call
error occurs during synchronization, it's aborted in a way that
allows the user to try again with another click on Apply/Ok.

I changed the dummy intro string above the list into something
more appropriate.

I still need Marco to look at the two FIXMEs in the code that
point out KWin quirks (also mentioned in the description).

REPOSITORY
R108 KWin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D14542?vs=39009&id=42592

BRANCH
arcpatch-D14542

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

AFFECTED FILES
kcmkwin/CMakeLists.txt
kcmkwin/kwindesktopng/CMakeLists.txt
kcmkwin/kwindesktopng/Messages.sh
kcmkwin/kwindesktopng/desktopsmodel.cpp
kcmkwin/kwindesktopng/desktopsmodel.h
kcmkwin/kwindesktopng/kcm_kwin_virtualdesktops.desktop
kcmkwin/kwindesktopng/org.kde.kwin.virtualdesktopmanager.xml
kcmkwin/kwindesktopng/package/contents/ui/main.qml
kcmkwin/kwindesktopng/package/metadata.desktop
kcmkwin/kwindesktopng/virtualdesktops.cpp
kcmkwin/kwindesktopng/virtualdesktops.h

To: hein, mart, davidedmundson
Cc: davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Andres Betts]

abetts added a comment.


Suggestions from the VDG Channel

- Make header "Row 1, Row 2, etc" grey
- Add icons (up for review) to the right of each desktop label

+1

REPOSITORY
R108 KWin

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

To: hein, mart, davidedmundson
Cc: abetts, davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, sebas, apol, mart

[Marco Martin]

mart added a comment.


wrt position of desktopAdded and eventual reorders..
in the desktop data, x11DesktopNubmer is always guaranteed to be the correct position (modulo the annoying off by one due to x11 desktops)

INLINE COMMENTS

desktopsmodel.cpp:295
+
+ // FIXME TODO: Positions are currently not zero- but one-indexed in KWin it seems.
+ call.setArguments({(uint)newIndex, m_names.value(m_desktops.at(newIndex - 1))});

this is for retrocompatibility with x11 desktops, where they are 1 indexed for some obscure reason.
1 has been kept elsewhere to not make kwin code even more confusing and error prone

REPOSITORY
R108 KWin

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

To: hein, mart, davidedmundson
Cc: abetts, davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, sebas, apol, mart

[Eike Hein]

hein updated this revision to Diff 42843.
hein added a comment.


Change background color of selection delegates to grey

REPOSITORY
R108 KWin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D14542?vs=42592&id=42843

BRANCH
arcpatch-D14542

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

AFFECTED FILES
kcmkwin/CMakeLists.txt
kcmkwin/kwindesktopng/CMakeLists.txt
kcmkwin/kwindesktopng/Messages.sh
kcmkwin/kwindesktopng/desktopsmodel.cpp
kcmkwin/kwindesktopng/desktopsmodel.h
kcmkwin/kwindesktopng/kcm_kwin_virtualdesktops.desktop
kcmkwin/kwindesktopng/org.kde.kwin.virtualdesktopmanager.xml
kcmkwin/kwindesktopng/package/contents/ui/main.qml
kcmkwin/kwindesktopng/package/metadata.desktop
kcmkwin/kwindesktopng/virtualdesktops.cpp
kcmkwin/kwindesktopng/virtualdesktops.h

To: hein, mart, davidedmundson
Cc: abetts, davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, sebas, apol, mart

[Eike Hein]

hein updated this revision to Diff 42897.
hein added a comment.


Adjust to Marco's DBus API changes, removes the FIXMEs from the code

REPOSITORY
R108 KWin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D14542?vs=42843&id=42897

BRANCH
arcpatch-D13887_1

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

AFFECTED FILES
kcmkwin/CMakeLists.txt
kcmkwin/kwindesktopng/CMakeLists.txt
kcmkwin/kwindesktopng/Messages.sh
kcmkwin/kwindesktopng/desktopsmodel.cpp
kcmkwin/kwindesktopng/desktopsmodel.h
kcmkwin/kwindesktopng/kcm_kwin_virtualdesktops.desktop
kcmkwin/kwindesktopng/org.kde.kwin.virtualdesktopmanager.xml
kcmkwin/kwindesktopng/package/contents/ui/main.qml
kcmkwin/kwindesktopng/package/metadata.desktop
kcmkwin/kwindesktopng/virtualdesktops.cpp
kcmkwin/kwindesktopng/virtualdesktops.h

To: hein, mart, davidedmundson
Cc: abetts, davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, sebas, apol, mart

[Eike Hein]

hein updated this revision to Diff 42898.
hein edited the summary of this revision.
hein removed a subscriber: abetts.
hein added a comment.


Update description to scratch off the done todos

REPOSITORY
R108 KWin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D14542?vs=42897&id=42898

BRANCH
arcpatch-D13887_1

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

AFFECTED FILES
kcmkwin/CMakeLists.txt
kcmkwin/kwindesktopng/CMakeLists.txt
kcmkwin/kwindesktopng/Messages.sh
kcmkwin/kwindesktopng/desktopsmodel.cpp
kcmkwin/kwindesktopng/desktopsmodel.h
kcmkwin/kwindesktopng/kcm_kwin_virtualdesktops.desktop
kcmkwin/kwindesktopng/org.kde.kwin.virtualdesktopmanager.xml
kcmkwin/kwindesktopng/package/contents/ui/main.qml
kcmkwin/kwindesktopng/package/metadata.desktop
kcmkwin/kwindesktopng/virtualdesktops.cpp
kcmkwin/kwindesktopng/virtualdesktops.h

To: hein, mart, davidedmundson
Cc: davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Marco Martin]

mart added a comment.


I think the last fixme is actually not a fixme.
dbus structs moved in vitualdesktopsdbustypes.h

INLINE COMMENTS

main.qml:176
+
+ Item { // FIXME TODO: Quick gross spacing hack.
+ Layout.fillWidth: true

Actually, that's the way rowlayouts are supposed to work, having empty items as spacers.
It mirrors the way QLayouts work, where the spacer is a dedicated item/class

REPOSITORY
R108 KWin

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

To: hein, mart, davidedmundson
Cc: davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Marco Martin]

mart added a comment.


once it uses the common dbus type, good to go for me, just rename it, eliminating the old kcm

REPOSITORY
R108 KWin

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

To: hein, mart, davidedmundson
Cc: davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Eike Hein]

hein updated this revision to Diff 42958.
hein edited the summary of this revision.
hein added a comment.


- Remove copied code.

REPOSITORY
R108 KWin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D14542?vs=42898&id=42958

BRANCH
arcpatch-D13887_2

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

AFFECTED FILES
kcmkwin/kwindesktop/CMakeLists.txt
kcmkwin/kwindesktop/Messages.sh
kcmkwin/kwindesktop/desktop.desktop
kcmkwin/kwindesktop/desktopnameswidget.cpp
kcmkwin/kwindesktop/desktopnameswidget.h
kcmkwin/kwindesktop/desktopsmodel.cpp
kcmkwin/kwindesktop/desktopsmodel.h
kcmkwin/kwindesktop/kcm_kwin_virtualdesktops.desktop
kcmkwin/kwindesktop/main.cpp
kcmkwin/kwindesktop/main.h
kcmkwin/kwindesktop/main.ui
kcmkwin/kwindesktop/org.kde.kwin.virtualdesktopmanager.xml
kcmkwin/kwindesktop/package/contents/ui/main.qml
kcmkwin/kwindesktop/package/metadata.desktop
kcmkwin/kwindesktop/virtualdesktops.cpp
kcmkwin/kwindesktop/virtualdesktops.h

To: hein, mart, davidedmundson
Cc: davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Eike Hein]

hein added a comment.


Forgot to mention it, but I also did the folder move/rename so this KCM now replaces the old one.

REPOSITORY
R108 KWin

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

To: hein, mart, davidedmundson
Cc: davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Eike Hein]

hein added a reviewer: ltoscano.
hein added a comment.


Adding Luigi due to the .pot rename caused by this.

REPOSITORY
R108 KWin

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

To: hein, mart, davidedmundson, ltoscano
Cc: davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Vlad Zagorodniy]

zzag added a comment.


Some minor nitpicks.

INLINE COMMENTS

desktopsmodel.cpp:37
+
+namespace KWin {
+

namespace KWin
{

desktopsmodel.cpp:288-291
+ s_serviceName,
+ s_virtDesktopsPath,
+ s_virtualDesktopsInterface,
+ QStringLiteral("createDesktop"));

Please indent it.

desktopsmodel.cpp:317
+
+ const QDBusPendingCallWatcher *watcher = new QDBusPendingCallWatcher(pending, this);
+ QObject::connect(watcher, &QDBusPendingCallWatcher::finished, this, callFinished);

You could also use `auto` here. ;-)

desktopsmodel.cpp:378
+ const KWin::DBusDesktopDataVector &desktops = qdbus_cast<KWin::DBusDesktopDataVector>(
+ data.value(QLatin1String("desktops")).value<QDBusArgument>()
+ );

Minor nitpick: In order to construct QString, we'll copy the QLatin1String (source <https://code.woboq.org/qt5/qtbase/src/corelib/tools/qstring.cpp.html#_ZN7QString17fromLatin1_helperEPKci>). Maybe, use QStringLiteral instead? (Same with the QLatin1String down below)

desktopsmodel.cpp:384
+
+ foreach(const KWin::DBusDesktopDataStruct &d, desktops) {
+ m_serverSideDesktops.append(d.id);

Minor nitpick: because that's a new code, maybe use range based for loop instead?

desktopsmodel.h:30
+
+namespace KWin {
+

Coding style nitpick: namespaces have the opening brace on a new line.

desktopsmodel.h:41
+
+ enum AdditionalRoles {

Coding style nitpick: the kdelibs coding style doesn't say anything about indenting access modifiers, but in KWin, we usually put access modifiers on the start of a line (i.e. they are not indented).

desktopsmodel.h:54
+ QVariant data(const QModelIndex &index, int role = Qt::DisplayRole) const override;
+ int rowCount(const QModelIndex &parent = QModelIndex()) const override;
+

Minor nitpick: you could also use uniform initialization, e.g.

int rowCount(const QModelIndex &parent = {}) const override;

It saves typing extra characters, also looks neater, IMHO. I didn't test it, so take my words with a grain of salt.

desktopsmodel.h:85
+ void desktopRowsChanged(uint rows);
+ void checkModifiedState(bool server = false);
+ void handleCallError();

Feel free to ignore this one: as an alternative, we could use an enum instead of the boolean trap.

virtualdesktops.cpp:29
+
+namespace KWin {
+

namespace KWin
{

virtualdesktops.cpp:56
+void VirtualDesktops::load()
+{
+}

It looks like it does nothing. If we need it to be empty, can you please add an explanatory comment why?

virtualdesktops.h:23
+
+namespace KWin {
+

namespace KWin
{

virtualdesktops.h:34
+ explicit VirtualDesktops(QObject* parent = nullptr, const QVariantList &list = QVariantList());
+ ~VirtualDesktops() override;

Coding style nitpick: `QObject *parent` -> `QObject *parent`.

virtualdesktops.h:45
+ KWin::DesktopsModel *m_desktopsModel;
+};

Minor nitpick: `KWin::` is redundant.

REPOSITORY
R108 KWin

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

To: hein, mart, davidedmundson, ltoscano
Cc: zzag, davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[David Edmundson]

davidedmundson added a comment.


QML and the rest is all fine.

I don't understand why desktopmodel is the way it is.

There are 2 DBus patterns one could do here.

- we buffer all changes in a model locally, when the user clicks save we apply them on the server and recall initialise.
- we do things async-realtime. The model is always in sync with remote changes. When the user clicks create/remove we send a request to the server; the model only updates when it gets the callback.

This seems to be doing both patterns at once.

INLINE COMMENTS

desktopsmodel.cpp:402
+ // If the user didn't make any changes, we can just stay in sync.
+ if (!m_userModified) {
+ beginInsertRows(QModelIndex(), data.position, data.position);

but if it is userModified don't we need to update the ID to be the non-dummy value in case that entry is then later removed?

desktopsmodel.cpp:416
+{
+ const int desktopIndex = m_serverSideDesktops.indexOf(id);
+

needs a guard.

could be emitted before the first load finishes

org.kde.kwin.virtualdesktopmanager.xml:1
+<interface name="org.kde.KWin.VirtualDesktopManager">
+ <signal name="desktopCreated">

You're not using this file?

I would strongly suggest generating the iface, it makes the C++ a lot nicer and you get compile errors if the iface ever changes.

REPOSITORY
R108 KWin

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

To: hein, mart, davidedmundson, ltoscano
Cc: zzag, davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Eike Hein]

hein added a comment.

Post by David Edmundson
QML and the rest is all fine.
I don't understand why desktopmodel is the way it is.
There are 2 DBus patterns one could do here.
- we buffer all changes in a model locally, when the user clicks save we apply them on the server and recall initialise.
- we do things async-realtime. The model is always in sync with remote changes. When the user clicks create/remove we send a request to the server; the model only updates when it gets the callback.
This seems to be doing both patterns at once.

I don't understand this review comment, either. But I can try to explain what DesktopsModel does again in addition to the earlier comments in the hopes it clears things up:

- It initially gets the state from KWin and populates.
- As long as the user makes no changes, KWin-side changes are directly exposed in the model.
- If the user makes changes, it stops exposing KWin-side changes live, but it keeps track of the KWin-side change, so it can figure out and apply the delta on Apply.
- When KWin-side changes happen while the model is user-modified, the user is informed that this has happened and that Apply will overwrite them.
- After Apply, it syncs live again, until the user makes further changes, etc.

From your comment, your second bullet was what the model used to do in the initial revision, when it was instant-apply. It's now delayed-apply. Your first bullet would work, but is clumsy and what this model does is better. It's not necessary to re-initialize and reset the model, because it's smart enough to figure out and apply the delta to the server, so at the end of that sync it ... well, knows it's in sync.

REPOSITORY
R108 KWin

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

To: hein, mart, davidedmundson, ltoscano
Cc: zzag, davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Eike Hein]

hein updated this revision to Diff 43180.
hein added a comment.


- Swap dummy ids out for real ids, otherwise sync can't finish (review by d_ed).
- Move hooking up state-altering signals to after initial state is in, so we don't need to guard against concurrency (review by d_ed).
- Removed unused file (review by d_ed).
- Various style cleanups pointed out by Vlad.

REPOSITORY
R108 KWin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D14542?vs=42958&id=43180

BRANCH
arcpatch-D13887_2

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

AFFECTED FILES
kcmkwin/kwindesktop/CMakeLists.txt
kcmkwin/kwindesktop/Messages.sh
kcmkwin/kwindesktop/desktop.desktop
kcmkwin/kwindesktop/desktopnameswidget.cpp
kcmkwin/kwindesktop/desktopnameswidget.h
kcmkwin/kwindesktop/desktopsmodel.cpp
kcmkwin/kwindesktop/desktopsmodel.h
kcmkwin/kwindesktop/kcm_kwin_virtualdesktops.desktop
kcmkwin/kwindesktop/main.cpp
kcmkwin/kwindesktop/main.h
kcmkwin/kwindesktop/main.ui
kcmkwin/kwindesktop/package/contents/ui/main.qml
kcmkwin/kwindesktop/package/metadata.desktop
kcmkwin/kwindesktop/virtualdesktops.cpp
kcmkwin/kwindesktop/virtualdesktops.h

To: hein, mart, davidedmundson, ltoscano
Cc: zzag, davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Eike Hein]

hein added inline comments.

INLINE COMMENTS

davidedmundson wrote in desktopsmodel.cpp:416
needs a guard.
could be emitted before the first load finishes

I'll move the other connnects to the initialize slot.

REPOSITORY
R108 KWin

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

To: hein, mart, davidedmundson, ltoscano
Cc: zzag, davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Eike Hein]

hein added inline comments.

INLINE COMMENTS

hein wrote in desktopsmodel.cpp:416
I'll move the other connnects to the initialize slot.

Sorry for the above, I wrote it before updating the review but forgot to submit.

REPOSITORY
R108 KWin

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

To: hein, mart, davidedmundson, ltoscano
Cc: zzag, davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Eike Hein]

hein updated this revision to Diff 43181.
hein retitled this revision from "WIP: Basic KCM using new virtual desktops DBus interface" to "KCM using new virtual desktops DBus interface".
hein added a reviewer: zzag.
hein removed a subscriber: zzag.
hein added a comment.


- Remove hack around Enter issue and move to onAccepted now that Marco has fixed it somewhere else.
- Update description.

REPOSITORY
R108 KWin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D14542?vs=43180&id=43181

BRANCH
arcpatch-D13887_2

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

AFFECTED FILES
kcmkwin/kwindesktop/CMakeLists.txt
kcmkwin/kwindesktop/Messages.sh
kcmkwin/kwindesktop/desktop.desktop
kcmkwin/kwindesktop/desktopnameswidget.cpp
kcmkwin/kwindesktop/desktopnameswidget.h
kcmkwin/kwindesktop/desktopsmodel.cpp
kcmkwin/kwindesktop/desktopsmodel.h
kcmkwin/kwindesktop/kcm_kwin_virtualdesktops.desktop
kcmkwin/kwindesktop/main.cpp
kcmkwin/kwindesktop/main.h
kcmkwin/kwindesktop/main.ui
kcmkwin/kwindesktop/package/contents/ui/main.qml
kcmkwin/kwindesktop/package/metadata.desktop
kcmkwin/kwindesktop/virtualdesktops.cpp
kcmkwin/kwindesktop/virtualdesktops.h

To: hein, mart, davidedmundson, ltoscano, zzag
Cc: davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Eike Hein]

hein added a comment.


@mart There's a last FIXME in the QML code for a spacing hack. Could you give me a hand with that one?

REPOSITORY
R108 KWin

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

To: hein, mart, davidedmundson, ltoscano, zzag
Cc: davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Marco Martin]

mart added a comment.

@mart There's a last FIXME in the QML code for a spacing hack (mentioned as a todo in the description). Could you give me a hand with that one?

to me is not an hack, it's how layouts pretty much always worked.. even at qlayouts times, there was a dedicated item as "spacer"

REPOSITORY
R108 KWin

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

To: hein, mart, davidedmundson, ltoscano, zzag
Cc: davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Eike Hein]

hein updated this revision to Diff 43315.
hein added a comment.


Drop FIXME from spacer on Marco's advice.

REPOSITORY
R108 KWin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D14542?vs=43181&id=43315

BRANCH
arcpatch-D13887_2

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

AFFECTED FILES
kcmkwin/kwindesktop/CMakeLists.txt
kcmkwin/kwindesktop/Messages.sh
kcmkwin/kwindesktop/desktop.desktop
kcmkwin/kwindesktop/desktopnameswidget.cpp
kcmkwin/kwindesktop/desktopnameswidget.h
kcmkwin/kwindesktop/desktopsmodel.cpp
kcmkwin/kwindesktop/desktopsmodel.h
kcmkwin/kwindesktop/kcm_kwin_virtualdesktops.desktop
kcmkwin/kwindesktop/main.cpp
kcmkwin/kwindesktop/main.h
kcmkwin/kwindesktop/main.ui
kcmkwin/kwindesktop/package/contents/ui/main.qml
kcmkwin/kwindesktop/package/metadata.desktop
kcmkwin/kwindesktop/virtualdesktops.cpp
kcmkwin/kwindesktop/virtualdesktops.h

To: hein, mart, davidedmundson, ltoscano, zzag
Cc: davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Eike Hein]

hein updated this revision to Diff 43814.
hein edited the summary of this revision.
hein added a comment.


Drop the TODO note from the message.

REPOSITORY
R108 KWin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D14542?vs=43315&id=43814

BRANCH
arcpatch-D14542

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

AFFECTED FILES
kcmkwin/kwindesktop/CMakeLists.txt
kcmkwin/kwindesktop/Messages.sh
kcmkwin/kwindesktop/desktop.desktop
kcmkwin/kwindesktop/desktopnameswidget.cpp
kcmkwin/kwindesktop/desktopnameswidget.h
kcmkwin/kwindesktop/desktopsmodel.cpp
kcmkwin/kwindesktop/desktopsmodel.h
kcmkwin/kwindesktop/kcm_kwin_virtualdesktops.desktop
kcmkwin/kwindesktop/main.cpp
kcmkwin/kwindesktop/main.h
kcmkwin/kwindesktop/main.ui
kcmkwin/kwindesktop/package/contents/ui/main.qml
kcmkwin/kwindesktop/package/metadata.desktop
kcmkwin/kwindesktop/virtualdesktops.cpp
kcmkwin/kwindesktop/virtualdesktops.h

To: hein, mart, davidedmundson, ltoscano, zzag
Cc: davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Eike Hein]

hein added a comment.


Ping?

REPOSITORY
R108 KWin

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

To: hein, mart, davidedmundson, ltoscano, zzag
Cc: davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Vlad Zagorodniy]

zzag added inline comments.

INLINE COMMENTS

kcm_kwin_virtualdesktops.desktop:2
[Desktop Entry]
+Exec=kcmshell5 kwn_virtualdesktops
+Icon=preferences-desktop

Is it a typo?

main.qml:30
+
+ ConfigModule.quickHelp: i18n("Virtual Desktops NG")
+

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

metadata.desktop:2
+[Desktop Entry]
+Name=Virtual Desktops NG
+Name[ar]=أسطح المكتؚ الافتراضية

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

REPOSITORY
R108 KWin

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

To: hein, mart, davidedmundson, ltoscano, zzag
Cc: davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Eike Hein]

hein updated this revision to Diff 44095.
hein added a comment.


- Fix Exec
- Clean up KCM titles

REPOSITORY
R108 KWin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D14542?vs=43814&id=44095

BRANCH
arcpatch-D14542_1

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

AFFECTED FILES
CMakeLists.txt
abstract_client.cpp
abstract_client.h
autotests/CMakeLists.txt
autotests/integration/virtual_desktop_test.cpp
autotests/test_window_paint_data.cpp
dbusinterface.cpp
dbusinterface.h
deleted.cpp
deleted.h
effects.cpp
effects/desktopgrid/desktopgrid.cpp
effects/desktopgrid/desktopgrid.h
kcmkwin/kwindesktop/CMakeLists.txt
kcmkwin/kwindesktop/Messages.sh
kcmkwin/kwindesktop/desktop.desktop
kcmkwin/kwindesktop/desktopnameswidget.cpp
kcmkwin/kwindesktop/desktopnameswidget.h
kcmkwin/kwindesktop/desktopsmodel.cpp
kcmkwin/kwindesktop/desktopsmodel.h
kcmkwin/kwindesktop/kcm_kwin_virtualdesktops.desktop
kcmkwin/kwindesktop/main.cpp
kcmkwin/kwindesktop/main.h
kcmkwin/kwindesktop/main.ui
kcmkwin/kwindesktop/package/contents/ui/main.qml
kcmkwin/kwindesktop/package/metadata.desktop
kcmkwin/kwindesktop/virtualdesktops.cpp
kcmkwin/kwindesktop/virtualdesktops.h
libkwineffects/kwineffects.cpp
libkwineffects/kwineffects.h
org.kde.KWin.VirtualDesktopManager.xml
toplevel.h
unmanaged.cpp
unmanaged.h
useractions.cpp
useractions.h
virtualdesktops.cpp
virtualdesktops.h
virtualdesktopsdbustypes.cpp
virtualdesktopsdbustypes.h
wayland_server.cpp
wayland_server.h
workspace.cpp
workspace.h

To: hein, mart, davidedmundson, ltoscano, zzag
Cc: davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Vlad Zagorodniy]

zzag added a comment.


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

REPOSITORY
R108 KWin

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

To: hein, mart, davidedmundson, ltoscano, zzag
Cc: davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Eike Hein]

hein updated this revision to Diff 44498.
hein added a comment.


Try to remove Marco's revision

REPOSITORY
R108 KWin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D14542?vs=44095&id=44498

BRANCH
arcpatch-D14542_1

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

AFFECTED FILES
kcmkwin/kwindesktop/CMakeLists.txt
kcmkwin/kwindesktop/Messages.sh
kcmkwin/kwindesktop/desktop.desktop
kcmkwin/kwindesktop/desktopnameswidget.cpp
kcmkwin/kwindesktop/desktopnameswidget.h
kcmkwin/kwindesktop/desktopsmodel.cpp
kcmkwin/kwindesktop/desktopsmodel.h
kcmkwin/kwindesktop/kcm_kwin_virtualdesktops.desktop
kcmkwin/kwindesktop/main.cpp
kcmkwin/kwindesktop/main.h
kcmkwin/kwindesktop/main.ui
kcmkwin/kwindesktop/package/contents/ui/main.qml
kcmkwin/kwindesktop/package/metadata.desktop
kcmkwin/kwindesktop/virtualdesktops.cpp
kcmkwin/kwindesktop/virtualdesktops.h

To: hein, mart, davidedmundson, ltoscano, zzag
Cc: davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Eike Hein]

hein added a comment.


Should be OK now

REPOSITORY
R108 KWin

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

To: hein, mart, davidedmundson, ltoscano, zzag
Cc: davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Vlad Zagorodniy]

zzag accepted this revision.
zzag added a comment.


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

REPOSITORY
R108 KWin

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

To: hein, mart, davidedmundson, ltoscano, zzag
Cc: davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, ngraham, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Eike Hein]

hein added a comment.

Post by Vlad Zagorodniy
Please wait for others.
- 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

REPOSITORY
R108 KWin

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

To: hein, mart, davidedmundson, ltoscano, zzag
Cc: davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, ngraham, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[David Edmundson]

davidedmundson requested changes to this revision.
davidedmundson added a comment.
This revision now requires changes to proceed.


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.

INLINE COMMENTS

desktopsmodel.cpp:202
+
+ m_desktops[desktopIndex] = id;
+ m_names[id] = name;

this is setting it to the value it already is

desktopsmodel.cpp:251
+ s_virtualDesktopsInterface,
+ QStringLiteral("removeDesktop"));
+

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

REPOSITORY
R108 KWin

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

To: hein, mart, davidedmundson, ltoscano, zzag
Cc: davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, ngraham, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Vlad Zagorodniy]

zzag requested changes to this revision.
zzag added inline comments.

INLINE COMMENTS

desktopsmodel.cpp:413
+ // so we can determine when we are in sync.
+ const QString &dummyId = m_desktops.at(data.position);
+ m_names.remove(dummyId);

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.

REPOSITORY
R108 KWin

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

To: hein, mart, davidedmundson, ltoscano, zzag
Cc: davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, ngraham, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Vlad Zagorodniy]

zzag added inline comments.

INLINE COMMENTS

desktopsmodel.cpp:253
+
+ call.setArguments({m_desktops.last()});
+

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.

REPOSITORY
R108 KWin

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

To: hein, mart, davidedmundson, ltoscano, zzag
Cc: davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, ngraham, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Eike Hein]

hein updated this revision to Diff 44671.
hein added a comment.


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

REPOSITORY
R108 KWin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D14542?vs=44498&id=44671

BRANCH
arcpatch-D14542_2

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

AFFECTED FILES
CMakeLists.txt
abstract_client.cpp
abstract_client.h
autotests/CMakeLists.txt
autotests/integration/virtual_desktop_test.cpp
autotests/test_window_paint_data.cpp
dbusinterface.cpp
dbusinterface.h
deleted.cpp
deleted.h
effects.cpp
effects/desktopgrid/desktopgrid.cpp
effects/desktopgrid/desktopgrid.h
kcmkwin/kwindesktop/CMakeLists.txt
kcmkwin/kwindesktop/Messages.sh
kcmkwin/kwindesktop/desktop.desktop
kcmkwin/kwindesktop/desktopnameswidget.cpp
kcmkwin/kwindesktop/desktopnameswidget.h
kcmkwin/kwindesktop/desktopsmodel.cpp
kcmkwin/kwindesktop/desktopsmodel.h
kcmkwin/kwindesktop/kcm_kwin_virtualdesktops.desktop
kcmkwin/kwindesktop/main.cpp
kcmkwin/kwindesktop/main.h
kcmkwin/kwindesktop/main.ui
kcmkwin/kwindesktop/package/contents/ui/main.qml
kcmkwin/kwindesktop/package/metadata.desktop
kcmkwin/kwindesktop/virtualdesktops.cpp
kcmkwin/kwindesktop/virtualdesktops.h
libkwineffects/kwineffects.cpp
libkwineffects/kwineffects.h
org.kde.KWin.VirtualDesktopManager.xml
toplevel.h
unmanaged.cpp
unmanaged.h
useractions.cpp
useractions.h
virtualdesktops.cpp
virtualdesktops.h
virtualdesktopsdbustypes.cpp
virtualdesktopsdbustypes.h
wayland_server.cpp
wayland_server.h
workspace.cpp
workspace.h

To: hein, mart, davidedmundson, ltoscano, zzag
Cc: davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, ngraham, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Eike Hein]

hein updated this revision to Diff 44672.
hein retitled this revision from "KCM using new virtual desktops DBus interface" to "[kcmkwin/desktop] KCM using new virtual desktops DBus interface".
hein added a comment.


Update the title.

REPOSITORY
R108 KWin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D14542?vs=44671&id=44672

BRANCH
arcpatch-D14542_2

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

AFFECTED FILES
kcmkwin/kwindesktop/CMakeLists.txt
kcmkwin/kwindesktop/Messages.sh
kcmkwin/kwindesktop/desktop.desktop
kcmkwin/kwindesktop/desktopnameswidget.cpp
kcmkwin/kwindesktop/desktopnameswidget.h
kcmkwin/kwindesktop/desktopsmodel.cpp
kcmkwin/kwindesktop/desktopsmodel.h
kcmkwin/kwindesktop/kcm_kwin_virtualdesktops.desktop
kcmkwin/kwindesktop/main.cpp
kcmkwin/kwindesktop/main.h
kcmkwin/kwindesktop/main.ui
kcmkwin/kwindesktop/package/contents/ui/main.qml
kcmkwin/kwindesktop/package/metadata.desktop
kcmkwin/kwindesktop/virtualdesktops.cpp
kcmkwin/kwindesktop/virtualdesktops.h

To: hein, mart, davidedmundson, ltoscano, zzag
Cc: davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, ngraham, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Eike Hein]

hein updated this revision to Diff 44673.
hein added a comment.


Fix build error.

REPOSITORY
R108 KWin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D14542?vs=44672&id=44673

BRANCH
arcpatch-D14542_2

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

AFFECTED FILES
kcmkwin/kwindesktop/CMakeLists.txt
kcmkwin/kwindesktop/Messages.sh
kcmkwin/kwindesktop/desktop.desktop
kcmkwin/kwindesktop/desktopnameswidget.cpp
kcmkwin/kwindesktop/desktopnameswidget.h
kcmkwin/kwindesktop/desktopsmodel.cpp
kcmkwin/kwindesktop/desktopsmodel.h
kcmkwin/kwindesktop/kcm_kwin_virtualdesktops.desktop
kcmkwin/kwindesktop/main.cpp
kcmkwin/kwindesktop/main.h
kcmkwin/kwindesktop/main.ui
kcmkwin/kwindesktop/package/contents/ui/main.qml
kcmkwin/kwindesktop/package/metadata.desktop
kcmkwin/kwindesktop/virtualdesktops.cpp
kcmkwin/kwindesktop/virtualdesktops.h

To: hein, mart, davidedmundson, ltoscano, zzag
Cc: davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, ngraham, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Vlad Zagorodniy]

zzag added a comment.


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:

F6377804: Screenshot_20181101_225444.png <https://phabricator.kde.org/F6377804>
(it will try to reach the maximum number of virtual desktops)

REPOSITORY
R108 KWin

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

To: hein, mart, davidedmundson, ltoscano, zzag
Cc: davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, ngraham, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Eike Hein]

hein updated this revision to Diff 46545.
hein added a comment.


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.

REPOSITORY
R108 KWin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D14542?vs=44673&id=46545

BRANCH
master

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

AFFECTED FILES
kcmkwin/kwindesktop/CMakeLists.txt
kcmkwin/kwindesktop/Messages.sh
kcmkwin/kwindesktop/desktop.desktop
kcmkwin/kwindesktop/desktopnameswidget.cpp
kcmkwin/kwindesktop/desktopnameswidget.h
kcmkwin/kwindesktop/desktopsmodel.cpp
kcmkwin/kwindesktop/desktopsmodel.h
kcmkwin/kwindesktop/kcm_kwin_virtualdesktops.desktop
kcmkwin/kwindesktop/main.cpp
kcmkwin/kwindesktop/main.h
kcmkwin/kwindesktop/main.ui
kcmkwin/kwindesktop/package/contents/ui/main.qml
kcmkwin/kwindesktop/package/metadata.desktop
kcmkwin/kwindesktop/virtualdesktops.cpp
kcmkwin/kwindesktop/virtualdesktops.h

To: hein, mart, davidedmundson, ltoscano, zzag
Cc: davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Eike Hein]

hein updated this revision to Diff 46551.
hein added a comment.


Rebase on master for good measure

REPOSITORY
R108 KWin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D14542?vs=46545&id=46551

BRANCH
master

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

AFFECTED FILES
kcmkwin/kwindesktop/CMakeLists.txt
kcmkwin/kwindesktop/Messages.sh
kcmkwin/kwindesktop/desktop.desktop
kcmkwin/kwindesktop/desktopnameswidget.cpp
kcmkwin/kwindesktop/desktopnameswidget.h
kcmkwin/kwindesktop/desktopsmodel.cpp
kcmkwin/kwindesktop/desktopsmodel.h
kcmkwin/kwindesktop/kcm_kwin_virtualdesktops.desktop
kcmkwin/kwindesktop/main.cpp
kcmkwin/kwindesktop/main.h
kcmkwin/kwindesktop/main.ui
kcmkwin/kwindesktop/package/contents/ui/main.qml
kcmkwin/kwindesktop/package/metadata.desktop
kcmkwin/kwindesktop/virtualdesktops.cpp
kcmkwin/kwindesktop/virtualdesktops.h

To: hein, mart, davidedmundson, ltoscano, zzag
Cc: davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Eike Hein]

hein updated this revision to Diff 46554.
hein added a comment.


Handle KWin restarts

REPOSITORY
R108 KWin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D14542?vs=46551&id=46554

BRANCH
master

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

AFFECTED FILES
kcmkwin/kwindesktop/CMakeLists.txt
kcmkwin/kwindesktop/Messages.sh
kcmkwin/kwindesktop/desktop.desktop
kcmkwin/kwindesktop/desktopnameswidget.cpp
kcmkwin/kwindesktop/desktopnameswidget.h
kcmkwin/kwindesktop/desktopsmodel.cpp
kcmkwin/kwindesktop/desktopsmodel.h
kcmkwin/kwindesktop/kcm_kwin_virtualdesktops.desktop
kcmkwin/kwindesktop/main.cpp
kcmkwin/kwindesktop/main.h
kcmkwin/kwindesktop/main.ui
kcmkwin/kwindesktop/package/contents/ui/main.qml
kcmkwin/kwindesktop/package/metadata.desktop
kcmkwin/kwindesktop/virtualdesktops.cpp
kcmkwin/kwindesktop/virtualdesktops.h

To: hein, mart, davidedmundson, ltoscano, zzag
Cc: davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Eike Hein]

hein updated this revision to Diff 46895.
hein added a comment.


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

REPOSITORY
R108 KWin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D14542?vs=46554&id=46895

BRANCH
master

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

AFFECTED FILES
kcmkwin/kwindesktop/CMakeLists.txt
kcmkwin/kwindesktop/Messages.sh
kcmkwin/kwindesktop/desktop.desktop
kcmkwin/kwindesktop/desktopnameswidget.cpp
kcmkwin/kwindesktop/desktopnameswidget.h
kcmkwin/kwindesktop/desktopsmodel.cpp
kcmkwin/kwindesktop/desktopsmodel.h
kcmkwin/kwindesktop/kcm_kwin_virtualdesktops.desktop
kcmkwin/kwindesktop/main.cpp
kcmkwin/kwindesktop/main.h
kcmkwin/kwindesktop/main.ui
kcmkwin/kwindesktop/package/contents/ui/main.qml
kcmkwin/kwindesktop/package/metadata.desktop
kcmkwin/kwindesktop/virtualdesktops.cpp
kcmkwin/kwindesktop/virtualdesktops.h

To: hein, mart, davidedmundson, ltoscano, zzag
Cc: davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Vlad Zagorodniy]

zzag added a comment.


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

+---+---+---+---+---+
| | | | | |
+---+---+---+---+---+
| |
+---+

- if any virtual desktop is removed, then System Settings window will be sent to the last virtual desktop.

INLINE COMMENTS

desktopsmodel.h:51
+ *
+ * After synchronization, the model tracks Kwin-side changes again,
+ * until the user makes further changes.

*KWin-side

desktopsmodel.h:119-124
+ QStringList m_serverSideDesktops;
+ QHash<QString,QString> m_serverSideNames;
+ int m_serverSideRows;
+ QStringList m_desktops;
+ QHash<QString,QString> m_names;
+ int m_rows;

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;

REPOSITORY
R108 KWin

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

To: hein, mart, davidedmundson, ltoscano, zzag
Cc: davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Eike Hein]

hein updated this revision to Diff 46916.
hein added a comment.


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.

REPOSITORY
R108 KWin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D14542?vs=46895&id=46916

BRANCH
master

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

AFFECTED FILES
kcmkwin/kwindesktop/CMakeLists.txt
kcmkwin/kwindesktop/Messages.sh
kcmkwin/kwindesktop/desktop.desktop
kcmkwin/kwindesktop/desktopnameswidget.cpp
kcmkwin/kwindesktop/desktopnameswidget.h
kcmkwin/kwindesktop/desktopsmodel.cpp
kcmkwin/kwindesktop/desktopsmodel.h
kcmkwin/kwindesktop/kcm_kwin_virtualdesktops.desktop
kcmkwin/kwindesktop/main.cpp
kcmkwin/kwindesktop/main.h
kcmkwin/kwindesktop/main.ui
kcmkwin/kwindesktop/package/contents/ui/main.qml
kcmkwin/kwindesktop/package/metadata.desktop
kcmkwin/kwindesktop/virtualdesktops.cpp
kcmkwin/kwindesktop/virtualdesktops.h

To: hein, mart, davidedmundson, ltoscano, zzag
Cc: davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Eike Hein]

hein added a comment.


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

Please have the latest version of @mart's D17265 <https://phabricator.kde.org/D17265> applied or you may encounter weirdness from KWin's side.

REPOSITORY
R108 KWin

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

To: hein, mart, davidedmundson, ltoscano, zzag
Cc: davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Marco Martin]

mart accepted this revision.
mart added a comment.


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)

REPOSITORY
R108 KWin

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

To: hein, mart, davidedmundson, ltoscano, zzag
Cc: davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Vlad Zagorodniy]

zzag added a comment.

Post by Vlad Zagorodniy
- 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.

REPOSITORY
R108 KWin

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

To: hein, mart, davidedmundson, ltoscano, zzag
Cc: ngraham, davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

[Eike Hein]

hein updated this revision to Diff 47240.
hein added a comment.


Add back nav wrap and OSD settings.

REPOSITORY
R108 KWin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D14542?vs=46916&id=47240

BRANCH
master

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

AFFECTED FILES
kcmkwin/kwindesktop/CMakeLists.txt
kcmkwin/kwindesktop/Messages.sh
kcmkwin/kwindesktop/desktop.desktop
kcmkwin/kwindesktop/desktopnameswidget.cpp
kcmkwin/kwindesktop/desktopnameswidget.h
kcmkwin/kwindesktop/desktopsmodel.cpp
kcmkwin/kwindesktop/desktopsmodel.h
kcmkwin/kwindesktop/kcm_kwin_virtualdesktops.desktop
kcmkwin/kwindesktop/main.cpp
kcmkwin/kwindesktop/main.h
kcmkwin/kwindesktop/main.ui
kcmkwin/kwindesktop/package/contents/ui/main.qml
kcmkwin/kwindesktop/package/metadata.desktop
kcmkwin/kwindesktop/virtualdesktops.cpp
kcmkwin/kwindesktop/virtualdesktops.h

To: hein, mart, davidedmundson, ltoscano, zzag
Cc: davidedmundson, broulik, plasma-devel, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart