D20237: Port to new KWorkspace API

[Eike Hein]

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&id=59660

BRANCH
  master

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

AFFECTED FILES
  applets/kicker/CMakeLists.txt
  applets/kicker/package/contents/ui/ItemGridDelegate.qml
  applets/kicker/package/contents/ui/ItemListDelegate.qml
  applets/kicker/plugin/abstractmodel.cpp
  applets/kicker/plugin/actionlist.h
  applets/kicker/plugin/systementry.cpp
  applets/kicker/plugin/systementry.h
  applets/kicker/plugin/systemmodel.cpp
  applets/kicker/plugin/systemmodel.h
  applets/kickoff/package/contents/ui/KickoffItem.qml

To: hein, #plasma, davidedmundson
Cc: broulik, apol, plasma-devel, LeGast00n, ericadams, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, mart

D20237: Port to new KWorkspace API

[Eike Hein]

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&id=55487

BRANCH
  master

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

AFFECTED FILES
  applets/kicker/CMakeLists.txt
  applets/kicker/package/contents/ui/ItemGridDelegate.qml
  applets/kicker/package/contents/ui/ItemListDelegate.qml
  applets/kicker/plugin/abstractmodel.cpp
  applets/kicker/plugin/actionlist.h
  applets/kicker/plugin/systementry.cpp
  applets/kicker/plugin/systementry.h
  applets/kicker/plugin/systemmodel.cpp
  applets/kicker/plugin/systemmodel.h
  applets/kickoff/package/contents/ui/KickoffItem.qml

To: hein, #plasma, davidedmundson
Cc: broulik, apol, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D20237: Port to new KWorkspace API

[Eike Hein]

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 😎

REPOSITORY
  R119 Plasma Desktop

BRANCH
  master

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

To: hein, #plasma, davidedmundson
Cc: broulik, apol, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D20237: Port to new KWorkspace API

[David Edmundson]

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 in the destructor

refresh is potentially called multiple times in the object's lifespan

refresh is called from the constructor so we may as well do it all there

REPOSITORY
  R119 Plasma Desktop

BRANCH
  master

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

To: hein, #plasma, davidedmundson
Cc: broulik, apol, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D20237: Port to new KWorkspace API

[Eike Hein]

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, #plasma, davidedmundson
Cc: broulik, apol, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D20237: Port to new KWorkspace API

[Eike Hein]

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&id=55440

BRANCH
  master

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

AFFECTED FILES
  applets/kicker/CMakeLists.txt
  applets/kicker/package/contents/ui/ItemGridDelegate.qml
  applets/kicker/package/contents/ui/ItemListDelegate.qml
  applets/kicker/plugin/abstractmodel.cpp
  applets/kicker/plugin/actionlist.h
  applets/kicker/plugin/systementry.cpp
  applets/kicker/plugin/systementry.h
  applets/kicker/plugin/systemmodel.cpp
  applets/kicker/plugin/systemmodel.h
  applets/kickoff/package/contents/ui/KickoffItem.qml

To: hein, #plasma, davidedmundson
Cc: broulik, apol, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D20237: Port to new KWorkspace API

[Eike Hein]

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 === "undefined" || model.enabled`

For the record (so someone doesn't copy this idea), model.hasOwnProperty() 
wouldn't work to catch unimplemented roles in a model.

I'll do another approach you might like better.

REPOSITORY
  R119 Plasma Desktop

BRANCH
  master

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

To: hein, #plasma, davidedmundson
Cc: broulik, apol, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D20237: Port to new KWorkspace API

[Eike Hein]

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, #plasma, davidedmundson
Cc: broulik, apol, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D20237: Port to new KWorkspace API

[Kai Uwe Broulik]

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()) {
> +QObject::connect(entry, &SystemEntry::isValidChanged, this,

range-for gives you values already, no need for `values()`

> systemmodel.cpp:45
> +[this]() {
> +SystemEntry *entry = qobject_cast *>(QObject::sender());
> +const QModelIndex &idx = index((int)entry->action(), 0);

Just capture `entry` into the lambda

> systemmodel.cpp:46
> +SystemEntry *entry = qobject_cast *>(QObject::sender());
> +const QModelIndex &idx = index((int)entry->action(), 0);
> +emit dataChanged(idx, idx, 
> QVector{Kicker::EnabledRole});

`static_cast`, also everywhere else

> KickoffItem.qml:31
>  
> +enabed: !(model.enabled === false)
> +

This means we now show all logout options and only disable them if not 
supported?

REPOSITORY
  R119 Plasma Desktop

BRANCH
  master

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

To: hein, #plasma, davidedmundson
Cc: broulik, apol, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D20237: Port to new KWorkspace API

[Aleix Pol Gonzalez]

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 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 === "undefined" || model.enabled`

