Fix TextInfo.collapse() in Notepad++ by mltony · Pull Request #18338 · nvaccess/nvda

mltony · 2025-06-25T20:15:04Z

Link to issue number:

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. There are several problems with this:

It breaks a bunch of add-ons that make use of TextInfo API. In particular, my WordNav add-on is broken as reported by my users. Probably more are broken - since collapse is frequently used method.
With this commit in place there is no way to actually collapse a TextInfo in place. This severely impacts the entire TextInfo API as many actions are now impossible to perform.
This is not intuitive: the name collapse() doesn't reflect that the TextInfo would also jump.
This is inconsistent across different apps - it only jumps in Notepad++.

@nvdaes tried to fix #17430 with this commit.

I propose this PR as a way to fix TextInfo API without breaking #17430.

Description of user facing changes:

N/A

Description of developer facing changes:

TextInfo.collapse() now works correctly in Notepad++.

Description of development approach:

Created a new method TextInfo.collapseAndMaybeMoveToNextParagraph so that the logic implemented by @nvdaes can stay in place, but not affect other users of TextInfo API.

TBH I would rather not pollute TextInfo API, but I don't have visibility into braille subsystem - and I don't have a Braille display to test any changes - and @nvdaes claims there is no other way to fix #17430. So This PR is a compromise: I kept @nvdaes's logic in place at the cost of introducing a new unnecessary method in TextInfo API. I think a cleaner way would be for @nvdaes to dfix the issue in def nextLine and def previousLine in braille.py., but @nvdaes claims that's impossible (see the discussion in #18320).

Testing strategy:

Tested with use case provided in #18320.

@nvdaes, can you test that this PR doesn't break #17430? I don't have a Braille display to test it on my end.

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

nvdaes · 2025-06-25T20:54:44Z

Tony wrote:

@nvdaes claims that's impossible

No, I didn't claim this. I said:

I didn't find a cleaner way to fix a braille issue.

I've reviewed the source code and likes good to me. I haven't tested it yet. I think that your issue isn't triaged, may require a product decission by NV Access, since breaking changes were accepted, so I'll wait to test it.

@seanbudd

AppVeyorBot · 2025-06-25T21:00:08Z

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/nwoef4ych9lcfkj4/artifacts/output/nvda_snapshot_pr18338-36994,9d1cf117.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 1.4,
INSTALL_END 1.0,
BUILD_START 0.0,
BUILD_END 20.4,
TESTSETUP_START 0.0,
TESTSETUP_END 0.4,
TEST_START 0.0,
TEST_END 20.5,
FINISH_END 0.1

See test results for failed build of commit 9d1cf1176e

Co-authored-by: Sean Budd <seanbudd123@gmail.com>

Co-authored-by: Sascha Cowley <16543535+SaschaCowley@users.noreply.github.com>

mltony · 2025-06-26T05:27:00Z

@nvdaes, please test this at your earliest convenience. I don't think this requires a project decision as this is clearly a bug to be fixed - but @seanbudd and @SaschaCowley are to correct me if I'm wrong.

nvdaes · 2025-06-26T05:32:38Z

@seanbudd left comments, so they have accepted this, and I like this PR a lot.
I'll test this in a few hours.
Thanks @mltony

LeonarddeR · 2025-06-26T06:38:57Z

I'm a bit worried about these changes as well as the changes in #17431. In my modest opinion, they obfuscate an underlying bug, namely that _getLineOffsets is broken or rather does not conform to what NVDA expects. So a fix should not occur in collapse, but _getLineOffsets should be fixed.
The problem here is that Notepad++ returns a line length of 0 for the last line of the document.
Fixing the last assignment in _getLineOffsets like this might work. If not, I must dive in deeper:
end = start + min(1, watchdog.cancellableSendMessage(self.obj.windowHandle, SCI_LINELENGTH, line, 0))

nvdaes · 2025-06-26T15:31:34Z

I've tested this and works as expected. Thanks @mltony
Anyway, I think that it would be better to try @LeonarddeR approach. Imo it would be cleaner.

LeonarddeR · 2025-06-26T16:18:31Z

I think we must also check for the case where word wrap is enabled. I always disable that in np++.

mltony · 2025-06-26T16:51:18Z

@LeonarddeR, I agree fixing the underlying bug would be a better path. Can you explain in more details - like steps to repro - what is broken with _getLineOffsets implementation? I can take a look into this myself.

Notepad++ returns a line length of 0 for the last line of the document

What is it supposed to return for an empty line?

LeonarddeR · 2025-06-26T17:47:38Z

Can you explain in more details - like steps to repro - what is broken with _getLineOffsets implementation?

You can compare the behavior of NotePad++ and a document in classic notepad, for example. Compare the output of _getStoryLength and _getLineOffsets for several offsets.

Notepad++ returns a line length of 0 for the last line of the document

What is it supposed to return for an empty line?

For an empty line, since an empty line contains \r\n.
So what happens is the following. When you have a document that contains:

Offset 1-5: hello
offset 6: \r
offset 7: \n
_getLineOffsets returns (7, 7) at offset 7, namely the offset of the second, empty, last line. However this is equal to being collapsed at the end of the first line. I think that's why braille thinks you're still at that first line.
Strictly spoken scintilla is correct, since the second line is in fact empty and it doesn't contain \r\n.
I'm also happy to take this one if you prefer. TextInfos are hard to grasp, you might know that better than me.

mltony · 2025-06-26T18:30:01Z

I just checked, in Notepad++ both _getStoryLength() and _getLineOffsets() appear to return correct (or at least reasonable) results - at least in your example.
I can't compare it to classic Notepad as it uses UIA TextInfo, which is not offset-based.
An empty line in NPP contains '\r\n' only if it is followed by another line. If there is no following line, it makes sense that there is no '\r\n' to me. In fact I cant imagine the last line containing '\r\n' since if it did that would imply that there is another line following it and this would be even more confusing.
You say: "However this is equal to being collapsed at the end of the first line." - I think there is difference. When collapsed at the end of the first line the offset is 5 - e.g. right before '\r\n'. However the beginning of the second line is offset=7, e.g. right after '\r\n'

Let me see if I can repro this without Braille display at hand.

mltony · 2025-06-26T18:38:55Z

Ok, I think I found the root cause: in Notepad++ when you try to do TextInfo.move('line', 1) - then it wouldn't move to the very last line if it's empty. This works correctly in classic Notepad. I'll see if I can fix that.

mltony · 2025-06-26T19:36:15Z

I think I found the root cause why TextInfo.move won't move to the last line. I'll close this PR for now - will send out another PR that fixes the root cause soon.

@nvdaes

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: None 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 #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 #17430. Known issues with pull request: None

Fix TextInfo.collapse()

2a16454

mltony changed the title ~~Fix TextInfo.collapse()~~ Fix TextInfo.collapse() in Notepad++ Jun 25, 2025

mltony marked this pull request as ready for review June 25, 2025 22:01

mltony requested a review from a team as a code owner June 25, 2025 22:01

mltony requested a review from seanbudd June 25, 2025 22:01

seanbudd reviewed Jun 26, 2025

View reviewed changes

Comment thread source/textInfos/__init__.py Outdated

SaschaCowley requested changes Jun 26, 2025

View reviewed changes

Comment thread user_docs/en/changes.md Outdated

Comment thread source/textInfos/__init__.py Outdated

SaschaCowley added this to the 2025.2 milestone Jun 26, 2025

mltony and others added 3 commits June 25, 2025 22:16

Update source/textInfos/__init__.py

b09e4d6

Co-authored-by: Sean Budd <seanbudd123@gmail.com>

Update source/textInfos/__init__.py

badc027

Co-authored-by: Sascha Cowley <16543535+SaschaCowley@users.noreply.github.com>

Addressing comments

bcddb3f

mltony closed this Jun 26, 2025

mltony mentioned this pull request Jun 26, 2025

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

Merged

5 tasks

Uh oh!

Conversation

mltony commented Jun 25, 2025

Link to issue number:

Summary of the issue:

Description of user facing changes:

Description of developer facing changes:

Description of development approach:

Testing strategy:

Known issues with pull request:

Code Review Checklist:

Uh oh!

nvdaes commented Jun 25, 2025

Uh oh!

AppVeyorBot commented Jun 25, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mltony commented Jun 26, 2025

Uh oh!

nvdaes commented Jun 26, 2025

Uh oh!

LeonarddeR commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nvdaes commented Jun 26, 2025

Uh oh!

LeonarddeR commented Jun 26, 2025

Uh oh!

mltony commented Jun 26, 2025

Uh oh!

LeonarddeR commented Jun 26, 2025

Uh oh!

mltony commented Jun 26, 2025

Uh oh!

mltony commented Jun 26, 2025

Uh oh!

mltony commented Jun 26, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

LeonarddeR commented Jun 26, 2025 •

edited

Loading