Repository: obsidian-tasks-group/obsidian-tasks
Pull request #1712 (2023-03-01 09:49:23)

fix: Simplify cursor repositioning during the 'ToggleDone' command
<!--- Provide a general summary of your changes in the Title above -->

# Description

Partially supercedes  #1703

This PR is one in a collection of PRs with the ultimate goal of providing an _option_ to write tasks using a plaintext version of this plugin's task format.

This PR's goal is to pave the way for implementing that plaintext task format, specifically by:


* fix: _Slightly_ reworking how the cursor is repositioned during the `ToggleDone` command. This to make it be agnostic to the task's format. 

Users should expect the following cursor behavior:

* When toggling a task, the cursor won't change columns. If toggling a task makes it shorter and leaves the cursor out of bounds, it is moved to the end of the line. This essentially was the previous behavior, except in a few edge cases.
* When toggling _an empty line_ or a line with _just_ a list marker (number, bullet, dash), the cursor will jump to the end of the line so that a user can continue typing.

---

> except in a few edge cases.

The specific edge case that are fixed are that:

- If the cursor is at the start of a line, and a hyphen (`-`) and space follows
    - previously after ToggleDone the cursor was placed inside the `[` and `]`
    - now, the cursor is placed at the end of the line
- With a trailing space at the end of a recurring task line
    - previously after ToggleDone the cursor used to move one character to the left for each trailing space.
    - now, the cursor retains its original position

---

## Motivation and Context

When the `ToggleDone` command moves the cursor after inserting text, it regexes for a completion date with a regex specific to the default task format. 

https://github.com/obsidian-tasks-group/obsidian-tasks/blob/8f6b58b38243dbfe5fda9b5933c9bd55c8805494/src/Commands/ToggleDone.ts#L118-L122

Given the goal to introduce new task formats, I wanted to make the cursor repositioning work consistently regardless of which Task Format you were using. The simplest way I found to do that was to change the behavior to what I described in the `#Description` section which _mostly_ matched the existing behavior according to the test suite.

## How has this been tested?

Leverages the existing test suite for Toggle Done.

A small number of those tests were modified to reflect the changed behavior.

## Screenshots (if appropriate)

## Types of changes

<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->

Changes visible to users:

