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 10420
Build 10438: 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
57

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

enabled: model.enabled no?

applets/kicker/package/contents/ui/ItemListDelegate.qml
33

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

applets/kickoff/package/contents/ui/KickoffItem.qml
31

enabled: model.enabled

hein added inline comments.Apr 4 2019, 12:39 PM
applets/kicker/package/contents/ui/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.

apol added inline comments.Apr 4 2019, 7:05 PM
applets/kicker/package/contents/ui/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

broulik added a subscriber: broulik.Apr 4 2019, 7:15 PM
broulik added inline comments.
applets/kicker/plugin/systementry.cpp
26

QSharedPointer :P

applets/kicker/plugin/systemmodel.cpp
42

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

45

Just capture entry into the lambda

46

static_cast, also everywhere else

applets/kickoff/package/contents/ui/KickoffItem.qml
31

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

Yep.

hein added inline comments.Apr 4 2019, 9:35 PM
applets/kicker/package/contents/ui/ItemGridDelegate.qml
33

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

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