D29390: Respect QIcon::fallbackSearchpaths()

2020-06-13 Thread Nicolas Fella
nicolasfella closed this revision.

REPOSITORY
  R302 KIconThemes

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

To: nicolasfella, #plasma, #frameworks, mart
Cc: mart, kossebau, aacid, kde-frameworks-devel, LeGast00n, cblack, michaelh, 
ngraham, bruns


D29390: Respect QIcon::fallbackSearchpaths()

2020-06-13 Thread Marco Martin
mart accepted this revision.
mart added a comment.
This revision is now accepted and ready to land.


  go for it :)

REPOSITORY
  R302 KIconThemes

BRANCH
  fallback

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

To: nicolasfella, #plasma, #frameworks, mart
Cc: mart, kossebau, aacid, kde-frameworks-devel, LeGast00n, cblack, michaelh, 
ngraham, bruns


D29390: Respect QIcon::fallbackSearchpaths()

2020-05-11 Thread Nicolas Fella
nicolasfella updated this revision to Diff 82600.
nicolasfella added a comment.


  - Use array

REPOSITORY
  R302 KIconThemes

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29390?vs=81821=82600

BRANCH
  fallback

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

AFFECTED FILES
  src/kiconloader.cpp

To: nicolasfella, #plasma, #frameworks
Cc: kossebau, aacid, kde-frameworks-devel, LeGast00n, cblack, michaelh, 
ngraham, bruns


D29390: Respect QIcon::fallbackSearchpaths()

2020-05-11 Thread Nicolas Fella
nicolasfella added inline comments.

INLINE COMMENTS

> aacid wrote in kiconloader.cpp:1142
> I just realized that function is private, not really easy to use :/
> 
> anyhow do you think we should remove svgz?
> 
> Also i think using
> 
> const QStringList extensions = { QStringLiteral(".png"), 
> QStringLiteral(".svg"), QStringLiteral(".svgz"), QStringLiteral(".xpm") };
> 
> should be a bit faster

I copied that list from somewhere else in KIconLoader. It would seem weird to 
me to support a different set of formats in the fallback than in the regular 
path

REPOSITORY
  R302 KIconThemes

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

To: nicolasfella, #plasma, #frameworks
Cc: kossebau, aacid, kde-frameworks-devel, LeGast00n, cblack, michaelh, 
ngraham, bruns


D29390: Respect QIcon::fallbackSearchpaths()

2020-05-03 Thread Friedrich W. H. Kossebau
kossebau added inline comments.

INLINE COMMENTS

> aacid wrote in kiconloader.cpp:1142
> I just realized that function is private, not really easy to use :/
> 
> anyhow do you think we should remove svgz?
> 
> Also i think using
> 
> const QStringList extensions = { QStringLiteral(".png"), 
> QStringLiteral(".svg"), QStringLiteral(".svgz"), QStringLiteral(".xpm") };
> 
> should be a bit faster

You could make this even a stack-only array, even more fast due to no heap 
alloc ;)

  const QString extensions[] = { QStringLiteral(".png"), 
QStringLiteral(".svg"), QStringLiteral(".svgz") << QStringLiteral(".xpm" };

REPOSITORY
  R302 KIconThemes

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

To: nicolasfella, #plasma, #frameworks
Cc: kossebau, aacid, kde-frameworks-devel, LeGast00n, cblack, michaelh, 
ngraham, bruns


D29390: Respect QIcon::fallbackSearchpaths()

2020-05-03 Thread Albert Astals Cid
aacid added inline comments.

INLINE COMMENTS

> aacid wrote in kiconloader.cpp:1142
> Would it make sense trying to use QIconLoader::lookupFallbackIcon ? This way 
> we "upstream" Qt behaviour? For example you're supporting svgz while Qt 
> doesn't. Which would mean different QIcon::fallbackSearchPaths whether you're 
> using KIconLoader or not which doesn't seem totally great?

I just realized that function is private, not really easy to use :/

anyhow do you think we should remove svgz?

Also i think using

const QStringList extensions = { QStringLiteral(".png"), 
QStringLiteral(".svg"), QStringLiteral(".svgz"), QStringLiteral(".xpm") };

should be a bit faster

REPOSITORY
  R302 KIconThemes

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

To: nicolasfella, #plasma, #frameworks
Cc: aacid, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns


D29390: Respect QIcon::fallbackSearchpaths()

2020-05-03 Thread Albert Astals Cid
aacid added inline comments.

INLINE COMMENTS

> kiconloader.cpp:1142
> +for (const QString  : fallbackPaths) {
> +const QStringList extensions = QStringList() << 
> QStringLiteral(".png") << QStringLiteral(".svg") << QStringLiteral(".svgz") 
> << QStringLiteral(".xpm");
> +

Would it make sense trying to use QIconLoader::lookupFallbackIcon ? This way we 
"upstream" Qt behaviour? For example you're supporting svgz while Qt doesn't. 
Which would mean different QIcon::fallbackSearchPaths whether you're using 
KIconLoader or not which doesn't seem totally great?

REPOSITORY
  R302 KIconThemes

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

To: nicolasfella, #plasma, #frameworks
Cc: aacid, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns


D29390: Respect QIcon::fallbackSearchpaths()

2020-05-03 Thread Nicolas Fella
nicolasfella created this revision.
nicolasfella added reviewers: Plasma, Frameworks.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
nicolasfella requested review of this revision.

REVISION SUMMARY
  When an icon isn't found within a theme we are supposed to look for it in 
QIcon::fallbackSearchpaths() (https://doc.qt.io/qt-5/qicon.html#fromTheme).
  
  BUG: 405522

TEST PLAN
  Create /home/nico/foo.svg
  Add QIcon::setFallbackSearchPaths(QIcon::fallbackSearchPaths() << 
QStringLiteral("/home/nico/myicons")); to my app
  Use foo icon somewhere

REPOSITORY
  R302 KIconThemes

BRANCH
  fallback

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

AFFECTED FILES
  src/kiconloader.cpp

To: nicolasfella, #plasma, #frameworks
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns