Replace KIconThemes dependency with equivalent QIcon usage
ClosedPublic

Authored by vkrause on Feb 4 2019, 6:25 PM.

Diff Detail

Repository
R294 KBookmarks
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 7872
Build 7890: arc lint + arc unit
vkrause created this revision.Feb 4 2019, 6:25 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptFeb 4 2019, 6:25 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
vkrause requested review of this revision.Feb 4 2019, 6:25 PM
davidedmundson accepted this revision.Feb 4 2019, 6:39 PM
This revision is now accepted and ready to land.Feb 4 2019, 6:39 PM
cfeck added a subscriber: cfeck.Feb 4 2019, 9:01 PM

Are application-specific icons now accessible with QIcon::fromTheme? Think Konqueror favicons in bookmark menu.

Also, according to https://api.kde.org/frameworks/kbookmarks/html/kbookmarks-dependencies.html KBookmarks indirectly depends on KIconThemes via KTextWidgets anyway.

broulik added a subscriber: broulik.Feb 5 2019, 8:34 AM

I think QIcon::fromTheme also handles absolute paths, if given. This should be fine.

Tested Konqueror bookmarks. New bookmarks don't seem to get the favicon set until you explicitly trigger the "Update Favicon" action, afterwards they have it and it remains across restarts. So this seems to work as expected.
Regarding the indirect dependencies, yes, this does not actually change much on its own. I run into this while debugging icon loading issues in Okular on Android, which seems related to KIconThemes interfering, so less places where I need to strip it out help.

cfeck accepted this revision.EditedFeb 9 2019, 2:13 PM

Ship it!

This revision was automatically updated to reflect the committed changes.