Fix OffsetTextInfo.collapse() and OffsetTextInfo.move()

[mltony]

Link to issue number:

Fixes #18320.

This is my second attempt to fix it without breaking #17430. For the reference, the first attempt was #18338, which is abandoned, and which didn't fix the root cause, but just decoupled @nvdaes's commit from TextInfo API. This PR however fixes the root cause of #17430.

Summary of the issue:

Commit 70cd311 by @nvdaes introduced a bug in TextInfo API: TextInfo.collapse() now unnecessarily jumps to the next paragraph in Notepad++. This bug also made its way into 2025.1 release.

I investigated the braille issue #17430 further and it boiled down to an issue with OffsetTextInfo.move() implementation: it turns out that unless it moves by character, it is unable to move to the very last position in the document. Fixing OffsetTextInfo.move() implementation.

Description of user facing changes:

N/A

Description of developer facing changes:

OffsetTextInfo.collapse() now works correctly in Notepad++.
OffsetTextInfo.move() can now move to the very last position even when unit is set to line or paragraph.

Description of development approach:

1. In OffsetTextInfo.move() modified the condition when self.allowMoveToOffsetPastEnd to increase highLimit regardless of current unit - previously this would only allow when unit is character.

2. Removed ScintillaTextInfo.expand() override.

Testing strategy:

1. Tested with use case provided in TextInfo.collapse(end=True) moves textInfo to the next paragraph in Notepad++ #18320.

2. Tested that OffsetTextInfo.move('line', 1) can now successfully go to the very last line in Notepad++ even if the last line is empty. This should be the proper fix for Notepad++: braille doesn't move to the next line if last line is empty #17430.

Known issues with pull request:

N/A

Code Review Checklist:

• Documentation:
  • Change log entry
  • User Documentation
  • Developer / Technical Documentation
  • Context sensitive help for GUI changes
• Testing:
  • Unit tests
  • System (end to end) tests
  • Manual testing
• UX of all users considered:
  • Speech
  • Braille
  • Low Vision
  • Different web browsers
  • Localization in other languages / culture than English
• API is compatible with existing add-ons.
• Security precautions taken.

@coderabbitai summary

[AppVeyorBot]

• PASS: Translation comments check.
• PASS: License check.
• PASS: Unit tests.
• FAIL: System tests (tags: installer NVDA). See test results for more information.
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/u6s5een6qgp32qok/artifacts/output/nvda_snapshot_pr18348-37028,e48645fa.exe
• CI timing (mins):
  INIT 0.0,
  INSTALL_START 1.5,
  INSTALL_END 1.0,
  BUILD_START 0.0,
  BUILD_END 20.3,
  TESTSETUP_START 0.0,
  TESTSETUP_END 0.4,
  TEST_START 0.0,
  TEST_END 20.7,
  FINISH_END 0.1

See test results for failed build of commit e48645facd

[mltony]

@nvdaes, could you test that this doesn't break #17430?
Also, cc: @LeonarddeR

[nvdaes]

Thanks @mltony . This works for me as expected in Notepad++ with braille when moving to the next line, if last line is empty.

[mltony]

Thanks @nvdaes!
Also regarding fixing the unit test. I haven't quite figured out why chromeTests.test_pr11606 started to fail. For some reason this PR changes speech after one of the keystrokes.
However the new speech makes more sense to me. In this test when end key is pressed it seems to me that the speech should be "blank" rather then "link" since this keystroke takes the cursor past the last link in the line. So the original behavior seems to be incorrect - even though I don't fully understand how did this PR fix it.
So at this point I just changed expected speech in the test and now it passes.

[LeonarddeR]

This is an interesting find. I'm afraid though that this implementation might report nonexisting blank lines when moving by line with the review cursor. I guess this clarifies why the chrome test behavior differs now.
A fix should be a bit less generalised.

Also note that the comment is no longer accurate now. It says: #: move with unit_character can move 1 past story length to allow braille routing to end insertion point. (#2096).

I agree though that what you said in an earlier comment in the previous pr is true, namely that the last empty line of a document has a length of 0. Treating that as 1 won't work due to the highLimit in the move method.

Note also that The story length of an empty notepad classic document (i.e. a default EditTextInfo) is 1, not 0. It uses a + 1 trick, though there is an ambiguous comment saying that an off by one error is created by the + 1. I think this means no more than that this trick causes the story length to be reported one higher than the actual length. It seems that EditTextInfo doesn't treat line breaks as actual characters.

A quick fix would be doing _getStoryLength + 1, but this introduces the same ambiguity as in EditTextInfo.

[mltony]

@LeonarddeR,

I'm afraid though that this implementation might report nonexisting blank lines when moving by line with the review cursor.

I don't think so. If I read this code correctly, allowMoveToOffsetPastEnd indicates whether we are allowed to move the cursor past the last character. This is for example allowed in Notepad++ but not allowed in virtual buffers. But the code in OffsetTextInfo.move() only respects this when moving by character. This part doesn't make any sense to me - hence this PR. If you can move to a certain position by character - why we can't move to it by line.

However having said that, I agree that any change in this low-level TextInfo implementation might cause undesired behavior somewhere else, so I'll keep an eye on any bug reports if there are any.

Also updated the comment.

[LeonarddeR]

I can't find any strange behavior with the review cursor in neither classic notepad nor NP++. NP++ improves in such a way that the review cursor can now also reach the last line, which is great.

[LeonarddeR]

I just added the merge early label since I think this needs some broader testing in Alpha.

[SaschaCowley]

@jcsteh looks like you implemented this in 792074e. Do you remember if there was a reason to restrict this to navigation by character only? Was it just because that is what was strictly necessary to solve #2096?

[jcsteh]

I don't remember the specifics. However, I'll point out that "the very last position" (and what is supported there) can differ between text implementations.

1. For some non-editable implementations, the length might not be a valid start offset.
2. Normally, length is considered part of the final line, not a blank line. That raises the question: does it actually make sense to move there by line or paragraph? Could that confuse some callers to behave like there is a blank line at the end when there in fact isn't?

I don't have answers to these points, but they need to be considered if they haven't already. Note also that I haven't read this entire thread, only skimmed it, so I might have missed something.

[seanbudd]

Would we consider this an API breaking change?

[mltony]

@seanbudd, regarding API breaking, I just wanted to mention yet again that this is also a bug. As I mentioned before, without this PR the entire textInfo API is unusable as there is no reliable way to collapse textInfo and this breaks a few of my add-ons. It would be unfortunate if we have to wait until next major release to fix a bug like that.