fix: Simplify cursor repositioning during the 'ToggleDone' command #1712

kedestin · 2023-03-01T09:49:23Z

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.

obsidian-tasks/src/Commands/ToggleDone.ts

Lines 118 to 122 in 8f6b58b

    
           // 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; 
        
           }

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

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)
Sample vault (prefix: vault - improvements to the Tasks-Demo sample vault)

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

When repositioning the cursor after toggling a task, the previous behavior had an explicit consideration for a completion date that was appended to a task and does so by regex'ing a hard-coded date format and a hard-coded completion symbol. Given that: * In most cases, the current behavior is that the cursor stays in place. * The hard-coded regex may not be applicable to other task formats This commit updates the way the the ToggleDone command repositions the cursor to *semantically* be "Leave it where it was before". The notable exceptions to this being toggling an *empty* line and a line that only has a list indictor. In both of those cases, the cursor is moved to the end of a line, to let a user continue typing out a task without needing to move the cursor manually.

claremacrae · 2023-03-01T10:07:10Z

Thank you!

The description of the change in behaviour is really clear.

When toggling a task, the cursor won't change columns. This essentially was the previous behavior, except in a few edge cases.

It would be helpful if this could also say what will happen if the cursor was inside a 'done date' before toggling, and the done date is removed.

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.

I do feel that makes this a bug-fix instead of a breaking change. The original behaviour was really annoying.

Which modes does this affect? I presume Live Preview and Source.

claremacrae · 2023-03-01T10:11:49Z

After some previous fixes to Toggle Done, the following file was created to support additional manual testing, by opening the Tasks-Demo folder as an Obsdian vault.

https://github.com/obsidian-tasks-group/obsidian-tasks/blob/main/resources/sample_vaults/Tasks-Demo/Manual%20Testing/Toggle%20Done%20Cursor%20Fixes.md

If you have time, would you mind running through the test steps, and updating anything that has been changed by this PR please?

(I guess it will also confirm that a load of things have not changed...)

kedestin · 2023-03-01T10:28:19Z

Thank you!

The description of the change in behaviour is really clear.

When toggling a task, the cursor won't change columns. This essentially was the previous behavior, except in a few edge cases.

It would be helpful if this could also say what will happen if the cursor was inside a 'done date' before toggling, and the done date is removed.

The description has been updated to document that if toggling a task would result in a cursor that is out of bounds, the cursor is moved to the end of the line.

- [x] ✅ 2023-|03-01 -> - [ ] |

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.

I do feel that makes this a bug-fix instead of a breaking change. The original behaviour was really annoying.

Title and description have been updated to position this as a bugfix.

Which modes does this affect? I presume Live Preview and Source.

As far as I'm aware, this only affects cursor repositioning as a result of running the ToggleDone command.

(I'm new to this plugin, and your question may be going over my head)

claremacrae · 2023-03-01T10:53:15Z

Hi @kedestin, I just wanted to say many thanks for this PR and all your replies - much appreciated.

I'm busy today, so will pick this up as soon as I can, as I realise it will be blocking your next PR...

kedestin · 2023-03-01T11:01:42Z

I'm busy today, so will pick this up as soon as I can, as I realise it will be blocking your next PR...

I'm thankful for how receptive you've been to these changes, and want to re-iterate that I'm happy to continue with this at your leisure.

I have a personal build that meets my needs already, so I personally have no time-pressure to get my changes upstreamed into the official repo. I apologise if my eagerness to respond to feedback made it seem like I was trying to force this in as quickly as possible.

claremacrae · 2023-03-01T21:22:01Z

Thank you.

I apologise if my eagerness to respond to feedback made it seem like I was trying to force this in as quickly as possible.

Don't worry, it didn't!

claremacrae

I've spent about an hour reading the code. I really like how many lines it has deleted, so how much of a simplification it is 😄 .

I've tried forming a mental model of how the new code works, and haven't really made much progress, so I've asked for a some comments in a few locations, to help whoever maintains this in future.

I also tried breaking the implementation in a few places to confirm the test coverage, and the tests did pick up the breakage.

Bit of a grumble to my past self (not requesting any changes in this area):
But as the author of the test tests, I do wish I had made any failures display any test failures by showing the wrong cursor position with a | marker in the string (if it was in range), for easier intepretation.

src/Commands/ToggleDone.ts

tests/Commands/ToggleDone.test.ts

claremacrae · 2023-03-03T18:57:59Z

Just to note that I've edited the description to detail the changes of behaviour observed in the tests.

tests/Commands/ToggleDone.test.ts

kedestin · 2023-03-04T23:31:57Z

I've tried forming a mental model of how the new code works, and haven't really made much progress, so I've asked for a some comments in a few locations, to help whoever maintains this in future.

In addition to the inline comments, I'll try to also describe the key ideas that I've been using to reason about this PR here:

toggleLine now tells the caller where to move the cursor, vs the previous implementation where the caller had to try and infer a cursor offset based on just the text that toggleLine returned.
Reason about cursor position using line number and column (a.k.a EditorPosition). The prior implementation treated the file as a single line which complicated how it dealt with repositioning the cursor across multiple lines.

kedestin · 2023-03-05T03:00:33Z

After some previous fixes to Toggle Done, the following file was created to support additional manual testing, by opening the Tasks-Demo folder as an Obsdian vault.

https://github.com/obsidian-tasks-group/obsidian-tasks/blob/main/resources/sample_vaults/Tasks-Demo/Manual%20Testing/Toggle%20Done%20Cursor%20Fixes.md

If you have time, would you mind running through the test steps, and updating anything that has been changed by this PR please?

(I guess it will also confirm that a load of things have not changed...)

Not sure how to attest that I did this other than by saying "I did it" and hoping that you trust that I'm acting in good faith.

Running through the manual tests did highlight a behavior change that I don't think is particularly obvious when looking at the unit tests:

The vertical bar | denotes the position of the editor cursor

Before

Start	End
`wib\|ble`	`- wib\|ble`
`>\|> wibble`	`>\|> - wibble`
`- wi\|bble`	`- [ ] wi\|bble`
`>\|> - wibble`	`>\|> - [ ] wibble`

After

Start	End
`wib\|ble`	`- wibble\|`
`>\|> wibble`	`>> - wibble\|`
`- wi\|bble`	`- [ ] wibble\|`
`>\|> - wibble`	`>> - [ ] wibble\|`

The current tests do technically capture this behavior change, but I added some more assertions that make it more obvious that the cursor will jump to the end of the line.

…al test more explicit

tests/Commands/ToggleDone.test.ts

claremacrae · 2023-03-05T17:40:43Z

I think this is now ready to go ahead and merge. Do you agree @kedestin?

kedestin · 2023-03-05T19:18:28Z

Looks good to me.

claremacrae · 2023-03-06T07:48:41Z

@kedestin Many thanks for a great simplification of the code - I learned a lot from it too!

kedestin mentioned this pull request Mar 1, 2023

refactor: Modularizing task serialization, task deserialization and suggestion building #1703

Closed

14 tasks

kedestin changed the title ~~feat!!: Simplify cursor repositioning during the ToggleDone command~~ fix: Simplify cursor repositioning during the ToggleDone command Mar 1, 2023

claremacrae requested changes Mar 3, 2023

View reviewed changes

claremacrae reviewed Mar 3, 2023

View reviewed changes

tests/Commands/ToggleDone.test.ts Outdated Show resolved Hide resolved

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

kedestin added 6 commits March 4, 2023 17:59

Revert accidental change to test name

3a32a09

Use Math.min instead of locally written min function

ea74b0b

Make EditorInsert.text a string instead of string[]

3a6fd48

Remove comment documentation that is no longer relevant

eb5ac55

Rename variable to be slightly less confusing

a3ddf25

Make EditorInsertion documentation a bit more precise

bbb3f6c

kedestin added 3 commits March 4, 2023 18:48

Improve inline documentation for cursorPosition

2e27ce2

Add more precise documentation for getNewCursorPosition

c177a16

Refactor getNewCursorPosition to be a bit easier to read

bdf0e62

Add more asserts to ToggleDone test to make behavior observed in manu…

0c37f42

…al test more explicit

claremacrae reviewed Mar 5, 2023

View reviewed changes

tests/Commands/ToggleDone.test.ts Show resolved Hide resolved

claremacrae changed the title ~~fix: Simplify cursor repositioning during the ToggleDone command~~ fix: Simplify cursor repositioning during the 'ToggleDone' command Mar 5, 2023

claremacrae approved these changes Mar 5, 2023

View reviewed changes

claremacrae assigned kedestin Mar 5, 2023

claremacrae merged commit feaafaf into obsidian-tasks-group:main Mar 6, 2023

claremacrae mentioned this pull request Mar 22, 2023

docs: Bump the next version to 2.0.0 #1785

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Simplify cursor repositioning during the 'ToggleDone' command #1712

fix: Simplify cursor repositioning during the 'ToggleDone' command #1712

kedestin commented Mar 1, 2023 •

edited by claremacrae

Loading

claremacrae commented Mar 1, 2023

claremacrae commented Mar 1, 2023

kedestin commented Mar 1, 2023 •

edited

Loading

claremacrae commented Mar 1, 2023

kedestin commented Mar 1, 2023

claremacrae commented Mar 1, 2023

claremacrae left a comment •

edited

Loading

claremacrae commented Mar 3, 2023

kedestin commented Mar 4, 2023

kedestin commented Mar 5, 2023

claremacrae commented Mar 5, 2023

kedestin commented Mar 5, 2023

claremacrae commented Mar 6, 2023

	// 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;
	}

fix: Simplify cursor repositioning during the 'ToggleDone' command #1712

fix: Simplify cursor repositioning during the 'ToggleDone' command #1712

Conversation

kedestin commented Mar 1, 2023 • edited by claremacrae Loading

Description

Motivation and Context

How has this been tested?

Screenshots (if appropriate)

Types of changes

Checklist

Terms

claremacrae commented Mar 1, 2023

claremacrae commented Mar 1, 2023

kedestin commented Mar 1, 2023 • edited Loading

claremacrae commented Mar 1, 2023

kedestin commented Mar 1, 2023

claremacrae commented Mar 1, 2023

claremacrae left a comment • edited Loading

Choose a reason for hiding this comment

claremacrae commented Mar 3, 2023

kedestin commented Mar 4, 2023

kedestin commented Mar 5, 2023

claremacrae commented Mar 5, 2023

kedestin commented Mar 5, 2023

claremacrae commented Mar 6, 2023

kedestin commented Mar 1, 2023 •

edited by claremacrae

Loading

kedestin commented Mar 1, 2023 •

edited

Loading

claremacrae left a comment •

edited

Loading