feat(#1389): add recurse opt for git and diag moves #2525

antznin · 2023-11-09T23:33:27Z

Would like having comments on this feature that I personally find useful. This solves the initial problem stated in #1389, I think. Also works when nvim-tree.git.show_on_dirs or nvim-tree.git.show_on_open_dirs are set to true/false.

alex-courtis

This is great! I've been wanting this for a long time...

Works beautifully:

Except for:

show_on_dirs = false - does not recurse

lua/nvim-tree/actions/moves/git.lua

doc/nvim-tree-lua.txt

antznin · 2023-11-13T03:03:30Z

Except for:

* [ ]  `show_on_dirs = false` - does not recurse

@alex-courtis This cannot work with show_on_dirs set to false because move_to_next_git_item() does not see the inner nodes statuses. Is this behavior OK? I don't see another approach that would make this work with show_on_dirs = false.

alex-courtis · 2023-11-13T04:56:47Z

Except for:
* [ ]  `show_on_dirs = false` - does not recurse
@alex-courtis This cannot work with show_on_dirs set to false because move_to_next_git_item() does not see the inner nodes statuses. Is this behavior OK? I don't see another approach that would make this work with show_on_dirs = false.

I see. It looks like we have a short-circuit to not retrieve git status on folders:

nvim-tree.lua/lua/nvim-tree/explorer/node.lua

Line 64 in c1466f9

-- dir

That's OK; we can remove that one: git status is now very cheap and will likely be fetched at some point.

lua/nvim-tree/keymap.lua

antznin · 2023-12-03T20:06:20Z

My previous commit defines a new recurse api opt, and moves the logic to actions/moves/item.lua. However there were some limitations, and perhaps bugs I faced. Let me summarize here below:

The opened case is different because there is no show_on_dirs/show_on_open_dirs for this type of move. The way it is currently implemented, it does not propagate that information to the parent. The only way I would see opened being recursive at the moment would be for the parent nodes (directories) to hold that opened information on their children.
The show_on_dirs/show_on_open_dirs options are acting strange for diagnostics. If show_on_dirs = true and show_on_open_dirs = false, the diagnostic mark is not propagated to the parent directories when they are closed. When both options are set to true the parent directory does show the diagnostic mark and the recurse works the same as for git. Git does not have that issue: the status is correctly displayed on the closed directory when show_on_dirs = true and show_on_open_dirs = false.
Taking my previous comment in consideration, diagnostics and git recurse moves do work well together though when both are enabled 👍.

I was able to map these next moves with the recurse opt like so:

  vim.keymap.set('n', ']e', function() api.node.navigate.diagnostics.next({recurse=true}) end, opts('Next Diagnostic'))
  vim.keymap.set('n', ']c', function() api.node.navigate.git.next({recurse=true}) end, opts('Next Git'))

It this the way you suggested it to be @alex-courtis?

alex-courtis · 2023-12-03T22:36:54Z

It this the way you suggested it to be @alex-courtis?

Fantastic! I'll review and test this one next weekend.

lua/nvim-tree/actions/moves/item.lua

alex-courtis · 2023-12-19T05:03:31Z

The show_on_dirs/show_on_open_dirs options are acting strange for diagnostics. If show_on_dirs = true and show_on_open_dirs = false, the diagnostic mark is not propagated to the parent directories when they are closed. When both options are set to true the parent directory does show the diagnostic mark and the recurse works the same as for git. Git does not have that issue: the status is correctly displayed on the closed directory when show_on_dirs = true and show_on_open_dirs = false.

I'm completely fine with that: there are many factors at play with diagnostics, which are usually quite lazy. Git is always known. We could try and tweak if it becomes a problem.

alex-courtis · 2023-12-19T05:04:24Z

The opened case is different because there is no show_on_dirs/show_on_open_dirs for this type of move. The way it is currently implemented, it does not propagate that information to the parent. The only way I would see opened being recursive at the moment would be for the parent nodes (directories) to hold that opened information on their children.

Many thanks for looking into that one; YAGNI

alex-courtis

Looking good; all the pieces are there and work, just needs API tidy.

Many thanks for all your work on this one and apologies for lacking the time to review.

lua/nvim-tree/api.lua

antznin · 2023-12-25T01:16:44Z

@alex-courtis Should be good now.

lua/nvim-tree/api.lua

lua/nvim-tree/actions/moves/item.lua

alex-courtis

This is looking great - change is well contained with a very small blast radius.

Same test results as #2525 (review)

Is it intended for this functionality to work when show_on_dirs = false? Did you look at always retrieving git status as per #2525 (comment) ?

If we don't intend it to work we could:

document the limitation
document the fallback behaviour

show_on_dirs = false
missing prevs
minor nits

lua/nvim-tree/actions/moves/item.lua

lua/nvim-tree/api.lua

antznin · 2023-12-30T03:26:11Z

Is it intended for this functionality to work when show_on_dirs = false? Did you look at always retrieving git status as per #2525 (comment) ?

@alex-courtis No, it will not work when show_on_dirs = false atm because the recursive mechanism relies on the original move mechanism: if no status is shown anywhere in the tree, moving is not possible even if one of the directory contains a file with a status.

Retrieving the git status for a directory when show_on_dirs = false is possible though, and would remove this limitation. I could work on this later but right now I will I will document the limitation in this PR instead.

alex-courtis · 2023-12-30T03:31:00Z

Retrieving the git status for a directory when show_on_dirs = false is possible though, and would remove this limitation. I could work on this later but right now I will I will document the limitation in this PR instead.

No worries. I will hold you to that ;)

