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

[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

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

[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

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

[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=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

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

[Eike Hein]

hein added a comment.


  In D14542#338981 , @davidedmundson 
wrote:
  
  > 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

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

[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
> +
> +

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

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

[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, ::finished, this, 
> callFinished);

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

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

Minor nitpick: In order to construct QString, we'll copy the QLatin1String 
(source 
).
 Maybe, use QStringLiteral instead? (Same with the QLatin1String down below)

> desktopsmodel.cpp:384
> +
> +foreach(const KWin::DBusDesktopDataStruct , 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
> +
> +public:
> +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 , int role = Qt::DisplayRole) 
> const override;
> +int rowCount(const QModelIndex  = QModelIndex()) const 
> override;
> +

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

  int rowCount(const QModelIndex  = {}) 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
> +public:
> +explicit VirtualDesktops(QObject* parent = nullptr, const 
> QVariantList  = QVariantList());
> +~VirtualDesktops() override;

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

> virtualdesktops.h:45
> +private:
> +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

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

[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

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

[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

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

[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=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

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

[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

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

[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

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

[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=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

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

[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=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

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

[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=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

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

[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

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

[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

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

[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=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

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

[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

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

[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

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

[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

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

[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 >>(const QDBusArgument , 
> KWin::DBusDesktopDataVector )
> +{

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

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

[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=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

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

[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=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

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

[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

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

[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

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

[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

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

[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

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

[Eike Hein]

hein added a comment.


  Screenshot:
  
  F6170928: Screenshot_20180802_035857.png 


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

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