fix: find tasks in blockquotes and Obsidian callouts

[AnnaKornfeldSimpson]

Description

Now that I am familiar with the dataview codebase, I found their regex for parsing list items, which works with Obsidian callouts, and it turns out the fix is super easy to add to Tasks! Blockquote/callout indicators are now stored along with spaces and tabs in the indentation field of the Task, so they are preserved when the task is rendered out to string.

Motivation and Context

Fixes #656 including nested blockquotes/callouts

How has this been tested?

Added unit tests for parsing the task, parsing tags out of the task, and toString. Something mentioned in the issue thread is testing specifically that the indentation is preserved when a new instance of a recurring task is generated - I could not figure out where in the tests to put that, if you still think it is needed, could you give me a pointer please to where in the tests it should go?

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• Refactor (non-breaking change which only improves the design or structure of existing code, and making no changes to its external behaviour)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

• My code follows the code style of this project and passes yarn run lint.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• My change has adequate Unit Test coverage. (mostly, see note above!)

By creating a Pull Request I agree to the Code of Conduct.

[claremacrae]

Very nice, clear change.

Let me know if you would like to address the issues raised separately. If so, I'll just go ahead and merge this, and bear in mind not to do the next released until exclude sub-items Is updated.

[AnnaKornfeldSimpson]

