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

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

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

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

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

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

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

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

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

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

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

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

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

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,

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

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,

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

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

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

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,

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

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

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

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

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

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

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

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

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

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:

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

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:

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

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

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

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,

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

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,

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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,

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

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,

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

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