Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Interactive mode: subdirectories are not sized in Windows 10 #53

Closed
mardukbp opened this issue May 20, 2020 · 7 comments
Closed

Interactive mode: subdirectories are not sized in Windows 10 #53

mardukbp opened this issue May 20, 2020 · 7 comments
Labels
help wanted Extra attention is needed windows-support

Comments

@mardukbp
Copy link

When executing dua i inside a directory in Windows 10 all subdirectories are reported as having a size of 0.0 Bytes.

@Byron Byron added help wanted Extra attention is needed windows-support labels May 21, 2020
@Byron
Copy link
Owner

Byron commented May 21, 2020

Thanks for reporting. That's an odd one, as directories are computed by looking at the sizes of all files contained within, recursively.
If files have a size, the directories should also have a size no matter which OS dua runs on.

Since I cannot try it on windows myself, a fix would have to be contributed.

@rivy
Copy link
Contributor

rivy commented Jul 21, 2020

@Byron , I've found the problem at https://github.com/Byron/dua-cli/blob/master/src/traverse.rs#L108.

data.name only contains the file name, not the full path, so data.name.size_on_disk_fast(m)... always ends up returning a 0 file size. I'm not sure why it would work on a *nix machine, but I'll look further.

I have a fix that works for windows that I'll post a PR tomorrow, if you'd like; I still need to test the change on a *nix machine first (my WSL instance doesn't format correctly).

@Byron
Copy link
Owner

Byron commented Jul 22, 2020

It's interesting, as the apparently working counterpart for the non-interactive version indeed uses the whole path. This might already be the fix here as well.

Ok, I had to try it out - and it really does want to have the filename in the entry, as it will build the path dynamically via the tree - this saves memory.

Sorry for not waiting for the PR, I was just too intrigued to finally see this issue go, but I would be happy if you could give it another round of testing on windows to verify this issue is fixed for good.

Thanks so much for your investigation and all your help!

Byron added a commit that referenced this issue Jul 22, 2020
This fixes a long standing issue on windows, #53, which relies
on the full path to be used.

On linux, it seems to be enough to use the meta-data, which is also
passed along.
On windows, the path seems to actually be used, explaining why it
wonderously didn't work on windows before.
Byron added a commit that referenced this issue Jul 22, 2020
Or so it seems…
Life is too short to look into yet another windows-path related issue.
@rivy
Copy link
Contributor

rivy commented Jul 22, 2020

It's interesting, as the apparently working counterpart for the non-interactive version indeed uses the whole path. This might already be the fix here as well.

Yep, that was the "working baseline" that I used to trace down the bug and figure out the correction.

Ok, I had to try it out - and it really does want to have the filename in the entry, as it will build the path dynamically via the tree - this saves memory.

Sorry for not waiting for the PR, I was just too intrigued to finally see this issue go, but I would be happy if you could give it another round of testing on windows to verify this issue is fixed for good.

No worries. I was fixing it in a different manner... your fix obviously better matches your coding style. 😄

And, it's working on my windows machine! 🚀

Thanks so much for your investigation and all your help!

Happy to help.

BTW, I fixed the disabled tests (see PR #61).

@Byron
Copy link
Owner

Byron commented Jul 22, 2020

Awesome! So I will be closing this issue, finally :)!

Besides, I wouldn't mind seeing an alternative solution in case you want to share your approach/fix. In any case, if you feel it might be more maintainable or easier to understand, another PR is definitely welcome 👍.
And now, back to TUI… I think the core capability of it, to me, is supporting various backends and using a buffer-diffing approach to manipulate the terminal state efficiently. Maybe one day that will become its own crate so people can build on top of it with alternative implementations - TUI's way of forcing certain ways of usage with traits is usually in my way 😅. Anyway, off-topic, rambling, done :D.

@rivy
Copy link
Contributor

rivy commented Jul 22, 2020

Besides, I wouldn't mind seeing an alternative solution in case you want to share your approach/fix. In any case, if you feel it might be more maintainable or easier to understand, another PR is definitely welcome 👍.

Nah, it was just a draft and I was still hacking/iterating on it. No grand refactoring is lost with it deleted. 😄
Your solution is quite understandable.

And now, back to TUI… I think the core capability of it, to me, is supporting various backends and using a buffer-diffing approach to manipulate the terminal state efficiently. Maybe one day that will become its own crate so people can build on top of it with alternative implementations - TUI's way of forcing certain ways of usage with traits is usually in my way 😅. Anyway, off-topic, rambling, done :D.

I'm somewhat new at rust. I look forward to seeing your progress.

Thanks for the tool and your recent work on the windows/cross-platform story!

@Byron Byron closed this as completed Jul 22, 2020
@mardukbp
Copy link
Author

Thank you so much for fixing this issue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed windows-support
Projects
None yet
Development

No branches or pull requests

3 participants