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

Collapse/uncollapse all files in tree #4131

Merged
merged 1 commit into from
Jan 13, 2025

Conversation

mtrajano
Copy link
Contributor

@mtrajano mtrajano commented Dec 27, 2024

Seems like this feature is highly requested #4095 #3554

Ability to collpase/expand all files in the tree both for working files and commit view files. One issue I noticed is that currently any functionality that we want to add related to the file tree has to be duplicated across both controllers/models. I would like to try to unite the functionality using generics but I would prefer if it was done as part of a separate PR as it will greatly increase the scope of this PR and doesn't look trivial. Any feedback is welcome, thanks!

Example of functionality working:
example

@stefanhaller
Copy link
Collaborator

What keymaps if any to add to these

I could imagine - for collapsing, and = for expanding. At least they are easy to reach on a US keyboard layout. Anybody got any better ideas?

should the functionality be to toggle vs collapse and uncollapse separately

I would probably do separate ones. For toggling it wouldn't be quite clear what the exact logic would be.

and if this should be added anywhere else (right now it's just for the file tree)

Yes, in commits_files_controller.go; this handles the list of files that you see when pressing enter on a commit. I'm not aware of any other places besides these two.

@jesseduffield
Copy link
Owner

I could imagine - for collapsing, and = for expanding.

That sounds good to me

@mtrajano mtrajano force-pushed the feature/collapse-all branch from 9ee58f0 to ffffe2e Compare January 6, 2025 22:01
@mtrajano mtrajano changed the title WIP: collapse/uncollapse all files in tree Collapse/uncollapse all files in tree Jan 6, 2025
@mtrajano mtrajano force-pushed the feature/collapse-all branch 3 times, most recently from ffffe2e to a9ec232 Compare January 10, 2025 19:13
@mtrajano
Copy link
Contributor Author

@jesseduffield @stefanhaller This pr is ready as well with localization and an integration test. I noticed there is a plan to add generics to generalize the logic between commit files and the files view but it does not look trivial. I can give it a shot in a separate pr although I might have some questions.

Copy link
Owner

@jesseduffield jesseduffield left a comment

Choose a reason for hiding this comment

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

Just one comment, otherwise LGTM. Tested it out and it works great

@@ -1258,6 +1262,10 @@ func EnglishTranslationSet() *TranslationSet {
NoBranchOnRemote: `This branch doesn't exist on remote. You need to push it to remote first.`,
Fetch: `Fetch`,
FetchTooltip: "Fetch changes from remote.",
CollapseAll: "Collapse all files",
CollapseAllTooltip: "Collapse all entries in the files tree",
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
CollapseAllTooltip: "Collapse all entries in the files tree",
CollapseAllTooltip: "Collapse all directories in the files tree",

Likewise with below

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fixed in d5374ac.

@jesseduffield
Copy link
Owner

I noticed there is a plan to add generics to generalize the logic between commit files and the files view but it does not look trivial. I can give it a shot in a separate pr although I might have some questions.

That would be great! Happy to answer any questions you have

@jesseduffield jesseduffield added the enhancement New feature or request label Jan 11, 2025
@stefanhaller
Copy link
Collaborator

I tested this and it works nicely, but what I'm missing is to keep the selection (if possible). This won't always be possible, e.g. when collapsing it will only be possible if a top-level entry is selected, and I'd be ok with the selection jumping to an arbitrary top-level entry when that's not the case (although it would be nice if it would jump to the top-level entry which is a parent of what's currently selected; this doesn't seem super important though). When expanding, however, it should always be possible, and it's a bit annoying that it isn't right now.

@stefanhaller
Copy link
Collaborator

Ok, I had a bit of a closer look at the code and found a few more things that can be improved. I pushed two commits, please see their commit messages for details.

Copy link
Owner

@jesseduffield jesseduffield left a comment

Choose a reason for hiding this comment

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

Those changes look good to me @stefanhaller

@stefanhaller
Copy link
Collaborator

I added one more commit (b04d1f5) that renames some UncollapseAll methods to ExpandAll, since that seems to be the terminology we're using for this.

I'm gonna squash this now, this is ready to merge from my side.

@stefanhaller stefanhaller merged commit 43106b6 into jesseduffield:master Jan 13, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants