[Details mode] Allow to fill the column size of directories with actual size
ClosedPublic

Authored by meven on Nov 16 2019, 11:22 AM.

Details

Summary

Allow to compute the recursive size of directories to fill the details view size column.
A setting allow to set a limit to the recursive level, allowing the user to have some power over the setting.

When sorting by size and the feature is on, we get progressive ordering as the directory size are gathered.

KDirectoryContentsCounter uses a cache internally to keep results so that it can display directory size faster, but counts the dir size of directories each time it is asked to count the size a directory nevertheless and when the size has changed, it is updated.
KDirectoryContentsCounter uses one worker per instance only, meaning one process per view makes the disk spin.

FIXED-IN: 20.08
BUG: 190580
BUG: 158090

Test Plan

With some recursion allowed:

Without any recursion allowed (default):

Diff Detail

Repository
R318 Dolphin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes
meven added a comment.Apr 5 2020, 10:38 AM

I will leave it for a few days for last minute review opportunity.

meven updated this revision to Diff 79425.Apr 5 2020, 6:21 PM

Avoid recursing to hidden dirs when show hidden files is not checked, clean unused m_workersCount

ngraham added inline comments.Apr 9 2020, 11:48 PM
src/kitemviews/private/kdirectorycontentscounterworker.h
40

whitespace

41

"number" would be more readable than "nb"

src/settings/dolphin_detailsmodesettings.kcfg
49

Given that the feature is off by default, let's set this at a more reasonable default level like 10, so that when you turn it on, it's instantly useful.

src/settings/viewmodes/viewsettingstab.cpp
123

Use i18ncp() to handle plurals:

i18ncp("spinbox:suffix Directory level recursion depth", " level deep", " levels deep")
188

whitespace

meven updated this revision to Diff 79759.Apr 10 2020, 11:02 AM
meven marked 9 inline comments as done.

Add a cache for file size counting, make the file counting always asynchronous, address review

Just an idea: can Baloo figure out this for us? This sounds like a feature that would fit at the baloo level, as it recurses through directories anyway and goes through their attributes.

meven added a comment.Apr 10 2020, 4:03 PM

Just an idea: can Baloo figure out this for us? This sounds like a feature that would fit at the baloo level, as it recurses through directories anyway and goes through their attributes.

1/ this requires having baloo enabled which is not available for all users, by choice or non-Linux systems
2/ Baloo could not provide recursive Data on its own we still would need a recursive job, so it would not be magical. And the data might not be as fresh. (I believe currently baloo does not save file size as KFileMetaData::Property::Property does not have a size property)

meven updated this revision to Diff 79775.Apr 10 2020, 4:27 PM

Allow to refresh faster the sorting when sorting by file size while directoryContentsCounter is running

meven updated this revision to Diff 79777.Apr 10 2020, 4:48 PM

Share the cache within the same dolphin application instance

Tagging Localization folks to help us figure out how to handle a singular/plural string used as a spinbox suffix.

src/settings/viewmodes/viewsettingstab.cpp
123

Hmm, it looks like because this string is just used as suffix and doesn't actually include the number, the singular version is always used.

Localization folks, any ideas?

