Skip to content

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

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.

Sign up for GitHub

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

Draft
axelson wants to merge 9 commits into
base: main
Choose a base branch
from
Draft

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
74fe90a
Toggle task done command adds global filter text
axelson Jun 3, 2023
86b4372
Add more tests
axelson Jun 4, 2023
6d116e6
Add regex testing
axelson Jun 4, 2023
750df8d
Add and respect autoInsertGlobalFilter setting
axelson Jun 4, 2023
05cca91
divide ToggleDone should add checkbox tests
axelson Jun 25, 2023
5ef7a10
Test when the global filter is at the end of the task
axelson Jun 25, 2023
31292fc
break up the should complete a task test
axelson Jun 25, 2023
ded74e8
Merge remote-tracking branch 'upstream/main' into insert-global-filter
axelson Jun 25, 2023
a523a0d
Clean up implementation
axelson Jun 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file modified docs/images/settings-global-filter.png
View file
Open in desktop
Copy link
Copy Markdown
Member

@claremacrae claremacrae Jun 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for updating the screenshot.

Things I noticed in it:

  • It would be nice for the description to end with a . for consistency with the others.
  • It might read better if it said 'Enabling this causes the ....'

Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
6 changes: 5 additions & 1 deletion src/Commands/ToggleDone.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { StatusRegistry } from '../StatusRegistry';

import { Task, TaskRegularExpressions } from '../Task';
import { TaskLocation } from '../TaskLocation';
import { GlobalFilter } from '../Config/GlobalFilter';

