| broulik | |
| meven | |
| apol | |
| dfaure |
| Frameworks |
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
Functionality testing:
Regression testing:
Unit testing:
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
| src/core/global.cpp | ||
|---|---|---|
| 280 | 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 #34675) | Why's this needed if inode-directory is the same as folder? |
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.
| src/core/global.cpp | ||
|---|---|---|
| 251 | I hate nested event loops (job->exec()). And I wonder if you need one. 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 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.
The original version handled that, but I got complaints. :) will investigate and fix in a few hours.