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

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.

FIXED-IN: 20.04
BUG: 190580
BUG: 158090

Test Plan

With some recursion allowed:

Without any recursion allowed:

Diff Detail

Repository
R318 Dolphin
Branch
arcpatch-D25335_1
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 19473
Build 19491: arc lint + arc unit
meven created this revision.Nov 16 2019, 11:22 AM
Restricted Application added a project: Dolphin. · View Herald TranscriptNov 16 2019, 11:22 AM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
meven requested review of this revision.Nov 16 2019, 11:22 AM
meven edited the test plan for this revision. (Show Details)Nov 16 2019, 11:24 AM
meven edited the summary of this revision. (Show Details)
meven edited the summary of this revision. (Show Details)

This is a feature I've wanted for ages, so I'm very excited to see a patch that implements it!

In terms of the UI, this shouldn't only be for Details view; it's possible to view sizes in icon view too, by adding the size metadata item below the filename. Also, I'm not too keen on that "maximum level of recursion" chooser. It seems like exposing the internal details to work around performance issues that we should try to solve instead. If the size computation happens in another thread, then there's no problem of slowing down the UI, right? Also, is this data cached at all?

meven added a comment.Nov 16 2019, 3:22 PM

This is a feature I've wanted for ages, so I'm very excited to see a patch that implements it!

In terms of the UI, this shouldn't only be for Details view; it's possible to view sizes in icon view too, by adding the size metadata item below the filename.

I plan to work on the information panel and status bar to get the same information.
I would be in favor of keeping the icon view simple and let the user use the information panel for column and icon view use cases.

Also, I'm not too keen on that "maximum level of recursion" chooser. It seems like exposing the internal details to work around performance issues that we should try to solve instead.

For sure it is not ideal, but the solution to those problems (like caching see below) are nowhere near.
And performance will affect differently users depending if they have fast or slow drives, so better give the user some control.
We could simplify this input or try to set a better default depending on kind of drive for instance.
But IMO this currently solves the needs of intermediate to power users as-is.
And we could as well improve on it later.

If the size computation happens in another thread, then there's no problem of slowing down the UI, right?

There is still a sensible impact on performance, because UI is not affected, but I/O is impacted, meaning other threads or processes accessing the filesystem can be slown down during this process.
Unless we have efficient caching of course to mitigate this.

Also, is this data cached at all?

Currently this data is not cached, I may work on that, but I don't know much about caching in dolphin UI yet.
This would be an improvement area.

I have an idea to perhaps replace the recursiveDirectorySizeLimit : using a timeout
That is, for instance, give 10ms per subdirectory 100 ms for the whole directory, to retrieve the directory size, if timeout is reached return the not-complete results : it will adapt to the IO system load and to drive speed.
This timeout might be configurable.

Also I am wondering about surfacing to users that the dir size computation is non complete.

Also I am wondering about surfacing to users that the dir size computation is non complete.

I you allow me the suggestion, you could convey that in a simple and non invasive way just by adding a more-than sign to the incomplete size (> xxx Gib).
And maybe rounding this incomplete size to one/two significant digits would help understanding that it is not a perfect calculation, while providing useful information.

ping @ngraham anything to add ?

Also I am wondering about surfacing to users that the dir size computation is non complete.

I have a working patch locally that adds "< " to folders whose size could not be fully computed "< 100GB" for instance

ngraham requested changes to this revision.EditedNov 21 2019, 6:28 PM

I gave this a try today. The performance is quite good and the feature works great! It will be very nice to have, even without any caching (though that is probably necessary IMO to improve the display and performance and to minimize disk wear for SSDs in particular). I have a few concerns and suggestions for improvement:

  1. The UI to turn this on is entirely too technical. It should be more like this:
Folder size display: (o) Number of items
                     ( ) Size of contents
                         [4] Levels deep

I'm still not fond of exposing the maximum level of recursion in the UI, but if this is absolutely necessary, that's how it should be presented IMO.

  1. After turning on the feature, size values are not updated in the UI until you refresh the view or navigate elsewhere and return.
  1. When this feature is on, the number of items still appears in the size column, even though it's no longer the sort criteria:
  1. This information needs to be available in the information panel as well.
This revision now requires changes to proceed.Nov 21 2019, 6:28 PM
meven updated this revision to Diff 70765.Dec 2 2019, 5:56 PM

Apply the setting in UI, better setting UI, remove nb of items when recurse count was selected, take into account folder size

meven added a comment.Dec 2 2019, 6:06 PM