(This was supposed to point directly at the ToggleDone.ts file changes in the "fix: More regexes needed updating" commit. I don't know how to github, I guess.)
@claremacrae Do these have/need tests? I cannot totally tell what is going on here.

Also any idea what is supposed to happen at line 39 of that file? Because the results in regex101 do not make any sense to me. Is it supposed to match on the content inside the checkbox (because it does, as long as that content is not a space)? If so, the comment in the file does not make a lot of sense.
EDIT: Maybe the comment in the file is wrong? #460 and #449 seem potentially related. I am very confused!
Edit (several hours later): I think the comment at line 37 of ToggleDone is just wrong: for LivePreview and Source Mode, editor.setLine() moves the cursor to the beginning of the line, not the end.
EDIT (again!): No, that is not quite right either. If and only if the cursor is at the end of the line, it will go to the new end of the line after the text is replaced. (This happens if the line becomes shorter, longer, or stays the exact same.) If the cursor is anywhere else in the line, it goes to the beginning of the line instead.
Never mind all that, not related to this PR, will make a separate one.

[AnnaKornfeldSimpson]

Ok I think this is ready for review again. You were very right about looking for other uses of indentation, especially ones that were not tested! There are now more changes.

Outstanding problems I am aware of:

• Pushing the docs fix to some other PR.
• See my comment of confusion about about where/how to test changes in ToggleDone

Please let me know any problems I am not currently aware of, happy to iterate again! Would rather get this right now and not have to stare at the regexes again later.

[claremacrae]

Just as a quick check before merging I tried this code out in my own vault.

With the updated code, I'm not able to get the tasks in block quotes and callouts to be found by Tasks.

What am I doing wrong, @AnnaKornfeldSimpson ?

Code changes

First of all, here is the evidence that I have got the updated code in my vault:

Also, I've restarted Obsidian.

My Test File

I created the attached test file... delete.me.md

delete me.md

Here is the content...

Note the paths to the file in my vault in 2 locations. They will need to be updated before anyone else uses this file.

---
creation date: 2022-08-03 16:22
tags: 
aliases: []
---

# delete me

%% >>> Begin Template Block '_templates/Tasks Templates/L - Tasks in This File.md' %%

## All Tasks in This File

> [!tasks-in-this-file] All tasks in this file
> ```tasks
> not done
> path includes Obsidian/tasks/tasks user support/03 Done - tasks user support/1.11.1 Release/delete me.md
> hide backlink
> sort by due date
> sort by description
> group by heading
> # hide task count
> ```

## Non-sub-tasks in This File

> [!tasks-in-this-file] non sub-tasks in this file
> ```tasks
> not done
> path includes Obsidian/tasks/tasks user support/03 Done - tasks user support/1.11.1 Release/delete me.md
> hide backlink
> sort by due date
> sort by description
> group by heading
> exclude sub-items
> # hide task count
> ```

---

## The Tasks


%% <<< End Template Block '_templates/Tasks Templates/L - Tasks in This File.md' %%

> - [ ] x1
>     - [ ] x1.1
> - [ ] x2
>     - [ ] x2.1
> - [ ] x3
>     - [ ] x3.1

---

> [!NOTE]
> - [ ] y1
>     - [ ] y1.1
> - [ ] y2
>     - [ ] y2.1
> - [ ] y3
>     - [ ] y3.1

- [ ] z1
    - [ ] z1.1
- [ ] z2
    - [ ] z2.1
- [ ] z3
    - [ ] z3.1

What I see

The x* tasks (in a block quote) and the y* tasks (in an Obsidian callout) are not matched in either query.

[claremacrae]

I removed the callouts from the 2 tasks blocks, and they still didn't find the tasks that are in callouts and block quotes.

[AnnaKornfeldSimpson]

Oh bother! I will investigate within the next couple of hours. Thank you for smoke testing this. I should have realized that it needed smoke testing even though there were automated tests.

[AnnaKornfeldSimpson]

Working on this now.

I removed the callouts from the 2 tasks blocks, and they still didn't find the tasks that are in callouts and block quotes.

I am pretty confident that I did not touch anything related to task query blocks. If behavior had changed when moving those I would have been extremely confused.

[AnnaKornfeldSimpson]

I may be running into issues here with not knowing the codebase well enough.

Changed the last test in Query.test.ts (arbitrarily, was searching for a test that called "applyQueryToTasks") to have the input be inside blockquote and that passed just fine... (commit with that change should hopefully show up below this comment, but I rebased to pull in the changes from upstream/main so maybe I confused things?)

If you have any intuition or pointers about what functions to check for the query flow, please do share! Even wrong guesses are going to be more helpful than me trying to read the entire codebase.

[claremacrae]

Firstly, can you reproduce the behaviour that I reported, from a build of the plugin?

I just had a look at the code for ideas...

Cache.ts is what actually reads a markdown file and extracts the tasks, and stores them. So if for some reason something in that code did not find callouts and similar, it could be there. That code does not yet have any tests.

Cache.ts calls Task.fromLine() so I just looked at its code. Are there any tests of calling Task.fromLine() with > prefix?
These 2 lines look a bit suspicious:

obsidian-tasks/src/Task.ts

Lines 207 to 208 in d1896d5

	// match[3] includes the whole body of the task after the brackets.
	const body = regexMatch[3].trim();

In haste... Clare

[claremacrae]

I'm suspecting that if the tests of Task and Query are all fine, then the significant difference between the code in the tests and the plugin as a whole is that latter's use of Cache.

[AnnaKornfeldSimpson]

These 2 lines look a bit suspicious:

obsidian-tasks/src/Task.ts

Lines 207 to 208 in d1896d5

	// match[3] includes the whole body of the task after the brackets.
	const body = regexMatch[3].trim();

The indentation is in regexMatch[1] at that point.

Great, I was having trouble understanding Cache.ts earlier, will look again! After testing that I can repro, good idea.

[claremacrae]

Unrelated - and for later: whilst I think of it... I noticed that there are very few tests of reading tasks beginning with * instead of - - once the feature is working as intended, it might be worth adding maybe one test that checks it does read tasks after asterisks correctly.

[claremacrae]

OK good luck - I really, really need sleep now...

[AnnaKornfeldSimpson]

Found the bug, and it was in Cache.ts - thank you so much for the pointer! Obsidian's sectionCache labels the entirety of a callout as a section of type "callout" while Tasks expects all tasks to be in sections of type "list". I will think about how to fix this.

Are there ways we can test the processing code in Cache.ts to make sure there is not a regression in the future?
Edit: there is some duplciation in processing code betwteen Cache.ts and File.ts (though File.ts does not look at the sectionCache and so does not have this bug.)

[AnnaKornfeldSimpson]

Still chasing issues, but now they are in LivePreview only. Not-a-tasks-issue but fun-fact: Obsidian renders the contents of callouts in Live Preview as long as the cursor is somewhere else, and does the same for checkboxes on normal lines of Markdown but it does not do the same for checkboxes inside blockquotes! Surprising!

[AnnaKornfeldSimpson]

I resolved the part of this that seemed feasibly resolvable. @claremacrae new stuff to code review, but I did smoke test it thoroughly this time.

• Fixed Cache.ts to properly find the section of tasks inside callouts or blockquotes.
• While I was in Cache.ts I also fixed up precedingHeader to use the right Obsidian cache, which might lead to some tiny perf improvements and resulted in clearer code.
• With that change, all tasks in callouts / blockquotes, including nested ones are now found in task queries, and correctly preserve indentation when toggled in Reading View or via "Toggle Task Done" command or from within a task query block in Live Preview.
• Obsidian's rendering choices for callouts in Live Preview make it impossible to handle toggling tasks written in callouts by clicking their checkbox. The entire callout's contents are treated as being at the same "position" within the document, so cannot find the right line within the click handler. The only true fix I could think of would be to set some kind of flag and then do the actual toggle when the metadataCache "changed" event comes through a couple seconds later, but that seemed way too complicated for this PR. Instead, I added a console warning and a pop-up Obsidian "Notice" in the LivePreviewExtension if it detects that the checkbox is inside a callout (and not a task query block within the callout, since those are handled independently) to redirect people to the "Toggle Task Done" command or checking the box in Reading View. Smoke tested that. Let me know if I can word the warning message better!

Outstanding things I will get to tomorrow (my time):

• Actually putting a proper test in Query somewhere to make sure tasks in blockquotes/callouts are picked up by queries. (Reverted the one I randomly modified earlier.)
• Adding a test or two for tasks that use * instead of - in the same places I added tests for blockquotes/callouts, per your excellent suggestion above.
• Better comments inside LivePreviewExtension about what the issue actually is.

Things that I think need tests added, but I do not know where to add them, any suggestions @claremacrae or would you rather add them yourself in a different PR?

• Test to prevent a regression of this bug in Cache.ts
• Tests for the regexes in ToggleDone

Other unresolved:

• Is there an existing Issue or Discussion for documentation that I should add a note to about documenting all this (see comment above)?

[claremacrae]

Found the bug, and it was in Cache.ts - thank you so much for the pointer! Obsidian's sectionCache labels the entirety of a callout as a section of type "callout" while Tasks expects all tasks to be in sections of type "list". I will think about how to fix this.

Excellent.

Are there ways we can test the processing code in Cache.ts to make sure there is not a regression in the future? Edit: there is some duplciation in processing code betwteen Cache.ts and File.ts (though File.ts does not look at the sectionCache and so does not have this bug.)

When I did my talk on testing plugin code, I was unable to find a way to call any code that used Obsidian code in tests...

It's possible others have found a way to do so since then, but I am unaware.

[AnnaKornfeldSimpson]

When I did my talk on testing plugin code, I was unable to find a way to call any code that used Obsidian code in tests...
It's possible others have found a way to do so since then, but I am unaware

I think the Obsidian Linter devs were working on this recently. I will ask!

[claremacrae]

I resolved the part of this that seemed feasibly resolvable. @claremacrae new stuff to code review, but I did smoke test it thoroughly this time.

Excellent. Thank you.

I would prefer to wait for the review until all changes are made.

• Obsidian's rendering choices for callouts in Live Preview make it impossible to handle toggling tasks written in callouts by clicking their checkbox. The entire callout's contents are treated as being at the same "position" within the document, so cannot find the right line within the click handler. The only true fix I could think of would be to set some kind of flag and then do the actual toggle when the metadataCache "changed" event comes through a couple seconds later, but that seemed way too complicated for this PR. Instead, I added a console warning and a pop-up Obsidian "Notice" in the LivePreviewExtension if it detects that the checkbox is inside a callout (and not a task query block within the callout, since those are handled independently) to redirect people to the "Toggle Task Done" command or checking the box in Reading View. Smoke tested that. Let me know if I can word the warning message better!

I think that showing a popup is a great idea.

When testing this, I will see if it fixes #688. If it does not, this will at least give enough pointers to enable the pop-up and warning to be extended to also work for #688.

Outstanding things I will get to tomorrow (my time):

• Actually putting a proper test in Query somewhere to make sure tasks in blockquotes/callouts are picked up by queries. (Reverted the one I randomly modified earlier.)

I'm not sure that you need to do that. You have tested that the parsing is fine, so the tasks will be recognised by Query, as all it does is loop over the recognised tasks.

By all means add perhaps 1 example - but it does not need to repeat testing done at the Task level.

• Adding a test or two for tasks that use * instead of - in the same places I added tests for blockquotes/callouts, per your excellent suggestion above.

Thank you. Feel free to modify a sprinkling of existing tests to try with *.

• Better comments inside LivePreviewExtension about what the issue actually is.

Thanks.

Things that I think need tests added, but I do not know where to add them, any suggestions @claremacrae or would you rather add them yourself in a different PR?

• Test to prevent a regression of this bug in Cache.ts

I do not think that is going to possible in the short term.

As an alternative, maybe slightly modify the file and text for the smoke test, to include a task in block quote and/or callout, so accidental regression would be spotted when running the smoke tests?

• Tests for the regexes in ToggleDone

I'm not sure what that is about. Are they not already covered by the toggle tests in Task.test.ts?

Other unresolved:

• Is there an existing Issue or Discussion for documentation that I should add a note to about documenting all this (see comment above)?

Please add any comments for this PR to the page at: Getting Started - Obsidian Tasks. Thank you.

[AnnaKornfeldSimpson]

Argh, I did git wrong again, I guess. Only the changes starting with docs: Document this feature and LP caveat are actually new.

---

When testing this, I will see if it fixes #688. If it does not, this will at least give enough pointers to enable the pop-up and warning to be extended to also work for #688.

Ah nope, I think this may help explain #688 (all rendered widget content are considered to be at the same line number) but it does not fix it. I made my selector searches as narrow as possible. Someone will have to extend.

---

Not a lot of luck on the mocking of Obsidian code, so I agree that changing the Smoke Test instructions is the best way to go here. Docs change complete, test and comment changes incoming.

[AnnaKornfeldSimpson]

Outstanding things I will get to tomorrow (my time):

• Actually putting a proper test in Query somewhere to make sure tasks in blockquotes/callouts are picked up by queries. (Reverted the one I randomly modified earlier.)

I'm not sure that you need to do that. You have tested that the parsing is fine, so the tasks will be recognised by Query, as all it does is loop over the recognised tasks.

By all means add perhaps 1 example - but it does not need to repeat testing done at the Task level.

You are right, since Query already worked once the Tasks tests passed, going to skip.

Thank you. Feel free to modify a sprinkling of existing tests to try with *.

Done.

• Better comments inside LivePreviewExtension about what the issue actually is.

Thanks.

Done.

Things that I think need tests added, but I do not know where to add them, any suggestions @claremacrae or would you rather add them yourself in a different PR?

• Test to prevent a regression of this bug in Cache.ts

I do not think that is going to possible in the short term.

As an alternative, maybe slightly modify the file and text for the smoke test, to include a task in block quote and/or callout, so accidental regression would be spotted when running the smoke tests?

Much better idea! Done.

• Tests for the regexes in ToggleDone

I'm not sure what that is about. Are they not already covered by the toggle tests in Task.test.ts?

No, because "ToggleDone" handles adding a [ ] if there is only a - on a line and checking a checkbox even if it is not a task due to lack of global filter. However I noticed several outstanding issues related to this file and it is hard to test some of it due to relying on the Obsidian API so I am going to defer this to a different PR that tries to fix the other issues.

Please add any comments for this PR to the page at: Getting Started - Obsidian Tasks. Thank you.

Done.

---

That is now everything resolved that I have been keeping track of! Happy to fix anything you find, of course.

[claremacrae]

Just a few minor questions and comments - it's very, very nearly there!

[AnnaKornfeldSimpson]

Once again I seem to have borked the git history, sorry! First new commit since your last review is "fix: Lengthen Notice in LP for callouts from 3s to 30s"

[AnnaKornfeldSimpson]

@claremacrae I will finish this "tomorrow" your time but I really could use some suggestions on the wording for the Notice in LP! You are correct about the behavior and I have no idea how to word that succinctly. The Notice is already pretty large...

[claremacrae]

Once again I seem to have borked the git history, sorry! First new commit since your last review is "fix: Lengthen Notice in LP for callouts from 3s to 30s"

Maybe after this PR is done we can try to figure out what the problem is. I'd love to try and get to the bottom of it.

[claremacrae]

@claremacrae I will finish this "tomorrow" your time but I really could use some suggestions on the wording for the Notice in LP! You are correct about the behavior and I have no idea how to word that succinctly. The Notice is already pretty large...

I am not sure of the wording, but here is an attempt at a simpler way of wording this.

---

Background

The Tasks plugin installation notes say:

4. Replace the “Toggle checklist status” hotkey with “Tasks: Toggle Done”.

Here is what I see in my command palette: - which refers to checkbox status...

---

The Observable Behaviour

I think that what is being said is:

In Live Preview, if you have any tasks in Callout blocks, when you click on the task checkboxes, this currently does not trigger “Tasks: Toggle Done”, because Obsidian does not provide valid line numbers in this particular case. So instead, just the basic Obsidian default action of “Toggle checkbox status” happens instead.

What this means is that, in this situation, the task's completion date won't get added, and any recurrence/repeat instruction is ignored.

The docs could show raw markdown examples of both these cases...

Possible idea for this error message

Slightly evading the question on wording at the moment - but is it possible for the popup banner thing to have a hyperlink in it? If so, could it link to a section in the user manual with chapter and verse on the problem?

Perhaps I could start an FAQ file, and add:

Q Why can't I check off tasks in callouts and block quotes, when in Live Preview mode?

[claremacrae]

@AnnaKornfeldSimpson I am starting to think that this is already such a major improvement that it is good enough to release... and we could improve the wording of the banner over time, if there any many people asking questions about it.

I'm just thinking that you have spent SOOO much time on this, and it is definitely a huge improvement....

Feel free to declare it done at any time!

edit: ... after the sections change has been reverted, probably - sorry!

[AnnaKornfeldSimpson]

Slightly evading the question on wording at the moment - but is it possible for the popup banner thing to have a hyperlink in it? If so, could it link to a section in the user manual with chapter and verse on the problem?

Since clicking on the popup closes it, I am not sure. It would be easy to add a link in the console warning, which can also be longer, but I do not think users on mobile OSes have any way to access it.

[AnnaKornfeldSimpson]

@claremacrae I reverted the unrelated Cache change, did my best at fixing up the wording in the LP Notice, and I am going to declare this done now! Please feel free to assign me a docs issue if you want me to add something further to the docs on this.

[claremacrae]

Excellent!

[claremacrae]

@claremacrae I reverted the unrelated Cache change, did my best at fixing up the wording in the LP Notice, and I am going to declare this done now! Please feel free to assign me a docs issue if you want me to add something further to the docs on this.

I really, really like the new wording in the LP notice - thank you!

[claremacrae]

I wonder whether the git problems were due to rebasing?

In my experience, there's very rarely any need to rebase a PR during development or review. The rare times when there are conflicts it might be worth it, but even they can often be fixed on merge.

So the workaround would be to not rename, and then not have to force-push.

(Also, sometimes when reviewing a PR I will create a branch off of the contributor's fork, with changes I would like to make as a separate follow-up PR. If anyone force-pushes, I have to remember to save those edits and re-apply them later)

[claremacrae]

Fantastic stuff - huge thanks @AnnaKornfeldSimpson! ❤️ 😄 🎉