Differential D25707

[renamedialog] Replace KIconLoader usage with QIcon::fromTheme
ClosedPublic
Actions

Authored by nicolasfella on Dec 3 2019, 12:45 AM.

Details

Reviewers
dfaure
Group Reviewers
Frameworks
Commits
R241:ee08d0b4e56e: [renamedialog] Replace KIconLoader usage with QIcon::fromTheme
Test Plan

Triggered rename dialog. Still have icons in there

Diff Detail

Repository
R241 KIO
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
nicolasfella created this revision.Dec 3 2019, 12:45 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptDec 3 2019, 12:45 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
nicolasfella requested review of this revision.Dec 3 2019, 12:45 AM
broulik added a subscriber: broulik.Dec 3 2019, 11:18 AM

From what I understood loadMimeTypeIcon has this fallback to use "Unknown" icon, see D15451

nicolasfella updated this revision to Diff 70853.Dec 3 2019, 7:54 PM
  • Fall back to application-octet-stream
nicolasfella added a comment.Dec 3 2019, 7:56 PM

It's a bit ugly that fallback is a QIcon, not a QString :/
I will send a patch to Qt

broulik added a comment.Dec 3 2019, 8:21 PM

Why is it ugly? I think the fallback is usually meant to fall back to e.g. a built-in icon pixmap, but doing a chain of fromTheme fallbacks is just fine imho.

nicolasfella added a comment.Dec 3 2019, 8:22 PM

Well, there are certainly more ugly things out there :)

dfaure accepted this revision.Apr 11 2020, 8:24 AM

loadMimeTypeIcon isn't deprecated, so what's the reason? Porting away from kiconthemes? Should we actually deprecate it?

This revision is now accepted and ready to land.Apr 11 2020, 8:24 AM
ahmadsamir added a subscriber: ahmadsamir.Apr 11 2020, 8:45 AM
In D25707#645676, @dfaure wrote:

loadMimeTypeIcon isn't deprecated, so what's the reason? Porting away from kiconthemes? Should we actually deprecate it?

It's been going on for a while https://phabricator.kde.org/T11637 :)

dfaure added a comment.Apr 11 2020, 8:47 AM

OK. Then let me change my comment to: please document in kiconloader.h how to port away from loadMimeTypeIcon, even if it's not actually deprecated.

I remember wondering the same thing at one point.

nicolasfella added a comment.Apr 14 2020, 3:13 PM
In D25707#645694, @dfaure wrote:

OK. Then let me change my comment to: please document in kiconloader.h how to port away from loadMimeTypeIcon, even if it's not actually deprecated.

I remember wondering the same thing at one point.

Done in D28831

Closed by commit R241:ee08d0b4e56e: [renamedialog] Replace KIconLoader usage with QIcon::fromTheme (authored by nicolasfella). · Explain WhyApr 14 2020, 3:15 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Msrc/widgets/renamedialog.cpp (4 lines)

Diff 80131

View Options

src/widgets/renamedialog.cpp

Loading...