Differential D28470

[PlasmaCore.IconItem] Refactor source handling for different types
ClosedPublic
Actions

Authored by kmaterka on Mar 31 2020, 8:39 PM.

Details

Reviewers
broulik
apol
davidedmundson
Group Reviewers
Plasma
Frameworks
Commits
R242:5a3fb570feda: [PlasmaCore.IconItem] Refactor source handling for different types
Summary

There are 3 possible strategies: QIcon, QImage and SVG. This change moves logic of each of these strategies into separate class.

Test Plan

Should behave exactly the same

Diff Detail

Repository
R242 Plasma Framework (Library)
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 25784
Build 25802: arc lint + arc unit
kmaterka created this revision.Mar 31 2020, 8:39 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMar 31 2020, 8:39 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
kmaterka requested review of this revision.Mar 31 2020, 8:39 PM
kmaterka added a comment.Mar 31 2020, 8:46 PM

Do you think it is a good direction?

cblack added a subscriber: cblack.Apr 3 2020, 12:53 AM

Splitting into multiple classes seems like a good idea, but "strategy"? Seems like an odd choice of name to me.

src/declarativeimports/core/iconitem.cpp
58

Is this class necessary? I feel like this class's behaviour should be what its parent does without a child implementation.

58

Also, "Stategy" instead of "Strategy"

80

Seems unused.

kmaterka updated this revision to Diff 79279.Apr 4 2020, 11:37 AM

fix some silly errors

kmaterka marked 2 inline comments as done.Apr 4 2020, 11:49 AM
kmaterka added inline comments.
src/declarativeimports/core/iconitem.cpp
58

I wanted IconItemStrategy to be just an abstract base class, so that to have no strategy selected one needs to select null strategy explicitly.

80

removed unused variable from NullStrategy

kmaterka updated this revision to Diff 79281.Apr 4 2020, 11:50 AM
kmaterka marked an inline comment as done.

be consistent with old implementation

kmaterka retitled this revision from WIP: IconItem: Refactor source handling for different types to [PlasmaCore.IconItem] Refactor source handling for different types.Apr 4 2020, 11:51 AM
kmaterka edited the test plan for this revision. (Show Details)
davidre added a subscriber: davidre.Apr 4 2020, 11:54 AM
In D28470#640468, @cblack wrote:

Splitting into multiple classes seems like a good idea, but "strategy"? Seems like an odd choice of name to me.

I had assumed it's because of https://en.m.wikipedia.org/wiki/Strategy_pattern

kmaterka added a comment.Apr 4 2020, 12:05 PM
In D28470#641230, @davidre wrote:
In D28470#640468, @cblack wrote:

Splitting into multiple classes seems like a good idea, but "strategy"? Seems like an odd choice of name to me.

I had assumed it's because of https://en.m.wikipedia.org/wiki/Strategy_pattern

Yeah, I totally agree, it is really confusing now. I'm bad at creating class names :)
I wanted to extract source handling (logic, internal state) to separate classes. "Handler" is overused, maybe something like this:
IconItemSource (or AbstractIconItemSource, or AbstractSource), then:
QImageSource (or QImageIconItemSource)
QIconSource (or QIconIconItemSource)
SvgSource (or SvgIconItemSource)
What do you think?

kmaterka updated this revision to Diff 79283.Apr 4 2020, 12:18 PM

Change class name: *Strategy -> *Source

kmaterka added a comment.Apr 6 2020, 6:06 PM

Is it OK now? Any other review comments?

kmaterka added a comment.Apr 10 2020, 7:05 PM

ping, review needed :)

mart added a subscriber: mart.Apr 14 2020, 7:46 AM
mart added inline comments.
src/declarativeimports/core/iconitem.cpp
40

does it have to be a QObject? it doesn't have properties, signals or invokables.. qobject is an expensive class so if you don't have to use oits features is better to avoid

davidedmundson added a comment.Apr 14 2020, 9:33 AM

Note there's a unit test for IconItem worth running if you haven't already.
The refactor in general makes sense - it's a lot cleaner.

Though I'm not sure what our super long term KF6 plan for IconItem is, it'll definitely be changing quite a bit.

kmaterka marked an inline comment as done.Apr 14 2020, 6:39 PM
In D28470#647757, @davidedmundson wrote:

Note there's a unit test for IconItem worth running if you haven't already.

I've checked that already, these test were really useful!

The refactor in general makes sense - it's a lot cleaner.
Though I'm not sure what our super long term KF6 plan for IconItem is, it'll definitely be changing quite a bit.

If there is no harm now, then it should be easier to change it in the future :)

src/declarativeimports/core/iconitem.cpp
40

There is one use of connect in SvgSource but I can change the implementation.

kmaterka updated this revision to Diff 80148.Apr 14 2020, 6:40 PM
kmaterka marked an inline comment as done.

Do not inherit from QObject

kmaterka added a comment.Apr 22 2020, 5:19 PM

Any other comment? Is it OK and can be approved?

kmaterka updated this revision to Diff 81031.Apr 23 2020, 6:24 PM

Rebase to master (includes D29102)

davidedmundson accepted this revision.Apr 23 2020, 9:27 PM
This revision is now accepted and ready to land.Apr 23 2020, 9:27 PM
Closed by commit R242:5a3fb570feda: [PlasmaCore.IconItem] Refactor source handling for different types (authored by kmaterka). · Explain WhyApr 24 2020, 12:50 PM
This revision was automatically updated to reflect the committed changes.
kmaterka added a comment.Apr 30 2020, 4:33 PM
In D28470#660492, @ngraham wrote:

git bisect says this caused https://bugs.kde.org/show_bug.cgi?id=420801.

I will check that, sorry...

ngraham added a comment.Apr 30 2020, 5:44 PM

No worries, it happens to the best of us. :)

kmaterka added a comment.Apr 30 2020, 8:32 PM

Regression fixed in D29314.

Revision Contents
Changeset List

Diff 81031

View Options

src/declarativeimports/core/iconitem.h

Loading...
View Options

src/declarativeimports/core/iconitem.cpp

Loading...