alex-courtis

This is great, it's working exactly as advertised.

However, we should still implement the prev use cases. They will inevitably be requested as soon as users discover this new functionality, which will be surprisingly quick.

I'm not seeing any reason why we can't do that with move_next_recursive - it's quite comprehensive.

Also... merge conflicts, sorry.

doc/nvim-tree-lua.txt

The recurse opt can be used to directly go to the next item showing git/diagnostic status recursively. Signed-off-by: Antonin Godard <[email protected]>

Rename get_status to status_is_valid. Use status_is_valid function in multiple place to avoid duplicating code. Signed-off-by: Antonin Godard <[email protected]>

Signed-off-by: Antonin Godard <[email protected]>

The root node cannot have a status. Previously if moving from the root node, status_is_valid was trying to fetch the status from it and errored. Signed-off-by: Antonin Godard <[email protected]>

antznin · 2024-01-05T01:07:05Z

Hi @alex-courtis, here are 4 commits implementing next and prev recursive moves as discussed.

lua/nvim-tree/actions/moves/item.lua

doc/nvim-tree-lua.txt

alex-courtis

Everything works and is documented, great job!

move to utils.lua
remove limitation from diagnostics help

lua/nvim-tree/actions/moves/item.lua

Signed-off-by: Antonin Godard <[email protected]>

alex-courtis

Working nicely, many thanks for all your work.

Changing my git bindings now :)

…2525) * feat(#1389): add next recursive for git and diag moves The recurse opt can be used to directly go to the next item showing git/diagnostic status recursively. Signed-off-by: Antonin Godard <[email protected]> * refactor: status logic in single function Rename get_status to status_is_valid. Use status_is_valid function in multiple place to avoid duplicating code. Signed-off-by: Antonin Godard <[email protected]> * feat(#1389): add prev recursive for git and diag moves Signed-off-by: Antonin Godard <[email protected]> * fix(#1389): next recursive: take root node into account The root node cannot have a status. Previously if moving from the root node, status_is_valid was trying to fetch the status from it and errored. Signed-off-by: Antonin Godard <[email protected]> * fix(#1389): doc: remove show_on_open_dirs limitation Signed-off-by: Antonin Godard <[email protected]> * feat(#1389): move find_node_line to utils Signed-off-by: Antonin Godard <[email protected]> * feat(#1389): doc: note recursive moves are to files only, tidy --------- Signed-off-by: Antonin Godard <[email protected]> Co-authored-by: Alexander Courtis <[email protected]>

antznin marked this pull request as draft November 10, 2023 02:05

antznin marked this pull request as ready for review November 10, 2023 02:35

alex-courtis requested changes Nov 12, 2023

View reviewed changes

lua/nvim-tree/actions/moves/git.lua Outdated Show resolved Hide resolved

lua/nvim-tree/actions/moves/git.lua Outdated Show resolved Hide resolved

doc/nvim-tree-lua.txt Outdated Show resolved Hide resolved

hinell reviewed Nov 27, 2023

View reviewed changes

lua/nvim-tree/keymap.lua Outdated Show resolved Hide resolved

antznin changed the title ~~feat(#1389): add git next_recurse~~ feat(#1389): add recurse opt for git and diag moves Dec 3, 2023

antznin commented Dec 14, 2023

View reviewed changes

lua/nvim-tree/actions/moves/item.lua Outdated Show resolved Hide resolved

alex-courtis requested changes Dec 19, 2023

View reviewed changes

lua/nvim-tree/api.lua Outdated Show resolved Hide resolved

alex-courtis reviewed Dec 26, 2023

View reviewed changes

lua/nvim-tree/api.lua Outdated Show resolved Hide resolved

alex-courtis reviewed Dec 26, 2023

View reviewed changes

lua/nvim-tree/actions/moves/item.lua Outdated Show resolved Hide resolved

alex-courtis requested changes Dec 26, 2023

View reviewed changes

lua/nvim-tree/actions/moves/item.lua Outdated Show resolved Hide resolved

lua/nvim-tree/api.lua Outdated Show resolved Hide resolved

alex-courtis requested changes Dec 31, 2023

View reviewed changes

doc/nvim-tree-lua.txt Outdated Show resolved Hide resolved

antznin added 4 commits January 4, 2024 10:40

feat(#1389): add next recursive for git and diag moves

1dfd894

The recurse opt can be used to directly go to the next item showing git/diagnostic status recursively. Signed-off-by: Antonin Godard <[email protected]>

refactor: status logic in single function

74f73b5

Rename get_status to status_is_valid. Use status_is_valid function in multiple place to avoid duplicating code. Signed-off-by: Antonin Godard <[email protected]>

feat(#1389): add prev recursive for git and diag moves

9e90d3a

Signed-off-by: Antonin Godard <[email protected]>

fix(#1389): next recursive: take root node into account

cc6def0

The root node cannot have a status. Previously if moving from the root node, status_is_valid was trying to fetch the status from it and errored. Signed-off-by: Antonin Godard <[email protected]>

antznin commented Jan 5, 2024

View reviewed changes

lua/nvim-tree/actions/moves/item.lua Outdated Show resolved Hide resolved

Merge branch 'master' into master

c7d1fb1

alex-courtis reviewed Jan 6, 2024

View reviewed changes

doc/nvim-tree-lua.txt Outdated Show resolved Hide resolved

alex-courtis requested changes Jan 6, 2024

View reviewed changes

lua/nvim-tree/actions/moves/item.lua Outdated Show resolved Hide resolved

fix(#1389): doc: remove show_on_open_dirs limitation

ed2b2d5

Signed-off-by: Antonin Godard <[email protected]>

antznin and others added 2 commits January 6, 2024 09:33

feat(#1389): move find_node_line to utils

e54aa18

Signed-off-by: Antonin Godard <[email protected]>

feat(#1389): doc: note recursive moves are to files only, tidy

2d1dd93

alex-courtis approved these changes Jan 6, 2024

View reviewed changes

alex-courtis merged commit 5d13cc8 into nvim-tree:master Jan 6, 2024
4 checks passed

github-actions bot mentioned this pull request Jan 6, 2024

chore(master): release nvim-tree 0.100.0 #2617

Closed

This was referenced Jan 20, 2024

chore(master): release nvim-tree 0.100.0 #2634

Closed

chore(master): release nvim-tree 0.100.0 #2635

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(#1389): add recurse opt for git and diag moves #2525

feat(#1389): add recurse opt for git and diag moves #2525

antznin commented Nov 9, 2023

alex-courtis left a comment •

edited

Loading

antznin commented Nov 13, 2023 •

edited

Loading

alex-courtis commented Nov 13, 2023 •

edited

Loading

antznin commented Dec 3, 2023

alex-courtis commented Dec 3, 2023

alex-courtis commented Dec 19, 2023

alex-courtis commented Dec 19, 2023

alex-courtis left a comment

antznin commented Dec 25, 2023

alex-courtis left a comment •

edited

Loading

antznin commented Dec 30, 2023

alex-courtis commented Dec 30, 2023

alex-courtis left a comment •

edited

Loading

antznin commented Jan 5, 2024

alex-courtis left a comment •

edited

Loading

alex-courtis left a comment

feat(#1389): add recurse opt for git and diag moves #2525

feat(#1389): add recurse opt for git and diag moves #2525

Conversation

antznin commented Nov 9, 2023

alex-courtis left a comment • edited Loading

Choose a reason for hiding this comment

antznin commented Nov 13, 2023 • edited Loading

alex-courtis commented Nov 13, 2023 • edited Loading

antznin commented Dec 3, 2023

alex-courtis commented Dec 3, 2023

alex-courtis commented Dec 19, 2023

alex-courtis commented Dec 19, 2023

alex-courtis left a comment

Choose a reason for hiding this comment

antznin commented Dec 25, 2023

alex-courtis left a comment • edited Loading

Choose a reason for hiding this comment

antznin commented Dec 30, 2023

alex-courtis commented Dec 30, 2023

alex-courtis left a comment • edited Loading

Choose a reason for hiding this comment

antznin commented Jan 5, 2024

alex-courtis left a comment • edited Loading

Choose a reason for hiding this comment

alex-courtis left a comment

Choose a reason for hiding this comment

alex-courtis left a comment •

edited

Loading

antznin commented Nov 13, 2023 •

edited

Loading

alex-courtis commented Nov 13, 2023 •

edited

Loading

alex-courtis left a comment •

edited

Loading

alex-courtis left a comment •

edited

Loading

alex-courtis left a comment •

edited

Loading