Show directory size in ruler #2343
Conversation
ui.go
Outdated
| if curr.IsDir() { | ||
| size = curr.dirSize | ||
| } else { | ||
| size = curr.Size() | ||
| } |
There was a problem hiding this comment.
This change makes it impossible to display the stat size of a directory (i.e. current behavior). I think it would make more sense to expose a new DirSize field rather than couple this functionality with the existing Size field. The type can be something like *uint64, with a pointer type to represent that it is an optional field (e.g. not a directory, or calcdirsize not called).
There was a problem hiding this comment.
Yes, but is the current behavior really useful? As I explained previously, I don't think anyone is seriously relying on seeing the stat size for a directory. (Which is why I assume the stat size also isn't shown in the info column.)
The problem that I have with introducing a new field is that it requires much more complicated logic in the rulerfile itself, because a user has to check what size he actually wants to display. My current implementation in contrast aimed to allow as smooth a transition as possible.
One compromise could be to introduce a new field TotalSize that has my current implementation, e.g. using total directory size for directories and normal size for all regular files. Then a user can just easily switch out Size with TotalSize.
I decided against a pointer type and instead to use negative values for errors, because both nil and 0 are interpreted as zero-values by the template engine. So something like {{with .Stat.Size}} can't differentiate between an empty directory or an unavailable result. However, I believe one could use something like {{if eq .Stat.Size nil}} instead, so I'm open to use a pointer type. But I feel like using negative values makes the difference more explicit to the user.
There was a problem hiding this comment.
I doubt most users will be interested in the stat size of a directory, but it doesn't make sense to not provide it given that all of the other basic stat fields (e.g. permissions, mtime, link count, etc.) are also provided. Conceptually, the left hand side of the ruler provides stat information, and other file managers like joshuto and yazi show the stat size for directories by default.
Adding a new TotalSize sounds reasonable to me, perhaps it could be extended to include dircounts as well. Or alternatively you could just export file.dirCount and file.dirSize directly as raw values. You argue that such a low-level approach results in more complicated logic, but I don't think it is that complex actually (just a few if statements), and in fact it could even be regarded as a benefit as the user gets to explicitly specify what they want displayed instead of having to rely on hard-coded high-level behavior.
As for pointer vs. negative values, from the perspective of providing an API, providing a negative value is less clear as the type implies that the value could also be -2, -3, and so on, whereas a pointer literally means a value that may or may not exist. In fact I think the original file.dirSize struct field should be changed to a pointer value as well, since it is always a non-negative value or -1. Also if you want to check if a value exists (i.e. not nil) then {{if .Value}} should be sufficient, no need for {{if eq .Value nil}}.
ruler.go
Outdated
| if s < 0 { | ||
| return "-" | ||
| } else { | ||
| return humanize(uint64(s)) | ||
| } |
There was a problem hiding this comment.
These kind of conditionals should be handled inside the actual ruler template itself (e.g. {{if lt .Field 0}}). Where possible, lf should pass in raw data values and not make any assumptions about how a user might want to process them.
There was a problem hiding this comment.
I generally agree. The only reason I even added a wrapper was because I changed the type for .Stat.Size from uint64 to int64, so I had to provide a wrapper so that the humanize function can accept int64s .
|
I certainly understand the desire for less hard-coded behaviour. Like suggested I added two fields |
2 participants
This is a continuation from #2329.
Currently the
.Stat.Sizefield displays the link size for directories. This is consistent with the behaviour fromls.It does however not allow us to show the total size of a directory. This PR fixes that, by instead using the total size of directories. (Similar to some
lsreplacements likeezaallow.) By default the size of a directory is reported as -1 and as the total size aftercalcdirsizeis used. The behaviour of normal files is unaltered.To accomodate these changes a small wrapper around the
humanizefunction has been created that displays negative values as "-". This means that existing Rulerfiles using{{.Size | humanize | printf " %5s" -}}(like the default rulerfile) will work properly without changes. It now also produces identical information to thesizefield for the info column (assumingdircountsis disabled).It might be worth to include this change directly into the
humanizefunction instead of creating a wrapper. But as thehumanizefunction is used in some other places in the code too, I didn't want to do that without consultation. As far as I can see, implementing this change into the realhumanizefunction should not cause any problems, as thehumanizefunction is never called with negative values anyway.As this PR changes the behaviour of the existing
.Stat.Sizefield it is technically breaking. However, as existing rulerfiles with{{.Size | humanize | printf " %5s" -}}will continue to work without problem I believe it will not cause any serious trouble.In addition I doubt that anyone is seriously reliant on having the link size of directories reported, so replacing it should also not cause any issues.
Lastly, it should be pointed out that with this PR the behaviour of the default rulerfile and the default
statfmtis not consistent anymore, as thestatfmtstill reports the link size of directories. But as thestatfmtis practically deprecated in favor of the rulerfile, I believe this discrepancy can be ignored.