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

Adds keybinding for 'm' to toggle sorting by modified time #179

Merged
merged 3 commits into from
Nov 25, 2023

Conversation

gosuwachu
Copy link
Contributor

@gosuwachu gosuwachu commented Nov 23, 2023

Adds keybinding for 'm' to toggle sorting by modified time. Additionally appropriate column is highlighted depending on if sorting is by time or by size + mtime column is hidden by default.

Sorting by modified time + current sorting mode in the footer:
Screenshot 2023-11-23 at 22 59 52

Help:
Screenshot 2023-11-23 at 23 00 08

Addresses issue #141

Copy link
Owner

@Byron Byron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for adding such a huge feature - t's much appreciated!

I'd expect the performance to remain roughly the same as metadata is already queried anyway. The only difference should be memory consumption as the mtime field has been added.

Besides that, I'd love to retain the initial cleanness of the GUI, and start with the mtime column hidden. This can probably be implemented by adding another state to the sorting toggle.

  • initial state: mtime hidden
  • press m: sort by mtime ascending
  • press m: sort by mtime descending
  • press m: mtime hidden

Does that make sense?

Thanks for your consideration.

src/traverse.rs Show resolved Hide resolved
Copy link
Owner

@Byron Byron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for applying the requested changes.

I left a note regarding the size of the structure, if you could have a look.

Further, I ran the program and love that the mtime column now starts hidden. However, I'd expect it to cycle through the hidden state as well, and right now there is no way to get to the original, hidden, state anymore. Was this intentional, and if so what's the rationale?

Thanks again for your help in getting this feature implemented.

@gosuwachu
Copy link
Contributor Author

The way to hide the mtime column is to sort by size. You press 'm' to sort by mtime, and press 's' to sort by size. If you keep pressing 'm' or 's' you just cycle between ascending / descending. My rationale is that if the user wants to sort by mtime then that column should be visible.

@Byron
Copy link
Owner

Byron commented Nov 25, 2023

Thanks for the explanation, that also makes sense and I think it's enough to wait for feedback from users. My guess is that it can stay as is as well.

Screenshot 2023-11-25 at 14 42 36

The above is a screenshot of dua after seeing a lot of files.

After trying the same with this branch, I got a crash (wait for it :D):

└──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── mark-move = d | mark-toggle = space | toggle-all = a┘
Sort mode: size descending  Total disk usage: -  Entries: 772604 in 5s (145882thread 'main' panicked at library/std/src/sys/unix/time.rs:77:9:
                                                                                                                                              assertion failed: tv_nsec >= 0 && tv_nsec < NSEC_PER_SEC as i
64
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
                                                                               [1]    49031 abort      cargo run --release -- i ~/dev
                                                                                                                                     %

It crashes in the standard library which isn't able to deal with timestamps before 1970 with nanoseconds on MacOS. Yes, it's true, and it will panic then.

Fortunately, this was already fixed and will be available in Rust 1.75. This should just mean I have to hold back with creating a new release until then.

Let's hope I remember.

Anyway, a nightly build produced a working binary on MacOS, and here is the outcome:
Screenshot 2023-11-25 at 14 57 22

Thus nothing changed beyond a rounding error, both in terms of performance or in terms of memory consumption.

Good to go 😊👍

@Byron Byron merged commit dd523e3 into Byron:main Nov 25, 2023
@gosuwachu gosuwachu deleted the sort-by-mtime branch November 25, 2023 17:08
@gosuwachu
Copy link
Contributor Author

gosuwachu commented Nov 25, 2023

Nice find! I have learned something new, thanks for sharing.

I know this PR has already landed, but let's keep an option of reverting it and keeping it on a branch until Rust 1.75 is released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants