Port to new KWorkspace API
AcceptedPublic

Authored by hein on Apr 3 2019, 9:00 PM.

Details

Reviewers
davidedmundson
Group Reviewers
Plasma
Summary

Depends on D19389.

Diff Detail

Repository
R119 Plasma Desktop
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 10450
Build 10468: arc lint + arc unit
hein created this revision.Apr 3 2019, 9:00 PM
Restricted Application added a project: Plasma. · View Herald TranscriptApr 3 2019, 9:00 PM
hein requested review of this revision.Apr 3 2019, 9:00 PM
davidedmundson accepted this revision.Apr 3 2019, 9:44 PM
davidedmundson added inline comments.
applets/kicker/plugin/systemmodel.cpp
56

qDeleteAll(m_entries)

This revision is now accepted and ready to land.Apr 3 2019, 9:44 PM
hein updated this revision to Diff 55381.Apr 3 2019, 9:51 PM

Fix up qDeleteAll usage, thanks David

apol added a subscriber: apol.Apr 3 2019, 10:04 PM
apol added inline comments.
applets/kicker/package/contents/ui/ItemGridDelegate.qml
33 ↗(On Diff #55381)

enabled: model.enabled no?

applets/kicker/package/contents/ui/ItemListDelegate.qml
33 ↗(On Diff #55381)

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

applets/kickoff/package/contents/ui/KickoffItem.qml
31 ↗(On Diff #55381)

enabled: model.enabled

hein added inline comments.Apr 4 2019, 12:39 PM
applets/kicker/package/contents/ui/ItemGridDelegate.qml
33 ↗(On Diff #55381)

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.

apol added inline comments.Apr 4 2019, 7:05 PM
applets/kicker/package/contents/ui/ItemGridDelegate.qml
33 ↗(On Diff #55381)

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

broulik added a subscriber: broulik.Apr 4 2019, 7:15 PM
broulik added inline comments.
applets/kicker/plugin/systementry.cpp
26 ↗(On Diff #55381)

QSharedPointer :P

applets/kicker/plugin/systemmodel.cpp
42 ↗(On Diff #55381)

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

45 ↗(On Diff #55381)

Just capture entry into the lambda

46 ↗(On Diff #55381)

static_cast, also everywhere else

applets/kickoff/package/contents/ui/KickoffItem.qml
31 ↗(On Diff #55381)

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

hein added inline comments.Apr 4 2019, 9:24 PM
applets/kickoff/package/contents/ui/KickoffItem.qml
31 ↗(On Diff #55381)

Yep.

hein added inline comments.Apr 4 2019, 9:35 PM
applets/kicker/package/contents/ui/ItemGridDelegate.qml
33 ↗(On Diff #55381)

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.

hein updated this revision to Diff 55440.Apr 4 2019, 9:41 PM

Do some cleanups suggested by Kai.

Invert the enabled state role to please Aleix' eyes.

hein added inline comments.Apr 4 2019, 9:43 PM
applets/kicker/plugin/systementry.cpp
26 ↗(On Diff #55381)

Personally I tend to prefer just readable, explicit code for simple cases.

Personally I tend to prefer just readable, explicit code for simple cases.

Now the timing of my comment becomes awkward :/

applets/kicker/plugin/systementry.cpp
86

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

hein added a comment.Apr 5 2019, 4:44 PM

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 😎

hein updated this revision to Diff 55487.Apr 5 2019, 4:47 PM

Fix refcounting

Happened because refresh used to be init

hein updated this revision to Diff 59660.Jun 12 2019, 4:02 PM
  • Fix typo
  • Rebase