D27539: KIO::iconNameForUrl: fix searching for kde protocol icons

2020-02-22 Thread Méven Car 
This revision was automatically updated to reflect the committed changes.
Closed by commit R241:eb20176d1a42: KIO::iconNameForUrl: fix searching for kde 
protocol icons (authored by meven).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27539?vs=76156=76162

REVISION DETAIL
  https://phabricator.kde.org/D27539

AFFECTED FILES
  autotests/kfileitemtest.cpp
  src/core/global.cpp

To: meven, ngraham, #frameworks, dfaure, broulik, sitter
Cc: sitter, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27539: KIO::iconNameForUrl: fix searching for kde protocol icons

2020-02-22 Thread David Faure 
dfaure accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D27539

REVISION DETAIL
  https://phabricator.kde.org/D27539

To: meven, ngraham, #frameworks, dfaure, broulik, sitter
Cc: sitter, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27539: KIO::iconNameForUrl: fix searching for kde protocol icons

2020-02-22 Thread Méven Car 
meven updated this revision to Diff 76156.
meven marked 2 inline comments as done.
meven added a comment.


  Improve comment, remove unnecessary check

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27539?vs=76148=76156

BRANCH
  arcpatch-D27539

REVISION DETAIL
  https://phabricator.kde.org/D27539

AFFECTED FILES
  autotests/kfileitemtest.cpp
  src/core/global.cpp

To: meven, ngraham, #frameworks, dfaure, broulik, sitter
Cc: sitter, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27539: KIO::iconNameForUrl: fix searching for kde protocol icons

2020-02-22 Thread David Faure 
dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> global.cpp:240
>  // Let KFileItem::iconName handle things for us
> -if (i == unknown || i.isEmpty() || mt.isDefault()) {
> +if (iconName.isEmpty() || mt.isDefault()) {
>  const KFileItem item(url, mt.name());

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

> global.cpp:268
>  }
> -return !i.isEmpty() ? i : unknown;
> +// if we found nothing, revert to mimeTypeIcon (which is usually 
> "application-octet-stream")
> +return !iconName.isEmpty() ? iconName : mt.iconName();

There's no variable called mimeTypeIcon anymore.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D27539

To: meven, ngraham, #frameworks, dfaure, broulik, sitter
Cc: sitter, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27539: KIO::iconNameForUrl: fix searching for kde protocol icons

2020-02-22 Thread Méven Car 
meven added a comment.


  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 ;)

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D27539

REVISION DETAIL
  https://phabricator.kde.org/D27539

To: meven, ngraham, #frameworks, dfaure, broulik, sitter
Cc: sitter, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27539: KIO::iconNameForUrl: fix searching for kde protocol icons

2020-02-22 Thread Méven Car 
meven updated this revision to Diff 76148.
meven added a comment.


  Add an https test case

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27539?vs=76147=76148

BRANCH
  arcpatch-D27539

REVISION DETAIL
  https://phabricator.kde.org/D27539

AFFECTED FILES
  autotests/kfileitemtest.cpp
  src/core/global.cpp

To: meven, ngraham, #frameworks, dfaure, broulik, sitter
Cc: sitter, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27539: KIO::iconNameForUrl: fix searching for kde protocol icons

2020-02-22 Thread David Faure 
dfaure added a comment.


  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.

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D27539

REVISION DETAIL
  https://phabricator.kde.org/D27539

To: meven, ngraham, #frameworks, dfaure, broulik, sitter
Cc: sitter, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27539: KIO::iconNameForUrl: fix searching for kde protocol icons

2020-02-22 Thread Méven Car 
meven added a comment.


  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"`.

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D27539

REVISION DETAIL
  https://phabricator.kde.org/D27539

To: meven, ngraham, #frameworks, dfaure, broulik, sitter
Cc: sitter, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27539: KIO::iconNameForUrl: fix searching for kde protocol icons

2020-02-22 Thread Méven Car 
meven updated this revision to Diff 76147.
meven added a comment.


  Add some tests

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27539?vs=76106=76147

BRANCH
  arcpatch-D27539

