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

Group by tag mode drops todos fix #832

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

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

LGTM

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

@baincd
Copy link
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