D28470: [PlasmaCore.IconItem] Refactor source handling for different types

[Konrad Materka]

kmaterka added a comment.


  Regression fixed in D29314 .

REPOSITORY
  R242 Plasma Framework (Library)

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

To: kmaterka, #plasma, broulik, apol, davidedmundson, #frameworks
Cc: ngraham, mart, davidre, cblack, kde-frameworks-devel, #plasma, LeGast00n, 
michaelh, bruns

D28470: [PlasmaCore.IconItem] Refactor source handling for different types

[Nathaniel Graham]

ngraham added a comment.


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

REPOSITORY
  R242 Plasma Framework (Library)

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

To: kmaterka, #plasma, broulik, apol, davidedmundson, #frameworks
Cc: ngraham, mart, davidre, cblack, kde-frameworks-devel, #plasma, LeGast00n, 
michaelh, bruns

D28470: [PlasmaCore.IconItem] Refactor source handling for different types

[Konrad Materka]

kmaterka added a comment.


  In D28470#660492 , @ngraham wrote:
  
  > `git bisect` says this caused https://bugs.kde.org/show_bug.cgi?id=420801.
  
  
  I will check that, sorry...

REPOSITORY
  R242 Plasma Framework (Library)

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

To: kmaterka, #plasma, broulik, apol, davidedmundson, #frameworks
Cc: ngraham, mart, davidre, cblack, kde-frameworks-devel, #plasma, LeGast00n, 
michaelh, bruns

D28470: [PlasmaCore.IconItem] Refactor source handling for different types

[Nathaniel Graham]

ngraham added a comment.


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

REPOSITORY
  R242 Plasma Framework (Library)

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

To: kmaterka, #plasma, broulik, apol, davidedmundson, #frameworks
Cc: ngraham, mart, davidre, cblack, kde-frameworks-devel, #plasma, LeGast00n, 
michaelh, bruns

D28470: [PlasmaCore.IconItem] Refactor source handling for different types

[Konrad Materka]

This revision was automatically updated to reflect the committed changes.
Closed by commit R242:5a3fb570feda: [PlasmaCore.IconItem] Refactor source 
handling for different types (authored by kmaterka).

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28470?vs=81031&id=81092

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

AFFECTED FILES
  src/declarativeimports/core/iconitem.cpp
  src/declarativeimports/core/iconitem.h

To: kmaterka, #plasma, broulik, apol, davidedmundson, #frameworks
Cc: mart, davidre, cblack, kde-frameworks-devel, #plasma, LeGast00n, michaelh, 
ngraham, bruns

D28470: [PlasmaCore.IconItem] Refactor source handling for different types

[David Edmundson]

davidedmundson accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  master

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

To: kmaterka, #plasma, broulik, apol, davidedmundson, #frameworks
Cc: mart, davidre, cblack, kde-frameworks-devel, #plasma, LeGast00n, michaelh, 
ngraham, bruns

D28470: [PlasmaCore.IconItem] Refactor source handling for different types

[Konrad Materka]

kmaterka added a reviewer: Frameworks.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: kmaterka, #plasma, broulik, apol, davidedmundson, #frameworks
Cc: mart, davidre, cblack, kde-frameworks-devel, #plasma, LeGast00n, michaelh, 
ngraham, bruns

D28470: [PlasmaCore.IconItem] Refactor source handling for different types

[Konrad Materka]

kmaterka updated this revision to Diff 81031.
kmaterka added a comment.


  Rebase to master (includes D29102 )

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28470?vs=80148&id=81031

BRANCH
  master

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

AFFECTED FILES
  src/declarativeimports/core/iconitem.cpp
  src/declarativeimports/core/iconitem.h

To: kmaterka, #plasma, broulik, apol, davidedmundson
Cc: mart, davidre, cblack, kde-frameworks-devel, #plasma, LeGast00n, michaelh, 
ngraham, bruns

D28470: [PlasmaCore.IconItem] Refactor source handling for different types

[Konrad Materka]

kmaterka added a comment.


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

REPOSITORY
  R242 Plasma Framework (Library)

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

To: kmaterka, #plasma, broulik, apol, davidedmundson
Cc: mart, davidre, cblack, kde-frameworks-devel, #plasma, LeGast00n, michaelh, 
ngraham, bruns

D28470: [PlasmaCore.IconItem] Refactor source handling for different types

