Implement QPlatformTheme::fileIconPixmap()
ClosedPublic

Authored by eshalygin on Apr 21 2017, 1:56 PM.

Details

Summary

Implement QPlatformTheme::fileIconPixmap() to make QFileIconProvider work.

Test Plan

Manual testing

Diff Detail

Repository
R135 Integration for Qt applications in Plasma
Lint
Lint Skipped
Unit
Unit Tests Skipped
eshalygin created this revision.Apr 21 2017, 1:56 PM
Restricted Application added a project: Plasma. · View Herald TranscriptApr 21 2017, 1:56 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
markg added a subscriber: markg.Apr 21 2017, 2:26 PM

It pains me a bit to say this since it looks like you've spend quite a bit of time writing that code.
But please do look at KIO::iconNameForUrl [1] (like also suggested by Kai on reviewboard). Much of the code can likely be replaced by just using that instead.

[1] https://api.kde.org/frameworks/kio/html/namespaceKIO.html#a215707adb0153b5ba4b318785fc746ea

In D5538#103838, @markg wrote:

But please do look at KIO::iconNameForUrl [1] (like also suggested by Kai on reviewboard). Much of the code can likely be replaced by just using that instead.

It can't support QPlatformTheme::DontUseCustomDirectoryIcons flag, that is why I re-submitted here the same implementation.

I would rather not support DontUseCustomDirectoryIcons than reimplementing all of this logic :) Perhaps we could just special-case it for inode/directory to return a generic folder icon.

eshalygin updated this revision to Diff 13675.EditedApr 21 2017, 2:47 PM

OK, here is the updated patch that uses KIO::iconForURL()

markg accepted this revision.Apr 21 2017, 2:52 PM

Looks OK to me.
Wait for a second accept though.

This revision is now accepted and ready to land.Apr 21 2017, 2:52 PM
eshalygin updated this revision to Diff 13676.Apr 21 2017, 2:55 PM

Fix a typo in function name

Not ready yet.

eshalygin updated this revision to Diff 13677.Apr 21 2017, 3:01 PM

use QFileInfo::absoluteFilePath() always

Ready. Works.

@broulik ? What do you think about this implementation?

markg accepted this revision.Apr 30 2017, 11:17 PM

Ship it :)

I don't have commit rights. Could someone commit this for me, please?

markg added a comment.May 2 2017, 8:43 PM

I would prefer if someone else ships it on your behalf, my setup is rather broken.
If nobody does it, i will somewhere next weekend.

In the future, please use arcanist to submit patches (or at least specify the repository).

graesslin requested changes to this revision.May 7 2017, 1:51 PM
graesslin added a subscriber: graesslin.

Please fix the style issues.

src/platformtheme/kdeplatformtheme.cpp
124–125

please either use indentation or use a single line

src/platformtheme/kdeplatformtheme.h
48

please use override instead of Q_DECL_OVERRIDE

71

nitpick: unrelated removal of empty line

This revision now requires changes to proceed.May 7 2017, 1:51 PM
eshalygin updated this revision to Diff 14227.May 7 2017, 2:05 PM
eshalygin edited edge metadata.
eshalygin marked 3 inline comments as done.

Address review comments.

markg accepted this revision.May 7 2017, 2:43 PM

Oke by me. Commit lands in a few minutes.

This revision was automatically updated to reflect the committed changes.

Thanks a lot!