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

feat: refresh all items in view / selected item #219

Merged
merged 17 commits into from Jan 17, 2024
Merged

Conversation

gosuwachu
Copy link
Contributor

@gosuwachu gosuwachu commented Jan 14, 2024

Implements feature requested in #96.

This PR includes:

  • changes in traversal to skip root element to support refreshing all items in view
  • change in wording and fixed calculation for number of "files" instead of "items"
    • there was also a bug where items deleted from a directory wouldn't refresh the file count in parent directories
  • refreshing while in the root of glob results exits glob mode
  • selection is still lost after refresh
  • changed how traversal stats are calculated and displayed

Byron and others added 13 commits January 10, 2024 20:48
- it doesn't deal with sub-trees - for that it would need awareness of the
  method that integrates tree events.
- selection handling isn't implemented, so the selection just disappears.
- if the root to be refreshed still exists, it should probably keep it selected
  instead of removing it.
- it seems useful to have some control over the scope of the refresh, and these
  are sketched with the `Refresh` enum.
@gosuwachu gosuwachu marked this pull request as ready for review January 14, 2024 15:11
@gosuwachu gosuwachu mentioned this pull request Jan 14, 2024
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 your help, it's greatly appreciated!

I only took a very quick look and wanted to see what's implemented, but couldn't see the keys to press in the help pane yet - I wanted to find out if R is refresh all, or just the selection, I wasn't sure.

Further, I'd really hope the selection could be restored after the refresh finished.

It also seems to replace the current search with another one if pressing R again or r while a search is running, even the initial search, which can lead the search stopping on the way. This might be a hidden feature though, I don't know, it felt a bit wonky until I understood what's happening.

In any case, please let me know what's still on your list or if you'd like to continue working on the PR, otherwise I might try to do my usual hands-on review once I find the time.

@gosuwachu
Copy link
Contributor Author

gosuwachu commented Jan 14, 2024

Thanks for taking a look.

I only took a very quick look and wanted to see what's implemented, but couldn't see the keys to press in the help pane yet - I wanted to find out if R is refresh all, or just the selection, I wasn't sure.

Ah, I have forgotten to add the keys to the help page. I will add that in a second. [done]

Further, I'd really hope the selection could be restored after the refresh finished.

I didn't look into restoring selection too much. What I have seen it looked more complicated than I have thought initially. Mainly because selection is based on the TreeIndex which makes sense, but after refresh all the original indexes are gone. [done]

It also seems to replace the current search with another one if pressing R again or r while a search is running, even the initial search, which can lead the search stopping on the way. This might be a hidden feature though, I don't know, it felt a bit wonky until I understood what's happening.

I have played with it a bit, and I couldn't break it, but it may be good idea to block starting another refresh if there is one already running. [done]

In any case, please let me know what's still on your list or if you'd like to continue working on the PR, otherwise I might try to do my usual hands-on review once I find the time.

I don't think there is anything left for this PR other than the things you have mentioned.

Here is a list of some other things that are worth improving separately:

  • Starting a glob search while refresh is running should be blocked [done].
  • I was also going to improve how the single input path is "expanded" to multiple input paths, but in the end decided that it is not worth putting it in this PR.
  • I was not super happy about existing code in the traversal, but this is not new and was there from the beginning. This code:
self.directory_info_per_depth_level
    .push(self.current_directory_at_depth);
self.current_directory_at_depth = EntryInfo::default();
for _ in 0..self.previous_depth {
    let dir_info = pop_or_panic(&mut self.directory_info_per_depth_level);
    self.current_directory_at_depth.size += dir_info.size;
    self.current_directory_at_depth.add_count(&dir_info);

    set_entry_info_or_panic(
        &mut traversal.tree,
        self.parent_node_idx,
        self.current_directory_at_depth,
    );
    self.parent_node_idx =
        parent_or_panic(&mut traversal.tree, self.parent_node_idx);
}

// TODO: this looks like a hack to fix the problem with the loop above 
// that doesn't seem to completely drain elements from `directory_info_per_depth_level`
let root_size = traversal.recompute_node_size(self.root_idx);
set_entry_info_or_panic(
    &mut traversal.tree,
    self.root_idx,
    EntryInfo {
        size: root_size,
        entries_count: (self.stats.entries_traversed > 0)
            .then_some(self.stats.entries_traversed),
    },
);

* Added keys to the Help page.
* Starting a new traversal is blocked if another traversal is already running.
* Glob search is blocked if traversal is already running.
@gosuwachu
Copy link
Contributor Author

gosuwachu commented Jan 14, 2024

@Byron I have done a quick solution for preserving the selection based on the name of the selected element.

This PR is ready for review.

@gosuwachu
Copy link
Contributor Author

@Byron Just checking that this has not been buried / lost among other notifications :)

It's more effort, which should be reflrected in the amount of work
done as well, which I think is more intuitive.
- show messages that indicate why sometimes key-presses are ignored
- maintain previous selection in a clearer fashion
- maintain seelction from glob-mode as well
- switch title to `entry` as it's not only 'file's there, also directories.
- also show how many entries there are visible
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 your continued contributions! It seems that all major feature requests have now been addressed thanks to you!

Once things settle a bit, one might consider to pin them down by adding journey tests, particularly for the more intricate behaviour like selection handling upon refresh - I myself managed to break it one too many times during refactor 😅.

One note shall be added here: I removed files as term, but also didn't bring back item. Instead I went with entry which I hope is some sort of middle ground. The reason for this is that an entry is a file or directory.

Once CI is green I will create a new release.

@Byron Byron merged commit bed351e into Byron:main Jan 17, 2024
2 checks passed
@Byron
Copy link
Owner

Byron commented Jan 17, 2024

@Byron Just checking that this has not been buried / lost among other notifications :)

On this note, I think it's fair to expect me to have missed something if you wait for a week or so, as me missing anything is quite unlikely due to using my inbox as task-list.
Pinging me is then subtly doing the opposite with the then probably overloaded system 😅.

@gosuwachu gosuwachu deleted the refresh branch January 17, 2024 10:09
@gosuwachu
Copy link
Contributor Author

@Byron I didn't want to ping you unnecessarily, because I am sure you are busy with other things and you will get to it once you are free, however I also got used to you responding to my PRs within hours :) Going forward I will give you more time :)

It seems that all major feature requests have now been addressed

Yes, agree. I will try to focus on adding some more tests, but after that you probably not going to receive too many PRs from me, unless someone comes with some super exciting feature that is worth implementing :) It was fun working on this project, and learning how to do TUI in Rust :)

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.

None yet

3 participants