Skip to content

Group by tag mode drops todos fix#832

Open
baincd wants to merge 2 commits into
Gruntfuggly:masterfrom
baincd:group-by-tag-mode-drops-todos-fix
Open

Group by tag mode drops todos fix#832
baincd wants to merge 2 commits into
Gruntfuggly:masterfrom
baincd:group-by-tag-mode-drops-todos-fix

Conversation

@baincd
Copy link
Copy Markdown
Contributor

@baincd baincd commented Mar 9, 2024

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
Copy link
Copy Markdown

LGTM

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

@baincd
Copy link
Copy Markdown
Contributor Author

baincd commented Mar 23, 2024

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 TODOs missing after appying group by tag #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.

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.

2 participants