search

Gruntfuggly / todo-tree

Use ripgrep to find TODO tags and display the results in a tree view
Other
1.4k stars 134 forks source link

Group by tag mode drops todos fix #832

Open baincd opened 3 months ago

baincd commented 3 months ago

This PR is a fix for #749 (while also maintaining the fix for #562)

In full transparency, I don't really understand how the TreeNodeProvider.reset method works. This PR is my attempt to merge the fix for both issues despite a lack of understanding of the logic. More details on this fix can be found here

I've done some testing of all the different TODOs view modes (flat/list/tags-only and group-by tags on/off) and it seems to fix both issues without other side effects.

@Gruntfuggly I think a fix for #749 should be considered a high priority as it is causing todos to not be displayed. Please let me know if I can help in any way.

TieWay59 commented 3 months ago

LGTM

@Gruntfuggly This fix would help solve my problem. If you got time, pls check this out. ❤

baincd commented 3 months ago

I have been testing this PR, and found a new issue this PR would introduce:

In flat or tree mode with group-by-tags enabled, when the todo tag is changed or removed, tree nodes for the old todo tag remain even if there are no more todos using that tag. For example, if the file only has a single [ ], and this is changed to [x], the todo tree has has 2 root nodes - one for [ ] (for which there are no todos), and the other for [x] Peek 2024-03-23 09-00

I have narrowed down where this is happening to at this point in the reset method: image

What happens is at this point in the code execution, when 

child.fsPath !== fullPath &&
child.isRootTagNode == true &&
config.shouldShowTagsOnly() == false
  1. if we set child.nodes = [] (as with this PR), then we see this new issue
  2. OR if we remove that (and don't make any change to child.nodes), then we see issue #749 (missing TODOs)

One theory I have is the child.fsPath value on the root node not correct in this scenario, which causes this conditional to not match. I haven't proven this theory, and I don't know enough about how the tree structure is defined to be able to confirm or test that theory.