I gave this a try today. The performance is quite good and the feature works great! It will be very nice to have, even without any caching (though that is probably necessary IMO to improve the display and performance and to minimize disk wear for SSDs in particular). I have a few concerns and suggestions for improvement:

  1. The UI to turn this on is entirely too technical. It should be more like this:

    ` Folder size display: (o) Number of items ( ) Size of contents [4] Levels deep `

I mostly followed your nice suggestion.

I'm still not fond of exposing the maximum level of recursion in the UI, but if this is absolutely necessary, that's how it should be presented IMO.

As long as we don't have a cache somewhere (and even then?), we will need to allow the user to limit the recursiveness somehow.
We can think of alternative to the recursion level setting (I suggested a timelimit, nb of files or nb of directories scanned), but I feel most users will be comfortable with it.

  1. After turning on the feature, size values are not updated in the UI until you refresh the view or navigate elsewhere and return.

Fixed

  1. When this feature is on, the number of items still appears in the size column, even though it's no longer the sort criteria:

Fixed
I thought about keeping the item count visible when real size is displayed, but it just clutters the UI.

  1. This information needs to be available in the information panel as well.

I plan to implement this in a separate diff.
Let's keep things atomic.

I'm still not super thrilled by exposing something as nerdy as the maximum depth of calculation/recursion, but before we continue on the UI design discussion, Dolphin has started reproducibly crashing every time I enter Details view with the latest version of this patch. Here's the backtrace:

1#0 __strlen_avx2 () at ../sysdeps/x86_64/multiarch/strlen-avx2.S:96
2#1 0x00007ffff54d293e in __vfprintf_internal (s=s@entry=0x7fffe8e16f70,
3 format=format@entry=0x7ffff7eabefe "%s/%s", ap=ap@entry=0x7fffe8e170b0,
4 mode_flags=mode_flags@entry=0) at vfprintf-internal.c:1688
5#2 0x00007ffff54ddf30 in __vsprintf_internal (
6 string=0x7fffe8e17240 "/home/nate/Documents/Tabletop games/40K/Inspiration",
7 maxlen=maxlen@entry=18446744073709551615,
8 format=format@entry=0x7ffff7eabefe "%s/%s", args=args@entry=0x7fffe8e170b0,
9 mode_flags=mode_flags@entry=0) at iovsprintf.c:96
10#3 0x00007ffff54bd384 in __sprintf (
11 s=s@entry=0x7fffe8e17240 "/home/nate/Documents/Tabletop games/40K/Inspiration",
12 format=format@entry=0x7ffff7eabefe "%s/%s") at sprintf.c:30
13#4 0x00007ffff7e5eab5 in walkDir (
14 dirPath=0x7fff0067706a <error: Cannot access memory at address 0x7fff0067706a>,
15 dirPath@entry=0x7fffe8e17430 "/home/nate/Documents/Tabletop games/40K",
16 countHiddenFiles=countHiddenFiles@entry=false, countDirectoriesOnly=97,
17 countDirectoriesOnly@entry=false, dirEntry=0x7fffe0014f88,
18 allowedRecursiveLevel=1818581280, allowedRecursiveLevel@entry=8)
19 at /home/nate/kde/src/dolphin/src/kitemviews/private/kdirectorycontentscounterworker.cpp:77
20#5 0x00007ffff7e5eaf3 in walkDir (
21 dirPath=dirPath@entry=0x7fffe8e17620 "/home/nate/Documents/Tabletop games",
22 countHiddenFiles=countHiddenFiles@entry=false,
23 countDirectoriesOnly=countDirectoriesOnly@entry=false, dirEntry=0x7fffe000cd40,
24 allowedRecursiveLevel=allowedRecursiveLevel@entry=9)
25 at /home/nate/kde/src/dolphin/src/kitemviews/private/kdirectorycontentscounterworker.cpp:90
26#6 0x00007ffff7e5eaf3 in walkDir (dirPath=0xcc50f8 "/home/nate/Documents",
27 countHiddenFiles=countHiddenFiles@entry=false,
28 countDirectoriesOnly=countDirectoriesOnly@entry=false, dirEntry=0x7fffe0004de0,
29 dirEntry@entry=0x0, allowedRecursiveLevel=allowedRecursiveLevel@entry=10)
30 at /home/nate/kde/src/dolphin/src/kitemviews/private/kdirectorycontentscounterworker.cpp:90
31#7 0x00007ffff7e5ec0e in KDirectoryContentsCounterWorker::subItemsCount (path=...,
32 options=...) at /usr/include/qt5/QtCore/qarraydata.h:208
33#8 0x00007ffff7e5ec77 in KDirectoryContentsCounterWorker::countDirectoryContents (
34 this=0x9ef7f0, path=..., options=...)
35 at /home/nate/kde/src/dolphin/src/kitemviews/private/kdirectorycontentscounterworker.cpp:131
36#9 0x00007ffff5c30eea in QObject::event(QEvent*) () from /usr/lib64/libQt5Core.so.5
37#10 0x00007ffff66dec62 in QApplicationPrivate::notify_helper(QObject*, QEvent*) ()
38 from /usr/lib64/libQt5Widgets.so.5
39#11 0x00007ffff66e81e0 in QApplication::notify(QObject*, QEvent*) ()
40 from /usr/lib64/libQt5Widgets.so.5
41#12 0x00007ffff5c05562 in QCoreApplication::notifyInternal2(QObject*, QEvent*) ()
42 from /usr/lib64/libQt5Core.so.5
43#13 0x00007ffff5c081f8 in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) () from /usr/lib64/libQt5Core.so.5
44#14 0x00007ffff5c5c333 in ?? () from /usr/lib64/libQt5Core.so.5
45#15 0x00007ffff4055f88 in g_main_context_dispatch () from /usr/lib64/libglib-2.0.so.0
46#16 0x00007ffff4056310 in ?? () from /usr/lib64/libglib-2.0.so.0
47#17 0x00007ffff405639f in g_main_context_iteration () from /usr/lib64/libglib-2.0.so.0
48#18 0x00007ffff5c5b971 in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/lib64/libQt5Core.so.5
49#19 0x00007ffff5c040cb in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) ()
50 from /usr/lib64/libQt5Core.so.5
51#20 0x00007ffff5a3c021 in QThread::exec() () from /usr/lib64/libQt5Core.so.5
52#21 0x00007ffff5a3d1a2 in ?? () from /usr/lib64/libQt5Core.so.5
53#22 0x00007ffff50bdf2a in start_thread (arg=<optimized out>) at pthread_create.c:479
54#23 0x00007ffff55654af in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95

