D29390: Respect QIcon::fallbackSearchpaths()
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()
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()
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&id=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()
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()
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()
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()
aacid added inline comments.
INLINE COMMENTS
> kiconloader.cpp:1142
> +for (const QString &path : 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()
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