src/uiwatchdog.h
7 ↗(On Diff #79777)

Should we make that a dependency rather than copying the file into Dolphin?

meven updated this revision to Diff 79802.Apr 11 2020, 5:47 AM

Clean fix, make suffix work in settings

meven updated this revision to Diff 79803.Apr 11 2020, 5:47 AM

Fix suffix setting

ngraham accepted this revision.Apr 12 2020, 12:10 AM

Excellent work.

meven updated this revision to Diff 79897.Apr 12 2020, 6:09 AM

Remove unrelated change

meven updated this revision to Diff 79898.Apr 12 2020, 6:17 AM

Update comment

meven updated this revision to Diff 79900.Apr 12 2020, 7:03 AM

Don't break windows

meven updated this revision to Diff 79903.Apr 12 2020, 7:38 AM

Display size unknown when it is unkwown in details view size column

meven updated this revision to Diff 79908.Apr 12 2020, 9:25 AM

Handle the case when DetailsModeSettings::recursiveDirectorySizeLimit is set, but size is not kwnown

meven added inline comments.Apr 12 2020, 9:29 AM
src/kitemviews/private/kdirectorycontentscounter.cpp
32

It would be great to be able to share this cache with baloo-widgets, so that those data can be used in the metadata widget

meven updated this revision to Diff 79913.Apr 12 2020, 10:21 AM

Prevent a warning

meven added inline comments.Apr 12 2020, 6:36 PM
src/kitemviews/private/kdirectorycontentscounter.cpp
32

The Status bar info could be updated to get this data.

meven added a comment.Apr 14 2020, 6:00 AM

A few improvements we could do :

  • prioritize folders not yet in cache to be scanned first
  • prevent duplicate in queue
  • share the cache / implementation with the rest of dolphin (status bar text), this would possibly need some heavy refactoring
  • share the cache / implementation with the FileMetadataWidget so it can show the same data, this would possibly need some heavy refactoring

Also I have a branch with a singleton thread and worker, so that no matter how many tabs and split views dolphin would have as much 1 worker making the disk spin.

But this state is probably a good state to start with.

A few improvements we could do :

  • prioritize folders not yet in cache to be scanned first
  • prevent duplicate in queue
  • share the cache / implementation with the rest of dolphin (status bar text), this would possibly need some heavy refactoring
  • share the cache / implementation with the FileMetadataWidget so it can show the same data, this would possibly need some heavy refactoring

    Also I have a branch with a singleton thread and worker, so that no matter how many tabs and split views dolphin would have as much 1 worker making the disk spin.

    But this state is probably a good state to start with.

@elvisangelaccio I'd like to merge this for the time being as a MVP and make a task of next items. But I'd like to have some basic review/agreement from you as I feel since this is not a lightweight feature, you might want to have a say.

Yes, give me some days and I'll try to do a proper review. Thanks :)

elvisangelaccio requested changes to this revision.Apr 26 2020, 11:00 PM

I think we should default to "number of items", because we can't know whether the user runs an SSD or an old HDD. Better safe than sorry.

src/kitemviews/kfileitemlistwidget.cpp
68–69

Please fix this comment (or drop it).

src/kitemviews/kfileitemmodelrolesupdater.cpp
770–774

Why remove these?

src/kitemviews/private/kdirectorycontentscounter.cpp
70

Why this change? What's wrong with m_workersCount ?

src/kitemviews/private/kdirectorycontentscounterworker.cpp
47

Why toLocal8Bit() ?

50–52

These can be declared below, where they are actually used.

76

QStringLiteral

120

Why call QFile::encodeName() here and not inside walkDir() ?

src/settings/viewmodes/viewsettingstab.cpp
134

Typo: "Folder size displays"

This revision now requires changes to proceed.Apr 26 2020, 11:00 PM
meven updated this revision to Diff 81411.Apr 28 2020, 6:13 AM
meven marked 8 inline comments as done.

Add a DetailsModeSettings::directorySizeCount to properly save directory size choice, review feedback

meven added a comment.Apr 28 2020, 6:13 AM

I think we should default to "number of items", because we can't know whether the user runs an SSD or an old HDD. Better safe than sorry.

That was my intend. It was just missing proper implementation, now done with DetailsModeSettings::directorySizeCount.

src/kitemviews/kfileitemmodelrolesupdater.cpp
770–774

To allow the slotItemsChanged to be called when new data about fileSize arrives.
It is needed to have progressive ordering of folder while the worker is walking through the directories.
Otherwise the view does refresh the column size as the results arrives.
The view is still updated with a delay to batch UI changes (m_recentlyChangedItemsTimer).

src/kitemviews/private/kdirectorycontentscounter.cpp
70

I intend to have a single worker KDirectoryContentsCounter, so why bother reimplementing a refCount ?

src/kitemviews/private/kdirectorycontentscounterworker.cpp
47

QtCreator was giving me a weird warning.

meven edited the test plan for this revision. (Show Details)Apr 28 2020, 6:16 AM
meven marked 2 inline comments as done.
elvisangelaccio added inline comments.May 3 2020, 5:44 PM
src/kitemviews/kfileitemmodelrolesupdater.cpp
770–774

I see. Please do mention this "progressive ordering" in the commit message.

src/kitemviews/private/kdirectorycontentscounter.cpp
70

I see. This is valuable information, please put it in the commit message :)