src/settings/viewmodes/viewsettingstab.cpp
116

"Deep" doesn't need to be capitalized

anthonyfieroni added inline comments.
src/kitemviews/private/kdirectorycontentscounterworker.cpp
118

Same, don't allow value more than 128.

src/settings/viewmodes/viewsettingstab.cpp
239

Don't allow more than 128 (even that can be higher)

meven added a comment.Dec 3 2019, 11:01 AM

I'm still not super thrilled by exposing something as nerdy as the maximum depth of calculation/recursion, but before we continue on the UI design discussion, Dolphin has started reproducibly crashing every time I enter Details view with the latest version of this patch. Here's the backtrace:

1#0 __strlen_avx2 () at ../sysdeps/x86_64/multiarch/strlen-avx2.S:96
2#1 0x00007ffff54d293e in __vfprintf_internal (s=s@entry=0x7fffe8e16f70,
3 format=format@entry=0x7ffff7eabefe "%s/%s", ap=ap@entry=0x7fffe8e170b0,
4 mode_flags=mode_flags@entry=0) at vfprintf-internal.c:1688
5#2 0x00007ffff54ddf30 in __vsprintf_internal (
6 string=0x7fffe8e17240 "/home/nate/Documents/Tabletop games/40K/Inspiration",
7 maxlen=maxlen@entry=18446744073709551615,
8 format=format@entry=0x7ffff7eabefe "%s/%s", args=args@entry=0x7fffe8e170b0,
9 mode_flags=mode_flags@entry=0) at iovsprintf.c:96
10#3 0x00007ffff54bd384 in __sprintf (
11 s=s@entry=0x7fffe8e17240 "/home/nate/Documents/Tabletop games/40K/Inspiration",
12 format=format@entry=0x7ffff7eabefe "%s/%s") at sprintf.c:30
13#4 0x00007ffff7e5eab5 in walkDir (
14 dirPath=0x7fff0067706a <error: Cannot access memory at address 0x7fff0067706a>,
15 dirPath@entry=0x7fffe8e17430 "/home/nate/Documents/Tabletop games/40K",
16 countHiddenFiles=countHiddenFiles@entry=false, countDirectoriesOnly=97,
17 countDirectoriesOnly@entry=false, dirEntry=0x7fffe0014f88,
18 allowedRecursiveLevel=1818581280, allowedRecursiveLevel@entry=8)
19 at /home/nate/kde/src/dolphin/src/kitemviews/private/kdirectorycontentscounterworker.cpp:77
20#5 0x00007ffff7e5eaf3 in walkDir (
21 dirPath=dirPath@entry=0x7fffe8e17620 "/home/nate/Documents/Tabletop games",
22 countHiddenFiles=countHiddenFiles@entry=false,
23 countDirectoriesOnly=countDirectoriesOnly@entry=false, dirEntry=0x7fffe000cd40,
24 allowedRecursiveLevel=allowedRecursiveLevel@entry=9)
25 at /home/nate/kde/src/dolphin/src/kitemviews/private/kdirectorycontentscounterworker.cpp:90
26#6 0x00007ffff7e5eaf3 in walkDir (dirPath=0xcc50f8 "/home/nate/Documents",
27 countHiddenFiles=countHiddenFiles@entry=false,
28 countDirectoriesOnly=countDirectoriesOnly@entry=false, dirEntry=0x7fffe0004de0,
29 dirEntry@entry=0x0, allowedRecursiveLevel=allowedRecursiveLevel@entry=10)
30 at /home/nate/kde/src/dolphin/src/kitemviews/private/kdirectorycontentscounterworker.cpp:90
31#7 0x00007ffff7e5ec0e in KDirectoryContentsCounterWorker::subItemsCount (path=...,
32 options=...) at /usr/include/qt5/QtCore/qarraydata.h:208
33#8 0x00007ffff7e5ec77 in KDirectoryContentsCounterWorker::countDirectoryContents (
34 this=0x9ef7f0, path=..., options=...)
35 at /home/nate/kde/src/dolphin/src/kitemviews/private/kdirectorycontentscounterworker.cpp:131
36#9 0x00007ffff5c30eea in QObject::event(QEvent*) () from /usr/lib64/libQt5Core.so.5
37#10 0x00007ffff66dec62 in QApplicationPrivate::notify_helper(QObject*, QEvent*) ()
38 from /usr/lib64/libQt5Widgets.so.5
39#11 0x00007ffff66e81e0 in QApplication::notify(QObject*, QEvent*) ()
40 from /usr/lib64/libQt5Widgets.so.5
41#12 0x00007ffff5c05562 in QCoreApplication::notifyInternal2(QObject*, QEvent*) ()
42 from /usr/lib64/libQt5Core.so.5
43#13 0x00007ffff5c081f8 in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) () from /usr/lib64/libQt5Core.so.5
44#14 0x00007ffff5c5c333 in ?? () from /usr/lib64/libQt5Core.so.5
45#15 0x00007ffff4055f88 in g_main_context_dispatch () from /usr/lib64/libglib-2.0.so.0
46#16 0x00007ffff4056310 in ?? () from /usr/lib64/libglib-2.0.so.0
47#17 0x00007ffff405639f in g_main_context_iteration () from /usr/lib64/libglib-2.0.so.0
48#18 0x00007ffff5c5b971 in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/lib64/libQt5Core.so.5
49#19 0x00007ffff5c040cb in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) ()
50 from /usr/lib64/libQt5Core.so.5
51#20 0x00007ffff5a3c021 in QThread::exec() () from /usr/lib64/libQt5Core.so.5
52#21 0x00007ffff5a3d1a2 in ?? () from /usr/lib64/libQt5Core.so.5
53#22 0x00007ffff50bdf2a in start_thread (arg=<optimized out>) at pthread_create.c:479
54#23 0x00007ffff55654af in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95

