D20237: Port to new KWorkspace API

hein updated this revision to Diff 59660. hein added a comment. - Fix typo - Rebase REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D20237?vs=55487=59660 BRANCH master REVISION DETAIL https://phabricator.kde.org/D20237 AFFECTED FILES

D20237: Port to new KWorkspace API

hein updated this revision to Diff 55487. hein added a comment. Fix refcounting Happened because `refresh` used to be `init` REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D20237?vs=55440=55487 BRANCH master REVISION DETAIL

D20237: Port to new KWorkspace API

hein added a comment. In D20237#443594 , @davidedmundson wrote: > > Personally I tend to prefer just readable, explicit code for simple cases. > > Now the timing of my comment becomes awkward :/ It's so readable you found the bug 

D20237: Port to new KWorkspace API

davidedmundson added a comment. > Personally I tend to prefer just readable, explicit code for simple cases. Now the timing of my comment becomes awkward :/ INLINE COMMENTS > systementry.cpp:86 > +{ > +++s_instanceCount; > you increment this on every refresh, only decrement once

D20237: Port to new KWorkspace API

hein added inline comments. INLINE COMMENTS > broulik wrote in systementry.cpp:26 > QSharedPointer :P Personally I tend to prefer just readable, explicit code for simple cases. REPOSITORY R119 Plasma Desktop BRANCH master REVISION DETAIL https://phabricator.kde.org/D20237 To: hein,

D20237: Port to new KWorkspace API

hein updated this revision to Diff 55440. hein added a comment. Do some cleanups suggested by Kai. Invert the enabled state role to please Aleix' eyes. REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D20237?vs=55381=55440 BRANCH master

D20237: Port to new KWorkspace API

hein added inline comments. INLINE COMMENTS > apol wrote in ItemGridDelegate.qml:33 > so model.enabled you want it to resolve to true? This will be hard to read to > anyone who isn't you. > > How about `model.hasOwnProperty("enabled") && model.enabled`? Or `typeof > model.enabled ===

D20237: Port to new KWorkspace API

hein added inline comments. INLINE COMMENTS > broulik wrote in KickoffItem.qml:31 > This means we now show all logout options and only disable them if not > supported? Yep. REPOSITORY R119 Plasma Desktop BRANCH master REVISION DETAIL https://phabricator.kde.org/D20237 To: hein,

D20237: Port to new KWorkspace API

broulik added inline comments. INLINE COMMENTS > systementry.cpp:26 > + > +int SystemEntry::s_instanceCount = 0; > +SessionManagement* SystemEntry::s_sessionManagement = nullptr; QSharedPointer :P > systemmodel.cpp:42 > + > +for (SystemEntry *entry : m_entries.values()) { > +

D20237: Port to new KWorkspace API

apol added inline comments. INLINE COMMENTS > hein wrote in ItemGridDelegate.qml:33 > QVariant() evaluates to false, and not all models implement this role to > return `true` by default. Hence the check for an explicit false. > > The alternative would be to patch all the models. so

D20237: Port to new KWorkspace API

hein added inline comments. INLINE COMMENTS > apol wrote in ItemGridDelegate.qml:33 > `enabled: model.enabled` no? QVariant() evaluates to false, and not all models implement this role to return `true` by default. Hence the check for an explicit false. The alternative would be to patch all

D20237: Port to new KWorkspace API

apol added inline comments. INLINE COMMENTS > ItemGridDelegate.qml:33 > > +enabled: !(model.enabled === false) > + `enabled: model.enabled` no? > ItemListDelegate.qml:33 > > -enabled: !isSeparator > +enabled: !isSeparator && !(model.enabled === false) && (!isParent || >

D20237: Port to new KWorkspace API

hein updated this revision to Diff 55381. hein added a comment. Fix up qDeleteAll usage, thanks David REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D20237?vs=55372=55381 BRANCH master REVISION DETAIL https://phabricator.kde.org/D20237

D20237: Port to new KWorkspace API

davidedmundson accepted this revision. davidedmundson added inline comments. This revision is now accepted and ready to land. INLINE COMMENTS > systemmodel.cpp:57 > { > -qDeleteAll(m_entryList); > -} > - > -void SystemModel::init() > -{ > -QList actions; > - > -actions << new

D20237: Port to new KWorkspace API

hein created this revision. hein added reviewers: Plasma, davidedmundson. Herald added a project: Plasma. hein requested review of this revision. REVISION SUMMARY Depends on D19389 . REPOSITORY R119 Plasma Desktop BRANCH master REVISION DETAIL