Differential D13048

Move redundant logic in KIO::iconNameForUrl() into KFileItem::iconName()
ClosedPublic
Actions

Authored by ngraham on May 22 2018, 11:14 PM.

Details

Reviewers
broulik
meven
apol
dfaure
Group Reviewers
Frameworks
Commits
R241:074c91a58318: Move redundant logic in KIO::iconNameForUrl() into KFileItem::iconName()
Summary

KIO::iconNameForUrl() was doing a lot of redundant work on file items that KFileItem::iconName()
already does for us. This patch moves most of that logic into KIO::iconNameForUrl() when operating
on local files (but not for remote files, which may be slower, and for which we don't want to stat).

BUG: 356045
FIXED-IN: 5.60

Test Plan

Functionality testing:

  • Tested that adding a directory with a custom icon to the Places panel resuts in the Places panel entry inheriting its custom icon:
  • Used the system with this patch in place for one year; did not find any instances of incorrect icons

Regression testing:

  • Tested that the trash changes its icon appropriately when adding an item to an empty trash, removing all items from the trash, and emptying the trash (testtrash test passes)
  • Tested that other icons on the Places panel look the same as they did before the patch

Unit testing:

  • No test regressions

Diff Detail

Repository
R241 KIO
Lint
Lint Skipped
Unit
Unit Tests Skipped
ngraham created this revision.May 22 2018, 11:14 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMay 22 2018, 11:14 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
ngraham requested review of this revision.May 22 2018, 11:14 PM
ngraham edited the test plan for this revision. (Show Details)May 22 2018, 11:15 PM
apol added a subscriber: apol.May 22 2018, 11:51 PM
apol added inline comments.
src/core/global.cpp
332

having a blocking stat isn't good. stat will take a while on some protocols and it's not even all that quick on local file systems. I guess that's why we had this code twice.

src/core/kfileitem.cpp
951 ↗(On Diff #34715)

Why's this needed if inode-directory is the same as folder?

ngraham planned changes to this revision.May 23 2018, 4:42 AM

Hmm, that makes sense. OK, I'll re-work this.

The problem here is that if we want to fix 356045, there's no getting around reading a .directory file as part of this process, but maybe I can narrowly conditionalize it so the performance penalty is acceptable.

ngraham updated this revision to Diff 34715.May 23 2018, 2:29 PM

Merge master to get those nice test fixes

ngraham planned changes to this revision.May 23 2018, 2:29 PM
ngraham abandoned this revision.May 24 2018, 2:42 AM

Let's do this in a more sensible and better-performing way with D13082 and D13083.

ngraham reclaimed this revision.Jun 25 2019, 3:19 PM
ngraham updated this revision to Diff 60649.
  • Don't stat non-local files
  • Reduce the size of the change
  • Still works
ngraham edited the summary of this revision. (Show Details)Jun 25 2019, 3:22 PM
ngraham edited the test plan for this revision. (Show Details)
ngraham added reviewers: meven, apol, dfaure.
ngraham mentioned this in D13082: Make iconFromDirectoryFile() and iconFromDesktopFile() public in KIOCore.
ngraham edited the summary of this revision. (Show Details)Jun 25 2019, 3:24 PM
ngraham marked an inline comment as done.
dfaure requested changes to this revision.Jun 27 2019, 6:58 PM
dfaure added inline comments.
src/core/global.cpp
251

I hate nested event loops (job->exec()). And I wonder if you need one.
KFileItem is able to stat local files all by itself, in process. So, wouldn't this work?

const KFileItem item(url, mt.name());
i = item.iconName();
255

It's a bit confusing to me to have this comment before the "else" statement, rather than inside the "else {" block...

This revision now requires changes to proceed.Jun 27 2019, 6:58 PM
ngraham updated this revision to Diff 60801.Jun 28 2019, 6:12 PM

Simplify logic to address review comments

ngraham marked 2 inline comments as done.Jun 28 2019, 6:12 PM
dfaure accepted this revision.Jun 29 2019, 8:47 AM
This revision is now accepted and ready to land.Jun 29 2019, 8:47 AM
Closed by commit R241:074c91a58318: Move redundant logic in KIO::iconNameForUrl() into KFileItem::iconName() (authored by ngraham). · Explain WhyJun 30 2019, 3:56 PM
This revision was automatically updated to reflect the committed changes.
meven added a comment.Jun 30 2019, 8:32 PM

Nice work !

dfaure added a comment.Jul 6 2019, 1:05 PM

This test breaks KFileItemTest: https://build.kde.org/job/Frameworks/view/Platform%20-%20SUSEQt5.12/job/kio/job/kf5-qt5%20SUSEQt5.12/136/testReport/junit/projectroot/autotests/kiocore_kfileitemtest/
QDEBUG : KFileItemTest::testIconNameForUrl(root) QUrl("file:///")
FAIL! : KFileItemTest::testIconNameForUrl(root) Compared values are not the same

Actual   (KIO::iconNameForUrl(url)): "inode-directory"
Expected (expectedIcon)            : "folder"

But maybe that's an improvement, in case all that needs to be done is to fix the test. In any case, please investigate.

ngraham added a comment.Jul 6 2019, 9:15 PM

The original version handled that, but I got complaints. :) will investigate and fix in a few hours.

Revision Contents
Changeset List

Diff 60649

View Options

src/core/global.cpp

Loading...
View Options

src/core/kfileitem.cpp

Loading...
View Options

src/ioslaves/trash/tests/testtrash.cpp

Loading...