Is your directory "/home/nate/Documents/Tabletop games/40K" owned by root or another user ?

src/kitemviews/private/kdirectorycontentscounterworker.cpp
118

Btw The UI currently max it at 20 viewsettingstab.cpp:123

meven updated this revision to Diff 70819.Dec 3 2019, 11:03 AM

s/Deep/deep

Is your directory "/home/nate/Documents/Tabletop games/40K" owned by root or another user ?

No:

ls -la "/home/nate/Documents/Tabletop games/40K"
total 19404
drwxrwxr-x  5 nate users     4096 Nov 24 09:07  .
meven updated this revision to Diff 70879.Dec 4 2019, 8:28 AM

Use a QString as buffer for filenames

meven added a comment.Dec 4 2019, 8:30 AM

Is your directory "/home/nate/Documents/Tabletop games/40K" owned by root or another user ?

No:

ls -la "/home/nate/Documents/Tabletop games/40K"
total 19404
drwxrwxr-x  5 nate users     4096 Nov 24 09:07  .

Issue should be fixed with my latest update

Thanks, it no longer crashes now.

I noticed one more quirk. When sorting by size and expanding a subfolder, the contents are sometimes (not always) not sorted correctly:

Let's talk through the setting that I'm objecting to. What happens if, say, we allow infinite recursion? Will Dolphin freeze on slow hard drive systems? What's the worst-case scenario if we allow no user-facing configuration of the performance consequences of this feature.