[Konrad Materka]

kmaterka updated this revision to Diff 80148.
kmaterka marked an inline comment as done.
kmaterka added a comment.


  Do not inherit from QObject

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28470?vs=79283&id=80148

BRANCH
  master

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

AFFECTED FILES
  src/declarativeimports/core/iconitem.cpp
  src/declarativeimports/core/iconitem.h

To: kmaterka, #plasma, broulik, apol, davidedmundson
Cc: mart, davidre, cblack, kde-frameworks-devel, #plasma, LeGast00n, michaelh, 
ngraham, bruns

D28470: [PlasmaCore.IconItem] Refactor source handling for different types

[Konrad Materka]

kmaterka marked an inline comment as done.
kmaterka added a comment.


  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 
:)

INLINE COMMENTS

> mart wrote in 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

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

REPOSITORY
  R242 Plasma Framework (Library)

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

To: kmaterka, #plasma, broulik, apol, davidedmundson
Cc: mart, davidre, cblack, kde-frameworks-devel, #plasma, LeGast00n, michaelh, 
ngraham, bruns

D28470: [PlasmaCore.IconItem] Refactor source handling for different types

[David Edmundson]

davidedmundson added a comment.


  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.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: kmaterka, #plasma, broulik, apol, davidedmundson
Cc: mart, davidre, cblack, kde-frameworks-devel, #plasma, LeGast00n, michaelh, 
ngraham, bruns

D28470: [PlasmaCore.IconItem] Refactor source handling for different types

[Marco Martin]

mart added inline comments.

INLINE COMMENTS

> iconitem.cpp:40
>  
> +class IconItemSource : public QObject
> +{

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

REPOSITORY
  R242 Plasma Framework (Library)

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

To: kmaterka, #plasma, broulik, apol, davidedmundson
Cc: mart, davidre, cblack, kde-frameworks-devel, #plasma, LeGast00n, michaelh, 
ngraham, bruns

D28470: [PlasmaCore.IconItem] Refactor source handling for different types

[Konrad Materka]

kmaterka added a comment.


  ping, review needed :)

REPOSITORY
  R242 Plasma Framework (Library)

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

To: kmaterka, #plasma, broulik, apol, davidedmundson
Cc: davidre, cblack, kde-frameworks-devel, #plasma, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D28470: [PlasmaCore.IconItem] Refactor source handling for different types

[Konrad Materka]

kmaterka added a comment.


  Is it OK now? Any other review comments?

REPOSITORY
  R242 Plasma Framework (Library)

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

To: kmaterka, #plasma, broulik, apol, davidedmundson
Cc: davidre, cblack, kde-frameworks-devel, #plasma, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D28470: [PlasmaCore.IconItem] Refactor source handling for different types

[Konrad Materka]

kmaterka updated this revision to Diff 79283.
kmaterka added a comment.


  Change class name: *Strategy -> *Source

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28470?vs=79281&id=79283

BRANCH
  master

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

AFFECTED FILES
  src/declarativeimports/core/iconitem.cpp
  src/declarativeimports/core/iconitem.h

To: kmaterka, #plasma, broulik, apol, davidedmundson
Cc: davidre, cblack, kde-frameworks-devel, #plasma, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D28470: [PlasmaCore.IconItem] Refactor source handling for different types

[Konrad Materka]

kmaterka added a comment.


  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?

REPOSITORY
  R242 Plasma Framework (Library)

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

To: kmaterka, #plasma, broulik, apol, davidedmundson
Cc: davidre, cblack, kde-frameworks-devel, #plasma, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D28470: [PlasmaCore.IconItem] Refactor source handling for different types

[David Redondo]

davidre added a comment.


  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

REPOSITORY
  R242 Plasma Framework (Library)

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

To: kmaterka, #plasma, broulik, apol, davidedmundson
Cc: davidre, cblack, kde-frameworks-devel, #plasma, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D28470: [PlasmaCore.IconItem] Refactor source handling for different types

[Konrad Materka]

kmaterka retitled this revision from "WIP: IconItem: Refactor source handling 
for different types" to "[PlasmaCore.IconItem] Refactor source handling for 
different types".
kmaterka edited the test plan for this revision.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: kmaterka, #plasma, broulik, apol, davidedmundson
Cc: cblack, kde-frameworks-devel, #plasma, LeGast00n, GB_2, michaelh, ngraham, 
bruns