export const toggleDone = (checking: boolean, editor: Editor, view: MarkdownView | MarkdownFileInfo) => {
if (checking) {
Expand Down Expand Up @@ -91,7 +92,10 @@ export const toggleLine = (line: string, path: string): EditorInsertion => {
return { text: line.replace(TaskRegularExpressions.taskRegex, `$1- [${newStatusString}] $4`) };
} else if (TaskRegularExpressions.listItemRegex.test(line)) {
// Convert the list item to a checklist item.
const text = line.replace(TaskRegularExpressions.listItemRegex, '$1$2 [ ]');
const globalFilter = GlobalFilter.get();
const newTaskText = GlobalFilter.shouldPrependToNewTask(line) ? `[ ] ${globalFilter}` : '[ ]';

const text = line.replace(TaskRegularExpressions.listItemRegex, `$1$2 ${newTaskText}`);
Copy link
Copy Markdown
Member

@claremacrae claremacrae Jun 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the methods in GlobalFilter, there has been a lot of effort to encapsulate not just data but actual behaviour... So methods like removeAsSubstringFromDependingOnSettings() can be called without the caller needing to understand what's going on.

Line 96 here is a curious mixture in that it asks GlobalFilter whether the global filter should do the prepending, and then it combines them together...

So it isn't really encapsulating at all...

I suggest seeing if you can move the global-filter bits of the code in to a new method in GlobalFilter.

So that the code might end up looking something like:

        } else if (TaskRegularExpressions.listItemRegex.test(line)) {
            // Convert the list item to a checklist item.
            const newTaskText = GlobalFilter.someMethodThatDescribeTheBehaviour(line);
            const text = line.replace(TaskRegularExpressions.listItemRegex, `$1$2 ${newTaskText}`);
            return { text, moveTo: { ch: text.length } };
        } else {

I note that there is a method GlobalFilter.prependTo(description) that might be useful here.

return { text, moveTo: { ch: text.length } };
} else {
// Convert the line to a list item.
Expand Down
6 changes: 6 additions & 0 deletions src/Config/GlobalFilter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,12 @@ export class GlobalFilter {
return GlobalFilter.get() + ' ' + description;
}

static shouldPrependToNewTask(line: string): boolean {
const { autoInsertGlobalFilter } = getSettings();

return !GlobalFilter.isEmpty() && autoInsertGlobalFilter && !GlobalFilter.includedIn(line);
Copy link
Copy Markdown
Member

@claremacrae claremacrae Jun 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering !GlobalFilter.includedIn(line) is a potential source of bugs, I think.

Firstly, the code GlobalFilter.includedIn is:

    static includedIn(description: string): boolean {
        const globalFilter = GlobalFilter.get();
        return description.includes(globalFilter);
    }

Notice how it says it's taking a description, but the new code is passing in the whole line.

Suppose that someone has used * as their global filter.

If they had a line that looked like this...

- Hello

and ran the Toggle command, they would get this, because '*' was not in the initial line:

- [ ] * Hello

But if their initial line looked like this:

* Hello

The running the toggle line would give them this - notice how the global filter hasn't been added:

* [ ] Hello

}

/**
* Search for the global filter for the purpose of removing it from the description, but do so only
* if it is a separate word (preceding the beginning of line or a space and followed by the end of line
Expand Down
2 changes: 2 additions & 0 deletions src/Config/Settings.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ export type TASK_FORMATS = typeof TASK_FORMATS; // For convenience to make some
export interface Settings {
globalQuery: string;
globalFilter: string;
autoInsertGlobalFilter: boolean;
removeGlobalFilter: boolean;
taskFormat: keyof TASK_FORMATS;
setCreatedDate: boolean;
Expand Down Expand Up @@ -82,6 +83,7 @@ export interface Settings {
const defaultSettings: Settings = {
globalQuery: '',
globalFilter: GlobalFilter.empty,
autoInsertGlobalFilter: false,
removeGlobalFilter: false,
taskFormat: 'tasksPluginEmoji',
setCreatedDate: false,
Expand Down
17 changes: 17 additions & 0 deletions src/Config/SettingsTab.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,23 @@ export class SettingsTab extends PluginSettingTab {
});
});

new Setting(containerEl)
.setName('"Tasks: Toggle task done" command inserts global task filter')
.setDesc(
SettingsTab.createFragmentWithHTML(
'Enabling this causes "Tasks: Toggle task done" command to insert the global task filter when creating a new checkbox',
),
)
.addToggle((toggle) => {
const settings = getSettings();

toggle.setValue(settings.autoInsertGlobalFilter).onChange(async (value) => {
updateSettings({ autoInsertGlobalFilter: value });

await this.plugin.saveSettings();
});
});

new Setting(containerEl)
.setName('Remove global filter from description')
.setDesc(
Expand Down
9 changes: 8 additions & 1 deletion src/TaskSerializer/DefaultTaskSerializer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,14 @@ export class DefaultTaskSerializer implements TaskSerializer {
// components but now we want them back.
// The goal is for a task of them form 'Do something #tag1 (due) tomorrow #tag2 (start) today'
// to actually have the description 'Do something #tag1 #tag2'
if (trailingTags.length > 0) line += ' ' + trailingTags;
if (trailingTags.length > 0) {
// If the line is empty besides the tag then don't prepend a space because it results in a double space
if (line === '') {
line += trailingTags;
} else {
line += ' ' + trailingTags;
}
}
Comment on lines +296 to +303
Copy link
Copy Markdown
Member

@claremacrae claremacrae Jun 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Various thoughts here...

What's the reason for this change in this PR? It seems unrelated.
What's the harm in a double-space? Presumably this was pre-existing behaviour?

Also, this is the code for the Tasks Emoji format.

So what happens when the user is using Dataview task format? Will it produce a double-space because it doesn't have this logic?

And what will happen if several more formats are added? Will they all need to have this check?


return {
description: line,
Expand Down
96 changes: 94 additions & 2 deletions tests/Commands/ToggleDone.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { GlobalFilter } from '../../src/Config/GlobalFilter';
import { StatusRegistry } from '../../src/StatusRegistry';
import { Status } from '../../src/Status';
import { StatusConfiguration } from '../../src/StatusConfiguration';
import { updateSettings } from '../../src/Config/Settings';

window.moment = moment;

Expand Down Expand Up @@ -79,6 +80,7 @@ function testToggleLineForOutOfRangeCursorPositions(
describe('ToggleDone', () => {
afterEach(() => {
GlobalFilter.reset();
updateSettings({ autoInsertGlobalFilter: false });
});

const todaySpy = jest.spyOn(Date, 'now').mockReturnValue(moment('2022-09-04').valueOf());
Expand All @@ -100,31 +102,121 @@ describe('ToggleDone', () => {
});

it('should add checkbox to hyphen and space', () => {
GlobalFilter.set('');
updateSettings({ autoInsertGlobalFilter: false });

testToggleLine('|- ', '- [ ] |');
testToggleLine('- |', '- [ ] |');
testToggleLine('- |foobar', '- [ ] foobar|');

updateSettings({ autoInsertGlobalFilter: false });

testToggleLine('|- ', '- [ ] |');
testToggleLine('- |', '- [ ] |');
testToggleLine('- |foobar', '- [ ] foobar|');

GlobalFilter.set('#task');
updateSettings({ autoInsertGlobalFilter: false });

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

GlobalFilter.set('#task');
updateSettings({ autoInsertGlobalFilter: true });

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

GlobalFilter.set('TODO');
updateSettings({ autoInsertGlobalFilter: false });

testToggleLine('|- ', '- [ ] |');
testToggleLine('- |', '- [ ] |');
testToggleLine('- |foobar', '- [ ] foobar|');
testToggleLine('- |TODO foobar', '- [ ] TODO foobar|');

GlobalFilter.set('TODO');
updateSettings({ autoInsertGlobalFilter: true });

testToggleLine('|- ', '- [ ] TODO |');
testToggleLine('- |', '- [ ] TODO |');
testToggleLine('- |foobar', '- [ ] TODO foobar|');
testToggleLine('- |TODO foobar', '- [ ] TODO foobar|');

// Test a global filter that has special characters from regular expressions
GlobalFilter.set('a.*b');
updateSettings({ autoInsertGlobalFilter: false });

testToggleLine('|- [ ] a.*b ', '|- [x] a.*b ✅ 2022-09-04');
testToggleLine('- [ ] a.*b foobar |', '- [x] a.*b foobar |✅ 2022-09-04');

GlobalFilter.set('a.*b');
updateSettings({ autoInsertGlobalFilter: true });

testToggleLine('|- [ ] a.*b ', '|- [x] a.*b ✅ 2022-09-04');
testToggleLine('- [ ] a.*b foobar |', '- [x] a.*b foobar |✅ 2022-09-04');
});

it('should complete a task', () => {
testToggleLine('|- [ ] ', '|- [x] ✅ 2022-09-04');
testToggleLine('- [ ] |', '- [x] | ✅ 2022-09-04');
testToggleLine('- [ ]| ', '- [x]| ✅ 2022-09-04');

// Issue #449 - cursor jumped 13 characters to the right on completion
testToggleLine('- [ ] I have a |proper description', '- [x] I have a |proper description ✅ 2022-09-04');

GlobalFilter.set('#task');

testToggleLine('|- [ ] ', '|- [x] ');
testToggleLine('- [ ] |', '- [x] |');
const completesWithTaskGlobalFilter = () => {
testToggleLine('|- [ ] ', '|- [x] ');
testToggleLine('- [ ] |', '- [x] |');

testToggleLine('|- [ ] #task ', '|- [x] #task ✅ 2022-09-04');
testToggleLine('- [ ] #task foobar |', '- [x] #task foobar |✅ 2022-09-04');
};

updateSettings({ autoInsertGlobalFilter: true });
completesWithTaskGlobalFilter();
updateSettings({ autoInsertGlobalFilter: false });
completesWithTaskGlobalFilter();

// Issue #449 - cursor jumped 13 characters to the right on completion
testToggleLine('- [ ] I have a |proper description', '- [x] I have a |proper description');

GlobalFilter.set('TODO');

const completesWithTodoGlobalFilter = () => {
testToggleLine('|- [ ] ', '|- [x] ');
testToggleLine('- [ ] |', '- [x] |');

testToggleLine('|- [ ] TODO ', '|- [x] TODO ✅ 2022-09-04');
testToggleLine('- [ ] TODO foobar |', '- [x] TODO foobar |✅ 2022-09-04');
};

updateSettings({ autoInsertGlobalFilter: true });
completesWithTodoGlobalFilter();
updateSettings({ autoInsertGlobalFilter: false });
completesWithTodoGlobalFilter();

// Test a global filter that has special characters from regular expressions
GlobalFilter.set('a.*b');

const completesWithRegexGlobalFilter = () => {
testToggleLine('|- [ ] ', '|- [x] ');
testToggleLine('- [ ] |', '- [x] |');

testToggleLine('|- [ ] a.*b ', '|- [x] a.*b ✅ 2022-09-04');
testToggleLine('- [ ] a.*b foobar |', '- [x] a.*b foobar |✅ 2022-09-04');
};

updateSettings({ autoInsertGlobalFilter: true });
completesWithRegexGlobalFilter();
updateSettings({ autoInsertGlobalFilter: false });
completesWithRegexGlobalFilter();
});

it('should un-complete a completed task', () => {
Expand Down
2 changes: 1 addition & 1 deletion tests/TaskSerializer/DataviewTaskSerializer.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ describe('DataviewTaskSerializer', () => {
});

it('should parse tags', () => {
const description = ' #hello #world #task';
const description = '#hello #world #task';
const taskDetails = deserialize(description);
expect(taskDetails).toMatchTaskDetails({ tags: ['#hello', '#world', '#task'], description });
});
Expand Down
2 changes: 1 addition & 1 deletion tests/TaskSerializer/DefaultTaskSerializer.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ describe.each(symbolMap)("DefaultTaskSerializer with '$taskFormat' symbols", ({
});

it('should parse tags', () => {
const description = ' #hello #world #task';
const description = '#hello #world #task';
const taskDetails = deserialize(description);
expect(taskDetails).toMatchTaskDetails({ tags: ['#hello', '#world', '#task'], description });
});
Expand Down