REVISION DETAIL
  https://phabricator.kde.org/D27539

AFFECTED FILES
  autotests/kfileitemtest.cpp
  src/core/global.cpp

To: meven, ngraham, #frameworks, dfaure, broulik, sitter
Cc: sitter, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27539: KIO::iconNameForUrl: fix searching for kde protocol icons

2020-02-21 Thread David Faure 
dfaure added a comment.


  kfileitemtest still passes?

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D27539

To: meven, ngraham, #frameworks, dfaure, broulik, sitter
Cc: sitter, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27539: KIO::iconNameForUrl: fix searching for kde protocol icons

2020-02-21 Thread Nathaniel Graham 
ngraham accepted this revision.

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D27539

To: meven, ngraham, #frameworks, dfaure, broulik, sitter
Cc: sitter, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27539: KIO::iconNameForUrl: fix searching for kde protocol icons

2020-02-21 Thread Méven Car 
meven marked an inline comment as done.

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D27539

To: meven, ngraham, #frameworks, dfaure, broulik, sitter
Cc: sitter, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27539: KIO::iconNameForUrl: fix searching for kde protocol icons

2020-02-21 Thread Méven Car 
meven updated this revision to Diff 76106.
meven added a comment.


  use QStringLitteral

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27539?vs=76088=76106

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D27539

AFFECTED FILES
  src/core/global.cpp

To: meven, ngraham, #frameworks, dfaure, broulik, sitter
Cc: sitter, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27539: KIO::iconNameForUrl: fix searching for kde protocol icons

2020-02-21 Thread Harald Sitter 
sitter accepted this revision.
sitter added a comment.
This revision is now accepted and ready to land.


  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.

INLINE COMMENTS

> global.cpp:227
>  if (url.scheme().isEmpty()) { // empty URL or relative URL (e.g. '~')
> -return unknown;
> +return QLatin1String ("unknown");
>  }

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

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D27539

To: meven, ngraham, #frameworks, dfaure, broulik, sitter
Cc: sitter, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27539: KIO::iconNameForUrl: fix searching for kde protocol icons

2020-02-21 Thread Harald Sitter 
sitter edited the summary of this revision.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D27539

To: meven, ngraham, #frameworks, dfaure, broulik
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27539: KIO::iconNameForUrl: fix searching for kde protocol icons

2020-02-21 Thread Méven Car 
meven edited the test plan for this revision.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D27539

To: meven, ngraham, #frameworks, dfaure, broulik
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27539: KIO::iconNameForUrl: fix searching for kde protocol icons

2020-02-21 Thread Méven Car 
meven added reviewers: dfaure, broulik.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D27539

To: meven, ngraham, #frameworks, dfaure, broulik
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27539: KIO::iconNameForUrl: fix searching for kde protocol icons

2020-02-21 Thread Méven Car 
meven updated this revision to Diff 76088.
meven added a comment.


  Clean code further

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27539?vs=76087=76088

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D27539

AFFECTED FILES
  src/core/global.cpp

To: meven, ngraham, #frameworks
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27539: KIO::iconNameForUrl: fix searching for kde protocol icons

2020-02-21 Thread Méven Car 
meven edited the summary of this revision.
meven edited the test plan for this revision.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D27539

To: meven, ngraham, #frameworks
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27539: KIO::iconNameForUrl: fix searching for kde protocol icons

2020-02-21 Thread Méven Car 
meven updated this revision to Diff 76087.
meven added a comment.


  Simplify

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27539?vs=76086=76087

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D27539

AFFECTED FILES
  src/core/global.cpp

To: meven, ngraham, #frameworks
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27539: KIO::iconNameForUrl: fix searching for kde protocol icons

2020-02-21 Thread Méven Car 
meven created this revision.
meven added reviewers: ngraham, Frameworks.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
meven requested review of this revision.

REVISION SUMMARY
  BUG: 417922
  BUG: 417921
  BUG: 417069 
  FIXED-IN: 5.68

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D27539

AFFECTED FILES
  src/core/global.cpp

To: meven, ngraham, #frameworks
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns