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
Branch
rena
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 19453
Build 19471: arc lint + arc unit
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 70853

View Options

src/widgets/renamedialog.cpp

Loading...