elvisangelaccio accepted this revision.May 3 2020, 5:49 PM

LGTM now. Please improve the commit message and then feel free to ship it.

This revision is now accepted and ready to land.May 3 2020, 5:49 PM
meven edited the summary of this revision. (Show Details)May 4 2020, 5:25 AM
This revision was automatically updated to reflect the committed changes.

After this landed, I'm seeing that sorting by size is still sorting by number of items, not on-disk size:

This also broke the Windows build on the CI:

07:29:32  FAILED: src/CMakeFiles/dolphinprivate.dir/kitemviews/private/kdirectorycontentscounterworker.cpp.obj 
07:29:32  C:\PROGRA~2\MICROS~1\2019\PROFES~1\VC\Tools\MSVC\1424~1.283\bin\Hostx64\x64\cl.exe  /nologo /TP -DKCOREADDONS_LIB -DKF_DEPRECATED_WARNINGS_SINCE=0x060000 -DQT_CONCURRENT_LIB -DQT_CORE_LIB -DQT_DBUS_LIB -DQT_DEPRECATED_WARNINGS_SINCE=0x060000 -DQT_GUI_LIB -DQT_NETWORK_LIB -DQT_NO_NARROWING_CONVERSIONS_IN_CONNECT -DQT_NO_URL_CAST_FROM_STRING -DQT_USE_QSTRINGBUILDER -DQT_WIDGETS_LIB -DQT_XML_LIB -DTRANSLATION_DOMAIN=\"dolphin\" -DUNICODE -DWIN32_LEAN_AND_MEAN -DWINVER=0x0600 -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D_USE_MATH_DEFINES -D_WIN32_IE=0x0600 -D_WIN32_WINNT=0x0600 -Ddolphinprivate_EXPORTS -Isrc -I..\src -Isrc\dolphinprivate_autogen\include -IC:\Craft\CI-Qt514\windows-msvc2019_64-cl-debug\include\qt5 -IC:\Craft\CI-Qt514\windows-msvc2019_64-cl-debug\include\qt5\QtWidgets -IC:\Craft\CI-Qt514\windows-msvc2019_64-cl-debug\include\qt5\QtGui -IC:\Craft\CI-Qt514\windows-msvc2019_64-cl-debug\include\qt5\QtANGLE -IC:\Craft\CI-Qt514\windows-msvc2019_64-cl-debug\include\qt5\QtCore -IC:\Craft\CI-Qt514\windows-msvc2019_64-cl-debug\.\mkspecs\win32-msvc -IC:\Craft\CI-Qt514\windows-msvc2019_64-cl-debug\include\qt5\QtConcurrent -I"C:\CI\Software Installs\dolphin\include\KF5\KI18n" -I"C:\CI\Software Installs\dolphin\include\KF5" -I"C:\CI\Software Installs\dolphin\include\KF5\KIconThemes" -I"C:\CI\Software Installs\dolphin\include\KF5\KIOCore" -I"C:\CI\Software Installs\dolphin\include\KF5\KCoreAddons" -I"C:\CI\Software Installs\dolphin\include\KF5\KService" -I"C:\CI\Software Installs\dolphin\include\KF5\KConfigCore" -IC:\Craft\CI-Qt514\windows-msvc2019_64-cl-debug\include\qt5\QtNetwork -IC:\Craft\CI-Qt514\windows-msvc2019_64-cl-debug\include\qt5\QtDBus -I"C:\CI\Software Installs\dolphin\include\KF5\KIOWidgets" -I"C:\CI\Software Installs\dolphin\include\KF5\KIOGui" -I"C:\CI\Software Installs\dolphin\include\KF5\KWindowSystem" -I"C:\CI\Software Installs\dolphin\include\KF5\KJobWidgets" -I"C:\CI\Software Installs\dolphin\include\KF5\KCompletion" -I"C:\CI\Software Installs\dolphin\include\KF5\KWidgetsAddons" -I"C:\CI\Software Installs\dolphin\include\KF5\KIOFileWidgets" -I"C:\CI\Software Installs\dolphin\include\KF5\KBookmarks" -IC:\Craft\CI-Qt514\windows-msvc2019_64-cl-debug\include\qt5\QtXml -I"C:\CI\Software Installs\dolphin\include\KF5\KItemViews" -I"C:\CI\Software Installs\dolphin\include\KF5\KXmlGui" -I"C:\CI\Software Installs\dolphin\include\KF5\KConfigWidgets" -I"C:\CI\Software Installs\dolphin\include\KF5\KCodecs" -I"C:\CI\Software Installs\dolphin\include\KF5\KConfigGui" -I"C:\CI\Software Installs\dolphin\include\KF5\KAuth" -I"C:\CI\Software Installs\dolphin\include\KF5\Solid" -I"C:\CI\Software Installs\dolphin\include\KF5\KTextWidgets" -I"C:\CI\Software Installs\dolphin\include\KF5\SonnetUi" -I"C:\CI\Software Installs\dolphin\include\KF5\KNewStuff3" -I"C:\CI\Software Installs\dolphin\include\KF5\KNewStuff3\KNS3" -I"C:\CI\Software Installs\dolphin\include\KF5\KNewStuff3\knscore" -I"C:\CI\Software Installs\dolphin\include\KF5\KNewStuff3\kns3" -I"C:\CI\Software Installs\dolphin\include\KF5\KNewStuff3\KNSCore" -I"C:\CI\Software Installs\dolphin\include\KF5\Attica" -I"C:\CI\Software Installs\dolphin\include\KF5\KParts" -I"C:\CI\Software Installs\dolphin\include\KF5\KFileMetaData" -I"C:\CI\Software Installs\dolphin\include\KF5\Baloo" -I"C:\CI\Software Installs\dolphin\include\KF5\BalooWidgets" /DWIN32 /D_WINDOWS /W3 /GR /EHsc /wd4250 /wd4251 /wd4396 /wd4661 /MDd /Zi /Ob0 /Od /RTC1 /showIncludes /Fosrc\CMakeFiles\dolphinprivate.dir\kitemviews\private\kdirectorycontentscounterworker.cpp.obj /Fdsrc\CMakeFiles\dolphinprivate.dir\ /FS -c ..\src\kitemviews\private\kdirectorycontentscounterworker.cpp
07:29:32  ..\src\kitemviews\private\kdirectorycontentscounterworker.cpp(42): error C2061: syntax error: identifier 'QT_DIRENT'
07:29:32  ..\src\kitemviews\private\kdirectorycontentscounterworker.cpp(47): error C3861: 'QT_OPENDIR': identifier not found
07:29:32  ..\src\kitemviews\private\kdirectorycontentscounterworker.cpp(50): error C2065: 'QT_STATBUF': undeclared identifier
07:29:32  ..\src\kitemviews\private\kdirectorycontentscounterworker.cpp(50): error C2146: syntax error: missing ';' before identifier 'buf'
07:29:32  ..\src\kitemviews\private\kdirectorycontentscounterworker.cpp(50): error C2065: 'buf': undeclared identifier
07:29:32  ..\src\kitemviews\private\kdirectorycontentscounterworker.cpp(52): error C2065: 'dirEntry': undeclared identifier
07:29:32  ..\src\kitemviews\private\kdirectorycontentscounterworker.cpp(52): error C3861: 'QT_READDIR': identifier not found
07:29:32  ..\src\kitemviews\private\kdirectorycontentscounterworker.cpp(53): error C2065: 'dirEntry': undeclared identifier
07:29:32  ..\src\kitemviews\private\kdirectorycontentscounterworker.cpp(54): error C2065: 'dirEntry': undeclared identifier
07:29:32  ..\src\kitemviews\private\kdirectorycontentscounterworker.cpp(58): error C2065: 'dirEntry': undeclared identifier
07:29:32  ..\src\kitemviews\private\kdirectorycontentscounterworker.cpp(58): error C2065: 'dirEntry': undeclared identifier
07:29:32  ..\src\kitemviews\private\kdirectorycontentscounterworker.cpp(68): error C2065: 'dirEntry': undeclared identifier
07:29:32  ..\src\kitemviews\private\kdirectorycontentscounterworker.cpp(68): error C2065: 'DT_DIR': undeclared identifier
07:29:32  ..\src\kitemviews\private\kdirectorycontentscounterworker.cpp(69): error C2065: 'dirEntry': undeclared identifier
07:29:32  ..\src\kitemviews\private\kdirectorycontentscounterworker.cpp(69): error C2065: 'DT_LNK': undeclared identifier
07:29:32  ..\src\kitemviews\private\kdirectorycontentscounterworker.cpp(70): error C2065: 'dirEntry': undeclared identifier
07:29:32  ..\src\kitemviews\private\kdirectorycontentscounterworker.cpp(70): error C2065: 'DT_UNKNOWN': undeclared identifier
07:29:32  ..\src\kitemviews\private\kdirectorycontentscounterworker.cpp(67): error C2789: 'countEntry': an object of const-qualified type must be initialized
07:29:32  ..\src\kitemviews\private\kdirectorycontentscounterworker.cpp(67): note: see declaration of 'countEntry'
07:29:32  ..\src\kitemviews\private\kdirectorycontentscounterworker.cpp(75): error C2065: 'allowedRecursiveLevel': undeclared identifier
07:29:32  ..\src\kitemviews\private\kdirectorycontentscounterworker.cpp(78): error C2065: 'dirEntry': undeclared identifier
07:29:32  ..\src\kitemviews\private\kdirectorycontentscounterworker.cpp(80): error C2065: 'dirEntry': undeclared identifier
07:29:32  ..\src\kitemviews\private\kdirectorycontentscounterworker.cpp(80): error C2065: 'DT_REG': undeclared identifier
07:29:32  ..\src\kitemviews\private\kdirectorycontentscounterworker.cpp(80): error C2065: 'dirEntry': undeclared identifier
07:29:32  ..\src\kitemviews\private\kdirectorycontentscounterworker.cpp(80): error C2065: 'DT_LNK': undeclared identifier
07:29:32  ..\src\kitemviews\private\kdirectorycontentscounterworker.cpp(81): error C2065: 'buf': undeclared identifier
07:29:32  ..\src\kitemviews\private\kdirectorycontentscounterworker.cpp(81): error C3861: 'QT_STAT': identifier not found
07:29:32  ..\src\kitemviews\private\kdirectorycontentscounterworker.cpp(82): error C2065: 'buf': undeclared identifier
07:29:32  ..\src\kitemviews\private\kdirectorycontentscounterworker.cpp(82): error C3861: 'S_ISDIR': identifier not found
07:29:32  ..\src\kitemviews\private\kdirectorycontentscounterworker.cpp(86): error C2065: 'buf': undeclared identifier
07:29:32  ..\src\kitemviews\private\kdirectorycontentscounterworker.cpp(89): error C2065: 'dirEntry': undeclared identifier
07:29:32  ..\src\kitemviews\private\kdirectorycontentscounterworker.cpp(89): error C2065: 'DT_DIR': undeclared identifier
07:29:32  ..\src\kitemviews\private\kdirectorycontentscounterworker.cpp(91): error C2065: 'dirEntry': undeclared identifier
07:29:32  ..\src\kitemviews\private\kdirectorycontentscounterworker.cpp(91): error C2065: 'allowedRecursiveLevel': undeclared identifier
07:29:32  ..\src\kitemviews\private\kdirectorycontentscounterworker.cpp(95): error C3861: 'QT_CLOSEDIR': identifier not found

Can you have a look @meven ?

meven added a comment.May 4 2020, 6:16 PM

After this landed, I'm seeing that sorting by size is still sorting by number of items, not on-disk size:

Thank you for the quite feedback D29424
This was due to the last change of course...

@meven Can you please also fix these issues?

src/settings/viewmodes/viewsettingstab.cpp
186–187

This will crash on Windows since these member variables are never initialized on that platform.

236–243

Same for this.

meven added a comment.Wed, May 6, 6:27 AM

@meven Can you please also fix these issues?

Thanks for pointing this out.
D29470