- [x] **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)
- [ ] **Sample vault** (prefix: `vault` - improvements to the [Tasks-Demo sample vault](https://github.com/obsidian-tasks-group/obsidian-tasks/tree/main/resources/sample_vaults/Tasks-Demo))

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

<!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->

- [x] 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](https://publish.obsidian.md/tasks-contributing/Documentation/About+Documentation) accordingly.
- [x] My change has adequate [Unit Test coverage](https://publish.obsidian.md/tasks-contributing/Testing/About+Testing).

## Terms

<!--
By submitting this pull request, you must agree to follow our
[contributing guide](https://publish.obsidian.md/tasks-contributing) and
[Code of Conduct](https://github.com/obsidian-tasks-group/obsidian-tasks/blob/main/CODE_OF_CONDUCT.md).
Put an x in the boxes to confirm you agree.
-->

- [x] My contribution follow this project's [contributing guide](https://publish.obsidian.md/tasks-contributing)
- [x] I agree to follow this project's [Code of Conduct](https://github.com/obsidian-tasks-group/obsidian-tasks/blob/main/CODE_OF_CONDUCT.md)



Patch:
diff --git a/src/Commands/ToggleDone.ts b/src/Commands/ToggleDone.ts
index 831ce335..9644179b 100644
--- a/src/Commands/ToggleDone.ts
+++ b/src/Commands/ToggleDone.ts
@@ -1,4 +1,4 @@
-import { Editor, MarkdownView, View } from 'obsidian';
+import { Editor, type EditorPosition, MarkdownView, View } from 'obsidian';
 import { StatusRegistry } from '../StatusRegistry';
 
 import { Task, TaskRegularExpressions } from '../Task';
@@ -33,29 +33,38 @@ export const toggleDone = (checking: boolean, editor: Editor, view: View) => {
     const lineNumber = origCursorPos.line;
     const line = editor.getLine(lineNumber);
 
-    const toggledLine = toggleLine(line, path);
-    editor.setLine(lineNumber, toggledLine);
+    const insertion = toggleLine(line, path);
+    editor.setLine(lineNumber, insertion.text);
 
     /* Cursor positions are 0-based for both "line" and "ch" offsets.
      * If "ch" offset bigger than the line length, will just continue to next line(s).
      * By default "editor.setLine()" appears to either keep the cursor at the end of the line if it is already there,
      * ...or move it to the beginning if it is anywhere else. Licat explained this on Discord as "sticking" to one side or another.
-     * Previously, Tasks would reset+move-right the cursor if there was any text in the line, including something inside the checkbox,
-     * moving right by (toggledLine.length - line.length). (Supposedly, but it still moves right, just by less, if the toggledLine is shorter than the old).
-     * This missed the need to move right on the blank line to "- " case (issue #460).
-     * This also meant the cursor moved nonsensically if it was before any newly inserted text,
-     * such as a done date at the end of the line, or after the ">" when "> -" changed to "> - [ ]".
      */
-    // Reset the cursor. Use the difference in line lengths and original cursor position to determine behavior
-    editor.setCursor({
-        line: origCursorPos.line,
-        ch: calculateCursorOffset(origCursorPos.ch, line, toggledLine),
-    });
+    editor.setCursor(getNewCursorPosition(origCursorPos, insertion));
 };
 
-export const toggleLine = (line: string, path: string) => {
-    let toggledLine = line;
-
+/**
+ * Represents text to be inserted into the editor
+ *
+ * @property text The text to insert. May span over multiple lines.
+ * @property [moveTo] An {@link EditorPosition} that represents an absolute position within {@link EditorInsertion.text} that is
+ *    recommended to move the cursor to.
+ *
+ * Any combination of subfields (or the whole {@link EditorPosition}) may be omitted.
+ * Missing fields should preserve the corresponding cursor position. That is:
+ *     * A {@link EditorInsertion.moveTo} that is `undefined` directs the caller to keep the cursor where it is.
+ *     * A {@link EditorInsertion.moveTo} that is `{line: 1}` directs the caller of to jump to {@link EditorInsertion.text}'s
+ *       second line but stay in the same column.
+ *
+ * @interface EditorInsertion
+ */
+interface EditorInsertion {
+    text: string;
+    moveTo?: Partial<EditorPosition>;
+}
+
+export const toggleLine = (line: string, path: string): EditorInsertion => {
     const task = Task.fromLine({
         // Why are we using Task.fromLine instead of the Cache here?
         line,
@@ -63,7 +72,8 @@ export const toggleLine = (line: string, path: string) => {
         fallbackDate: null, // We don't need this to toggle it here in the editor.
     });
     if (task !== null) {
-        toggledLine = toggleTask(task);
+        const lines = task.toggle().map((t) => t.toFileLineString());
+        return { text: lines.join('\n'), moveTo: { line: lines.length - 1 } };
     } else {
         // If the task is null this means that we have one of:
         // 1. a regular checklist item
@@ -78,61 +88,44 @@ export const toggleLine = (line: string, path: string) => {
             const statusString = regexMatch[3];
             const status = StatusRegistry.getInstance().bySymbol(statusString);
             const newStatusString = status.nextStatusSymbol;
-            toggledLine = line.replace(TaskRegularExpressions.taskRegex, `$1- [${newStatusString}] $4`);
+            return { text: line.replace(TaskRegularExpressions.taskRegex, `$1- [${newStatusString}] $4`) };
         } else if (TaskRegularExpressions.listItemRegex.test(line)) {
             // Convert the list item to a checklist item.
-            toggledLine = line.replace(TaskRegularExpressions.listItemRegex, '$1$2 [ ]');
+            const text = line.replace(TaskRegularExpressions.listItemRegex, '$1$2 [ ]');
+            return { text, moveTo: { ch: text.length } };
         } else {
             // Convert the line to a list item.
-            toggledLine = line.replace(TaskRegularExpressions.indentationRegex, '$1- ');
+            const text = line.replace(TaskRegularExpressions.indentationRegex, '$1- ');
+            return { text, moveTo: { ch: text.length } };
         }
     }
-
-    return toggledLine;
 };
 
-const toggleTask = (task: Task): string => {
-    // Toggling a recurring task will produce two Tasks
-    const toggledTasks = task.toggle();
-    return toggledTasks.map((task: Task) => task.toFileLineString()).join('\n');
-};
-
-/* Cases (another way):
-0) Line got shorter: done date removed from end of task, cursor should reset or be moved to new end if reset position is too long.
-1) Line stayed the same length: Checking & unchecking textbox that is not a task - cursor should reset.
-2) Line got longer:
-    a) List marker could have been added. Find it in new text: if cursor was at or right of where it was added, move the cursor right.
-    b) Empty checkbox could have been added. If cursor was after the list marker (in old or new), it should move right.
-    c) Done emoji and date could have been added to the end. Cursor should reset if 0, and stay end of line otherwise.
-    d) Recurring task could have been added to the beginning and done emoji and date added to the end. Current behavior adds so much to the offset to make this right.
-
-So cursor should be reset if 0, which includes being moved to new end if got shorter. Then might need to move right 2 or 3.
-*/
-export const calculateCursorOffset = (origCursorCh: number, line: string, toggledLine: string) => {
-    let newLineLen = toggledLine.length;
-    if (newLineLen <= line.length) {
-        // Line got shorter or stayed same length. Reset cursor to original position, capped at end of line.
-        return origCursorCh >= toggledLine.length ? newLineLen : origCursorCh;
-    }
-
-    // Special-case for done-date append, fixes #449
-    const doneDateLength = ' ✅ YYYY-MM-DD'.length;
-    if (toggledLine.match(TaskRegularExpressions.doneDateRegex) && newLineLen - line.length >= doneDateLength) {
-        newLineLen -= doneDateLength;
-    }
-
-    // Handle recurring tasks: entire line plus newline prepended. Fix for #449 above means appended done date treated correctly.
-    if (newLineLen >= 2 * line.length && toggledLine.search('.+\n.+') !== -1) {
-        return origCursorCh + newLineLen - line.length;
-    }
-
-    /* Line got longer, not a recurring task. Were the added characters before or after the cursor?
-     * At this point the line is at least a list item. Find the first list marker. */
-    const firstListItemChar = toggledLine.search(/[-*]/);
-    if (origCursorCh < firstListItemChar) {
-        // Cursor was in indentation. Reset to where it was.
-        return origCursorCh;
-    }
-
-    return origCursorCh + newLineLen - line.length;
+/**
+ * Computes the new absolute position of the cursor so that it is positioned within the inserted text as specified
+ * by {@link insertion}.moveTo.
+ *
+ * @note This function assumes that text was inserted at the beginning of the line, which is
+ *       the case when used together with {@link Editor.setLine}. This is a simplifying assumption,
+ *       but may result in incorrect behavior if used outside the intended context (i.e. not by {@link toggleDone}).
+ *
+ *       Example: Assume {@link insertion}=`{text: "Hello World", moveTo: {line: 0, ch: 6}}`, where {@link insertion}.text
+ *                had been appended to a line with content "------":  `------Hello World`.
+ *                The cursor will be offset to the left by the number of characters that were already on the line.
+ *                Resulting in the incorrect result `------|Hello World` instead of the intended `------Hello |World`.
+ *
+ * @param startPos The starting cursor position
+ * @param insertion The inserted text and suggested cursor position within that text
+ */
+export const getNewCursorPosition = (startPos: EditorPosition, insertion: EditorInsertion): EditorPosition => {
+    const defaultMoveTo = { line: 0, ch: startPos.ch };
+    // Fill in any missing moveTo values using the default
+    const moveTo = { ...defaultMoveTo, ...(insertion.moveTo ?? {}) };
+    // Find the length of the line we're moving the cursor to, so that cursor isn't moved out of bounds
+    const destinationLineLength = insertion.text.split('\n')[moveTo.line].length;
+
+    return {
+        line: startPos.line + moveTo.line,
+        ch: Math.min(moveTo.ch, destinationLineLength),
+    };
 };


Test patch:
diff --git a/tests/Commands/ToggleDone.test.ts b/tests/Commands/ToggleDone.test.ts
index d1558f87..9f00b30f 100644
--- a/tests/Commands/ToggleDone.test.ts
+++ b/tests/Commands/ToggleDone.test.ts
@@ -3,7 +3,8 @@
  */
 
 import moment from 'moment';
-import { calculateCursorOffset, toggleLine } from '../../src/Commands/ToggleDone';
+import type { EditorPosition } from 'obsidian';
+import { getNewCursorPosition, toggleLine } from '../../src/Commands/ToggleDone';
 import { resetSettings, updateSettings } from '../../src/Config/Settings';
 import { StatusRegistry } from '../../src/StatusRegistry';
 import { Status } from '../../src/Status';
@@ -32,11 +33,20 @@ function testToggleLine(inputWithCursorMark: string, expectedWithCursorMark: str
     expect(input.length).toEqual(inputWithCursorMark.length - 1);
     expect(expected.length).toEqual(expectedWithCursorMark.length - 1);
 
+    const cursorPosition = (s: string): EditorPosition => {
+        // Split input string on cursor marker, and make array of lines
+        const linesBeforeCursor = s.split(cursorMarker, 1)[0].split('\n');
+        // Cursor was positioned at the end of the last line
+        const line = linesBeforeCursor.length - 1;
+        const ch = linesBeforeCursor[line].length;
+        return { line, ch };
+    };
+
     testToggleLineForOutOfRangeCursorPositions(
         input,
-        inputWithCursorMark.indexOf(cursorMarker),
+        cursorPosition(inputWithCursorMark),
         expected,
-        expectedWithCursorMark.indexOf(cursorMarker),
+        cursorPosition(expectedWithCursorMark),
     );
 }
 
@@ -56,13 +66,13 @@ function testToggleLine(inputWithCursorMark: string, expectedWithCursorMark: str
  */
 function testToggleLineForOutOfRangeCursorPositions(
     input: string,
-    initialCursorOffset: number,
+    initialCursorOffset: EditorPosition,
     expected: string,
-    expectedCursorOffset: number,
+    expectedCursorOffset: EditorPosition,
 ) {
     const result = toggleLine(input, 'x.md');
-    expect(result).toStrictEqual(expected);
-    const actualCursorOffset = calculateCursorOffset(initialCursorOffset, input, result);
+    expect(result.text).toStrictEqual(expected);
+    const actualCursorOffset = getNewCursorPosition(initialCursorOffset, result);
     expect(actualCursorOffset).toEqual(expectedCursorOffset);
 }
 
@@ -81,20 +91,24 @@ describe('ToggleDone', () => {
 
     it('should add hyphen and space to empty line', () => {
         testToggleLine('|', '- |');
+        testToggleLine('foo|bar', '- foobar|');
 
         updateSettings({ globalFilter: '#task' });
 
         testToggleLine('|', '- |');
+        testToggleLine('foo|bar', '- foobar|');
     });
 
     it('should add checkbox to hyphen and space', () => {
-        testToggleLine('|- ', '- [ |] ');
+        testToggleLine('|- ', '- [ ] |');
         testToggleLine('- |', '- [ ] |');
+        testToggleLine('- |foobar', '- [ ] foobar|');
 
         updateSettings({ globalFilter: '#task' });
 
-        testToggleLine('|- ', '- [ |] ');
+        testToggleLine('|- ', '- [ ] |');
         testToggleLine('- |', '- [ ] |');
+        testToggleLine('- |foobar', '- [ ] foobar|');
     });
 
     it('should complete a task', () => {
@@ -141,11 +155,11 @@ describe('ToggleDone', () => {
         );
 
         // With a trailing space at the end of the initial line, which is deleted
-        // when the task lines are regenerated, the cursor moves one character to the left:
+        // when the task lines are regenerated, the cursor does not move one character to the left:
         testToggleLine(
             '- [ ] I am a recurring task| 🔁 every day 📅 2022-09-04 ',
             `- [ ] I am a recurring task 🔁 every day 📅 2022-09-05
-- [x] I am a recurring tas|k 🔁 every day 📅 2022-09-04 ✅ 2022-09-04`,
+- [x] I am a recurring task| 🔁 every day 📅 2022-09-04 ✅ 2022-09-04`,
         );
 
         updateSettings({ globalFilter: '#task' });
@@ -179,10 +193,10 @@ describe('ToggleDone', () => {
             const line1 = '- [P] this is a task starting at Pro';
 
             // Assert
-            const line2 = toggleLine(line1, 'x.md');
+            const line2 = toggleLine(line1, 'x.md').text;
             expect(line2).toStrictEqual('- [C] this is a task starting at Pro');
 
-            const line3 = toggleLine(line2, 'x.md');
+            const line3 = toggleLine(line2, 'x.md').text;
             expect(line3).toStrictEqual('- [P] this is a task starting at Pro');
         });
 
@@ -192,10 +206,10 @@ describe('ToggleDone', () => {
             const line1 = '- [C] #task this is a task starting at Con';
 
             // Assert
-            const line2 = toggleLine(line1, 'x.md');
+            const line2 = toggleLine(line1, 'x.md').text;
             expect(line2).toStrictEqual('- [P] #task this is a task starting at Con');
 
-            const line3 = toggleLine(line2, 'x.md');
+            const line3 = toggleLine(line2, 'x.md').text;
             expect(line3).toStrictEqual('- [C] #task this is a task starting at Con');
         });
 
@@ -205,10 +219,10 @@ describe('ToggleDone', () => {
             const line1 = '- [P] this is a task starting at Pro, not matching the global filter';
 
             // Assert
-            const line2 = toggleLine(line1, 'x.md');
+            const line2 = toggleLine(line1, 'x.md').text;
             expect(line2).toStrictEqual('- [C] this is a task starting at Pro, not matching the global filter');
 
-            const line3 = toggleLine(line2, 'x.md');
+            const line3 = toggleLine(line2, 'x.md').text;
             expect(line3).toStrictEqual('- [P] this is a task starting at Pro, not matching the global filter');
         });
     });