Differential D27539

KIO::iconNameForUrl: fix searching for kde protocol icons
ClosedPublic
Actions

Authored by meven on Feb 21 2020, 10:09 AM.

Details

Reviewers
ngraham
dfaure
broulik
sitter
Group Reviewers
Frameworks
Commits
R241:eb20176d1a42: KIO::iconNameForUrl: fix searching for kde protocol icons
Summary

iconName was set to mt.iconName() at the beginning, but this returns always "application-octet-stream" making all subsequent iconName.isEmpty() check not work as expected.
This patch sets iconName to mt.iconName() only when needed.

BUG: 417922
BUG: 417921
CCBUG: 417069
FIXED-IN: 5.68

Test Plan

In dolphin:

  • open "Recent Files"
  • Ctrl+T
  • open any folder
  • Ctrl+T
  • Ctrl+F (Search
  • Search for anything, type something then return

Same for smb:/ location

In all cases the protocol icon is now visible.

Diff Detail

Repository
R241 KIO
Branch
arcpatch-D27539
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 22779
Build 22797: arc lint + arc unit
meven created this revision.Feb 21 2020, 10:09 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptFeb 21 2020, 10:09 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
meven requested review of this revision.Feb 21 2020, 10:09 AM
meven updated this revision to Diff 76087.Feb 21 2020, 10:11 AM

Simplify

meven edited the summary of this revision. (Show Details)Feb 21 2020, 10:15 AM
meven edited the test plan for this revision. (Show Details)
meven updated this revision to Diff 76088.Feb 21 2020, 10:28 AM

Clean code further

meven edited the test plan for this revision. (Show Details)Feb 21 2020, 12:59 PM
sitter edited the summary of this revision. (Show Details)Feb 21 2020, 1:28 PM
sitter accepted this revision.Feb 21 2020, 1:32 PM
sitter added a subscriber: sitter.

I've changed 417069 to CCBUG only, there's more to that issue than just the bogus fallback in this function.

Diff LGTM, besides one minor comment about not keeping that QLatin1String.

src/core/global.cpp
227

I think we can QStringLiteral this. QLatin1String has no performance advantage when it gets type converted to QString anyway.

This revision is now accepted and ready to land.Feb 21 2020, 1:32 PM
meven updated this revision to Diff 76106.Feb 21 2020, 2:03 PM

use QStringLitteral

meven marked an inline comment as done.Feb 21 2020, 2:07 PM
ngraham accepted this revision.Feb 21 2020, 2:11 PM
dfaure added a comment.Feb 21 2020, 5:22 PM

kfileitemtest still passes?

meven updated this revision to Diff 76147.Feb 22 2020, 8:41 AM

Add some tests

meven added a comment.Feb 22 2020, 8:41 AM
In D27539#615288, @dfaure wrote:

kfileitemtest still passes?

It does, and I added more tests.

A question I have is that in case we don't find an icon depending on how we determine it we can return application-octet-stream or unknown.
I guess we should return one of the two, in all cases.
I would be in favor of unknown as application may be already checking this value.
That would mean if (mt.iconName() == "application-octet-stream") return "unknown".

dfaure added a comment.Feb 22 2020, 9:14 AM

This is about an icon name. Apps don't (shouldn't) "check the value".

We should return application-octet-stream if we did find the file, but mimetype determination failed. That's what this mimetype and its icon are about.

We should return unknown if we have no clue what that URL is.

meven updated this revision to Diff 76148.Feb 22 2020, 9:29 AM

Add an https test case

meven added a comment.Feb 22 2020, 9:30 AM
In D27539#615654, @dfaure wrote:

This is about an icon name. Apps don't (shouldn't) "check the value".

We should return application-octet-stream if we did find the file, but mimetype determination failed. That's what this mimetype and its icon are about.

We should return unknown if we have no clue what that URL is.

So unless I am mistaken, you are saying the current behavior is the correct, one. And I can go with it.
The code does :

  • if no scheme "unkwown"
  • else a bunch of rules, islocalFile, KFileItem handling, http, trash, KProtocolInfo::icon...
  • else whatever db.mimeTypeForUrl(url) returns (worse case application-octet-stream)

Feel free to accept the diff ;)

dfaure requested changes to this revision.Feb 22 2020, 10:38 AM
dfaure added inline comments.
src/core/global.cpp
240 ↗(On Diff #76106)

With the new logic, I'm not sure what the mt.isDefault() is useful for. I think it can be removed.

268–269 ↗(On Diff #76106)

There's no variable called mimeTypeIcon anymore.

This revision now requires changes to proceed.Feb 22 2020, 10:38 AM
meven updated this revision to Diff 76156.Feb 22 2020, 10:41 AM
meven marked 2 inline comments as done.

Improve comment, remove unnecessary check

dfaure accepted this revision.Feb 22 2020, 10:48 AM
This revision is now accepted and ready to land.Feb 22 2020, 10:48 AM
Closed by commit R241:eb20176d1a42: KIO::iconNameForUrl: fix searching for kde protocol icons (authored by meven). · Explain WhyFeb 22 2020, 11:35 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mautotests/kfileitemtest.cpp (12 lines)
Msrc/core/global.cpp (31 lines)

Diff 76147

View Options

autotests/kfileitemtest.cpp

Loading...
View Options

src/core/global.cpp

Loading...