feat: Toggle task done command adds global filter text, if enabled #2015

axelson · 2023-06-03T21:02:06Z

New behavior: if the user has a global filter set then the global filter is inserted when the "Tasks: Toggle task done" is run.

Description

The "Tasks: Toggle task done" command will now prepend the globalFilter when creating a new checkbox if the autoInsertGlobalFilter setting is enabled. The autoInsertGlobalFilter setting defaults to false.

Also fix an edge case where an extra empty space was added when the task doesn't have any description besides the global filter. Otherwise some of test cases felt broken because an extra space would be inserted.

Previously '- |#task' would become '- [ ] #task|' (notice the two spaces before #task)
Now it becomes '- [ ] #task|'

TODO:

Update this description to reflect all the changes (e.g. the new extractLineItemComponents)

Motivation and Context

Fixes #1320

I like to use the "Tasks: Toggle task done" command to create tasks, but since I have a global filter set I had to manually type the global filter each time. This PR changes the behavior so that if the user has a global filter set then the global filter is inserted when the "Tasks: Toggle task done" is run.

I'm not sure if this requires an update to the documentation. I didn't see anywhere where the behavior of "Tasks: Toggle task done" is explained. However this is a change in behavior that merits a notice in the release notes.

I'm not sure if this should be considered a breaking change.

Proposed release notes text

"Tasks: Toggle task done" now has the ability to prepend the global filter to tasks created with the "Tasks: Toggle task done" command if the "Tasks: Toggle task done" command inserts global task filter setting is enabled.

How has this been tested?

Tested in a test vault and updated the unit tests.

Screenshots (if appropriate)

Types of changes

Changes visible to users:

Bug fix (prefix: fix - non-breaking change which fixes an issue)
New feature (prefix: feat - non-breaking change which adds functionality)
Breaking change (prefix: feat!! or fix!! - fix or feature that would cause existing functionality to not work as expected)
Documentation (prefix: docs - improvements to any documentation content for users)
Sample vault (prefix: vault - improvements to the Tasks-Demo sample vault)
Contributing Guidelines (prefix: contrib - any improvements to documentation content for contributors - see Contributing to Tasks)

Internal changes:

Refactor (prefix: refactor - non-breaking change which only improves the design or structure of existing code, and making no changes to its external behaviour)
Tests (prefix: test - additions and improvements to unit tests and the smoke tests)
Infrastructure (prefix: chore - examples include GitHub Actions, issue templates)

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.

Terms

My contribution follow this project's contributing guide
I agree to follow this project's Code of Conduct

Fixes 1320 I like to use the "Tasks: Toggle task done" command to create tasks, but since I have a global filter set I had to manually type the global filter each time. This PR changes the behavior so that if the user has a global filter set then the global filter is inserted when the "Tasks: Toggle task done" is run.

claremacrae · 2023-06-03T21:36:01Z

Hi, thanks very much indeed for adding this.

I don't personally use a global filter, but I imagine that those who do will find it useful.

claremacrae · 2023-06-03T21:39:56Z

tests/Commands/ToggleDone.test.ts

+        testToggleLine('|- ', '- [ ] #task |');
+        testToggleLine('- |', '- [ ] #task |');
+        testToggleLine('- |foobar', '- [ ] #task foobar|');
    });


It would be good to see some more thorough testing here.

Such as, what happens when there is already #task present on the line. To ensure that it doesn't add a duplicate tag.

Also, can we test the behaviour with non-tag global filter please?

Also, what happens if the global filter contains characters which are special characters in regular expressions?
(There is some code somewhere to deal with that - like, escaping regex special characters in arbitrary strings).

That's all I can think of for now!

Also it would be good to have tests for the case where there is no global filter defined, for example to guard against accidental insertion of a space at the beginning of the updated lines.

Okay I've added more testing in 86b4372. Does that seem sufficient to you? Are there any other tests that are missing?

Some of those tests revealed a smaller bug which is also fixed in that commit. If the task description was empty then toggling the task done would insert an extra space.

- |#task would become - [ ] #task| instead of - [ ] #task|

I also added handling for when the description already includes the global filter.

Also, what happens if the global filter contains characters which are special characters in regular expressions?
I added some tests for this in 6d116e6

claremacrae · 2023-06-03T22:07:08Z

I'm not sure if this requires an update to the documentation. I didn't see anywhere where the behavior of "Tasks: Toggle task done" is explained.

Thanks for considering that. I searched the docs for 'command' and only found tangential references, so it's not something to include in the docs right now.

I've just made a note in #767 to ensure that the capability added here will (eventually) be documented...

However this is a change in behavior that merits a notice in the release notes.

Agreed. As a PR, it will automatically get included in the next release notes.

I'm not sure if this should be considered a breaking change.

Having re-read #1320, I am comfortable labelling this a bug fix, just because without this, the command adds lines which Tasks then ignores if the user has a global filter. That seems just wrong.

Also fix the addition of an empty space when the task doesn't have any description besides the global filter. Otherwise some of test cases felt broken because an extra space would be inserted. '- |#task' would become '- [ ] #task|' instead of '- [ ] #task|' Also fix the case where the task description already includes the global filter. In that case it is not necessary to add the global filter.

Also use the GlobalFilter.includedIn for the check

ilandikov · 2023-06-04T08:31:24Z

I'm a bit confused, in my workflow, I use Tasks to manage tasks with Global Filter and I also have tasks without it. I don't want Tasks to manage those. I use Toggle task cmd to complete both of them and this has been very useful - I have one cmd to complete both types of tasks.

So this kind of breaks it for me, there could be other people with this use-case. Maybe it could be possible to add this feature/bugfix with a setting? (Disabled by default)

claremacrae · 2023-06-04T09:13:37Z

I'm a bit confused, in my workflow, I use Tasks to manage tasks with Global Filter and I also have tasks without it. I don't want Tasks to manage those. I use Toggle task cmd to complete both of them and this has been very useful - I have one cmd to complete both types of tasks.

So this kind of breaks it for me, there could be other people with this use-case. Maybe it could be possible to add this feature/bugfix with a setting? (Disabled by default)

Thank you for saying this @ilandikov.

Overnight I had a nagging doubt about whether this might be the case, prompted by a dim recollection of someone else saying that they only used Tasks for a small subset of their checkboxes, and it was in their muscle memory to add the global filter in the few areas where they wanted Tasks to track.

So yes, I do think that having a setting to turn this PR on will be necessary to avoid disrupting people's muscle memory and workflow.

claremacrae · 2023-06-04T09:18:05Z

src/Commands/ToggleDone.ts

+            const addGlobalFilter = globalFilter != '' && !GlobalFilter.includedIn(line);
+            const newTaskText = addGlobalFilter ? `[ ] ${globalFilter}` : '[ ]';
+            const text = line.replace(TaskRegularExpressions.listItemRegex, `$1$2 ${newTaskText}`);


It just dawned on me that someone recently spent a lot of time moving all code related to the global filter, including manipulating descriptions, in the new GlobalFilter class.

If the above cannot be done with the existing methods in GlobalFilter, I feel that the new code should go in to a new method in that class. The existing names there may give ideas.

Ah that makes sense. I took a stab at moving as much of the GlobalFilter-related logic to the GlobalFilter class as I could 750df8d. I didn't go all the way because I wanted to keep the regex-related logic in ToggleDone.ts. Let me know if this matches what you're thinking! If not then I'm open to suggestions.

Don't automatically insert the global filter when creating a new task, instead only insert the global filter if autoInsertGlobalFilter is true.

axelson · 2023-06-04T19:22:09Z

I'm a bit confused, in my workflow, I use Tasks to manage tasks with Global Filter and I also have tasks without it. I don't want Tasks to manage those. I use Toggle task cmd to complete both of them and this has been very useful - I have one cmd to complete both types of tasks.

So yes, I do think that having a setting to turn this PR on will be necessary to avoid disrupting people's muscle memory and workflow.

Ah I hadn't considered that use case because for me 90% percent of the checkboxes I create I want to be considered as Tasks plugin tasks. In 750df8d I've added a autoInsertGlobalFilter setting that controls this new behavior and defaults to false so there's no change for users unless they opt in to the new behavior.

claremacrae · 2023-06-20T12:03:36Z

Thank you again for offering this contribution, and apologies for not picking this up yet. It's not forgotten and I will get to it.

axelson · 2023-06-23T07:07:14Z

@claremacrae Thank you! And I'll try to fix the merge conflict over the weekend

claremacrae · 2023-06-23T08:07:03Z

@claremacrae Thank you! And I'll try to fix the merge conflict over the weekend

Thank you. That is useful to know - I will try and do the code review before the weekend then, so if there is any feedback you would only need to look at it once.

Create a extractLineItemComponents function GlobalFilter now does the insertion into the body and operates only on description instead of the entire line Tweaked listItemRegex to capture the body explicitly

axelson · 2023-06-25T21:11:30Z

Okay I've pushed up some changes based on pairing as well as a few additional changes (such as extractLineItemComponents), I think the main items still remaining is to update the PR description to reflect all the changes and possibly a refactor that will allow us to reduce the repetition in the tests (i.e. a function tied directly to the auto insert behavior, possibly by testing addGlobalFilterToDescriptionDependingOnSettings directly instead of transitively testing it in ToggleDone).

claremacrae · 2023-06-30T21:29:35Z

Okay I've pushed up some changes based on pairing as well as a few additional changes (such as extractLineItemComponents),

Thanks!

I think the main items still remaining is to update the PR description to reflect all the changes

Am happy to see the PR description updated...

and possibly a refactor that will allow us to reduce the repetition in the tests (i.e. a function tied directly to the auto insert behavior, possibly by testing addGlobalFilterToDescriptionDependingOnSettings directly instead of transitively testing it in ToggleDone).

I have held off saying this as it feels a bit negative, but I basically feel that all the tests added in the PR are now in the wrong place.

The logic of the new behaviour should now be tested in the GlobalFilter tests.

I would pretty-much revert the changes to ToggleDone.test.ts and then add just these 2 tests:

when autoAddGlobalFilter enabled, it only adds global filter at the point of making a list item in to a task
when autoAddGlobalFilter disabled, it never adds the global filter

In other words, the tests in ToggleDone are written in terms of user-visible behaviour.

How do you feel about doing more in the testing?

If the description above is too vague, and if you are available on Sunday, we could pair on it together.

If that won't work, I don't mind editing the tests and pushing to your branch, and then merging.

claremacrae · 2023-11-07T14:12:22Z

Just to note that @ilandikov has added kindly merged in main and responded to the feedback requests here.

I will have time to review that over the next 2 or 3 weeks, and then figure out a way to first merge this one, and then to merge a PR from ilandikov to add the later stages - so that both contributors get credit...

Thanks to both @axelson and @ilandikov for working on this...

I hope that this message prevents the possible risk of two people working on it at the same time...

ilandikov · 2023-11-07T17:23:21Z

FYI: main...ilandikov:obsidian-tasks:insert-global-filter

claremacrae · 2024-05-07T19:33:53Z

Setting to draft status any PRs which are not currently mergable...

claremacrae added the scope: commands Additions and changes to the commands supplied by Tasks label Jun 3, 2023

claremacrae reviewed Jun 3, 2023

View reviewed changes

claremacrae mentioned this pull request Jun 3, 2023

Add documentation for Tasks Commands #767

Open

claremacrae changed the title ~~Toggle task done command adds global filter text~~ fix: Toggle task done command adds global filter text, if any Jun 3, 2023

axelson added 2 commits June 3, 2023 14:57

Add regex testing

6d116e6

Also use the GlobalFilter.includedIn for the check

claremacrae reviewed Jun 4, 2023

View reviewed changes

Add and respect autoInsertGlobalFilter setting

750df8d

Don't automatically insert the global filter when creating a new task, instead only insert the global filter if autoInsertGlobalFilter is true.

axelson changed the title ~~fix: Toggle task done command adds global filter text, if any~~ fix: Toggle task done command adds global filter text, if enabled Jun 4, 2023

axelson requested a review from claremacrae June 4, 2023 19:23

axelson changed the title ~~fix: Toggle task done command adds global filter text, if enabled~~ feat: Toggle task done command adds global filter text, if enabled Jun 4, 2023

axelson added 5 commits June 25, 2023 08:39

divide ToggleDone should add checkbox tests

05cca91

Test when the global filter is at the end of the task

5ef7a10

break up the should complete a task test

31292fc

Merge remote-tracking branch 'upstream/main' into insert-global-filter

ded74e8

Clean up implementation

a523a0d

Create a extractLineItemComponents function GlobalFilter now does the insertion into the body and operates only on description instead of the entire line Tweaked listItemRegex to capture the body explicitly

claremacrae marked this pull request as draft May 7, 2024 19:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Toggle task done command adds global filter text, if enabled #2015

feat: Toggle task done command adds global filter text, if enabled #2015

axelson commented Jun 3, 2023 •

edited

Loading

claremacrae commented Jun 3, 2023

claremacrae Jun 3, 2023

claremacrae Jun 3, 2023

axelson Jun 4, 2023

claremacrae commented Jun 3, 2023

ilandikov commented Jun 4, 2023

claremacrae commented Jun 4, 2023

claremacrae Jun 4, 2023

axelson Jun 4, 2023

axelson commented Jun 4, 2023

claremacrae commented Jun 20, 2023

axelson commented Jun 23, 2023

claremacrae commented Jun 23, 2023

axelson commented Jun 25, 2023

claremacrae commented Jun 30, 2023

claremacrae commented Nov 7, 2023

ilandikov commented Nov 7, 2023

claremacrae commented May 7, 2024

feat: Toggle task done command adds global filter text, if enabled #2015

Are you sure you want to change the base?

feat: Toggle task done command adds global filter text, if enabled #2015

Conversation

axelson commented Jun 3, 2023 • edited Loading

Description

Motivation and Context

Proposed release notes text

How has this been tested?

Screenshots (if appropriate)

Types of changes

Checklist

Terms

claremacrae commented Jun 3, 2023

claremacrae Jun 3, 2023

Choose a reason for hiding this comment

claremacrae Jun 3, 2023

Choose a reason for hiding this comment

axelson Jun 4, 2023

Choose a reason for hiding this comment

claremacrae commented Jun 3, 2023

ilandikov commented Jun 4, 2023

claremacrae commented Jun 4, 2023

claremacrae Jun 4, 2023

Choose a reason for hiding this comment

axelson Jun 4, 2023

Choose a reason for hiding this comment

axelson commented Jun 4, 2023

claremacrae commented Jun 20, 2023

axelson commented Jun 23, 2023

claremacrae commented Jun 23, 2023

axelson commented Jun 25, 2023

claremacrae commented Jun 30, 2023

claremacrae commented Nov 7, 2023

ilandikov commented Nov 7, 2023

claremacrae commented May 7, 2024

axelson commented Jun 3, 2023 •

edited

Loading