REPOSITORY
  R119 Plasma Desktop

BRANCH
  master

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

To: hein, #plasma, davidedmundson
Cc: apol, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, mart

D20237: Port to new KWorkspace API

[Eike Hein]

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

REPOSITORY
  R119 Plasma Desktop

BRANCH
  master

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

To: hein, #plasma, davidedmundson
Cc: apol, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, mart

D20237: Port to new KWorkspace API

[Aleix Pol Gonzalez]

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 || 
> (isParent && hasChildren))
>  

`!isSeparator && !model.enabled && (!isParent || hasChildren)`

> KickoffItem.qml:31
>  
> +enabed: !(model.enabled === false)
> +

`enabled: model.enabled`

REPOSITORY
  R119 Plasma Desktop

BRANCH
  master

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

To: hein, #plasma, davidedmundson
Cc: apol, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, mart

D20237: Port to new KWorkspace API

[Eike Hein]

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&id=55381

BRANCH
  master

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

AFFECTED FILES
  applets/kicker/CMakeLists.txt
  applets/kicker/package/contents/ui/ItemGridDelegate.qml
  applets/kicker/package/contents/ui/ItemListDelegate.qml
  applets/kicker/plugin/abstractmodel.cpp
  applets/kicker/plugin/actionlist.h
  applets/kicker/plugin/systementry.cpp
  applets/kicker/plugin/systementry.h
  applets/kicker/plugin/systemmodel.cpp
  applets/kicker/plugin/systemmodel.h
  applets/kickoff/package/contents/ui/KickoffItem.qml

To: hein, #plasma, davidedmundson
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20237: Port to new KWorkspace API

[David Edmundson]

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 SystemEntry(this, SystemEntry::LockSession);
> -actions << new SystemEntry(this, SystemEntry::LogoutSession);
> -actions << new SystemEntry(this, SystemEntry::SaveSession);
> -actions << new SystemEntry(this, SystemEntry::SwitchUser);
> -actions << new SystemEntry(this, SystemEntry::SuspendToRam);
> -actions << new SystemEntry(this, SystemEntry::SuspendToDisk);
> -actions << new SystemEntry(this, SystemEntry::Reboot);
> -actions << new SystemEntry(this, SystemEntry::Shutdown);
> -
> -foreach(SystemEntry *entry, actions) {
> -if (entry->isValid()) {
> -m_entryList << entry;
> -} else {
> -delete entry;
> -}
> -}
> +qDeleteAll(m_entries.values());
>  }

qDeleteAll(m_entries)

REPOSITORY
  R119 Plasma Desktop

BRANCH
  master

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

To: hein, #plasma, davidedmundson
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20237: Port to new KWorkspace API

[Eike Hein]

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
  https://phabricator.kde.org/D20237

AFFECTED FILES
  applets/kicker/CMakeLists.txt
  applets/kicker/package/contents/ui/ItemGridDelegate.qml
  applets/kicker/package/contents/ui/ItemListDelegate.qml
  applets/kicker/plugin/abstractmodel.cpp
  applets/kicker/plugin/actionlist.h
  applets/kicker/plugin/systementry.cpp
  applets/kicker/plugin/systementry.h
  applets/kicker/plugin/systemmodel.cpp
  applets/kicker/plugin/systemmodel.h
  applets/kickoff/package/contents/ui/KickoffItem.qml

To: hein, #plasma, davidedmundson
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart