Fix LibreOffice braille scrolling#19204
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a regression in LibreOffice braille scrolling introduced by PR #18348. The fix replaces the class-level attribute allowMoveToOffsetPastEnd with a unit-aware method allowMoveToUnitOffsetPastEnd to allow fine-grained control over when moving past the end of text is permitted based on the movement unit.
- Introduced
allowMoveToUnitOffsetPastEnd(unit)method to replace the boolean class attribute - Applied the new method to virtual buffers and compound document leaf text info classes
- Added comprehensive unit tests for compound document text navigation
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| user_docs/en/changes.md | Documents the deprecation of allowMoveToOffsetPastEnd attribute |
| tests/unit/test_compoundDocuments.py | Adds comprehensive unit tests for TreeCompoundTextInfo navigation across character, word, and line units |
| tests/unit/objectProvider.py | Adds _isEqual method to PlaceholderNVDAObject for proper object comparison in tests |
| source/virtualBuffers/init.py | Implements allowMoveToUnitOffsetPastEnd to return False for all units (no insertion point in virtual buffers) |
| source/textInfos/offsets.py | Replaces allowMoveToOffsetPastEnd attribute with allowMoveToUnitOffsetPastEnd(unit) method and adds deprecation handling |
| source/compoundDocuments.py | Introduces CompoundTextLeafTextInfo mixin that allows moving past end only for character and word units |
| source/appModules/soffice.py | Applies CompoundTextLeafTextInfo mixin to SymphonyTextInfo for proper LibreOffice braille navigation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
|
I think we would all feel a lot safer merging this if we get system tests for libre Office and notepad++. I'm not sure how feasible it is to install them on GitHub runners, but I think it's worth investigating as we've had this be a constant, testable problem. |
|
Agreed there. Problem with LibreOffice is that it has a first start experience with tips at startup. I think they'll be getting in the way during tests, unless there's a workaround for that. May be @michaelweghorn can chime in here? |
Tips (and other things like infobars, first start wizard,...) can be disabled via a LibreOffice option/setting. https://wiki.documentfoundation.org/Deployment_and_Migration#Post_deployment_configuration describes some ways those can be deployed/set before starting LibreOffice. |
|
@LeonarddeR , I like this a lot. |
|
@nvdaes Does the issue you're describing also apply to NVDA 2025.3 and older? In that case, we definitely need to handle that separately, though it is slightly related. |
|
@LeonarddeR wrote:
I tested this with NVDA 2025.3.1 and the issue, i.e., cursor is not moved to the end of the document from the last braille window when scrolling forward, is happening, so the described issue is not a regression. I was wondering if your new function should cover this case, to allow move the cursor to the end when no more text or objects are available. |
I might be able to do this. |
|
Leonard wrote:
Great. If this should be done in a different PR, I don't mind to implement this using your new function, or if you can do it here, great too. |
…he actual end rather than stalling before it
|
@nvdaes I think the last commit should fix this and make this consistent to other textInfo impls. |
|
Leonard wrote:
The behavior is not consistent with other programs, since the cursor moves to the last character of the document, not to the end, i.e., after the last character.
This produces a consistent behavior in LibreOffice Writer, though perhaps another solution is better. |
|
@nvdaes wrote:
No no, this solution is actually very elegant and perfect, thank you very much for proposing it. I added it just now. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Thanks @LeonarddeR - this fix looks good to me but would also like a review from @SaschaCowley . |
SaschaCowley
left a comment
There was a problem hiding this comment.
Thanks, @LeonarddeR, this looks good to me too
|
Apart from that I'm not very comfortabele with system test writing yet, I think it is a problem that a runner might need to install LibreOffice before it can be used. A LibreOffice installation takes a significant amount of time. May be we can use the version in the portable apps format |
Yeah caching a portable format I think would be ideal |
Link to issue number:
Fixes #19152
Fixup for #18348,
Supersedes #19203, #19171
Summary of the issue:
In LibreOffice, it is not possible to use braille panning since #18348.
Description of user facing changes:
Panning works again in LibreOffice
Description of developer facing changes:
removed OffsetsTextInfo.allowMoveToOffsetPastEnd.
Description of development approach:
Rather than having one
allowMoveToOffsetPastEndclass attribute, introduce aallowMoveToUnitOffsetPastEndmethod that takes a unit.Testing strategy:
Tested in libre office.
Known issues with pull request:
Deprecation pattern is a bit tricky here because
allowMoveToOffsetPastEndis a class level attribute. I think the pattern in this pr suffices because it was only used at the instance level.Code Review Checklist: