Fix routing to account for rawToContentPos #16497
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
seanbudd
reviewed
May 8, 2024
source/braille.py
Outdated
| @@ -1345,18 +1345,27 @@ def update(self): | |||
| self._brailleInputIndStart = None | |||
|
|
|||
| def getTextInfoForBraillePos(self, braillePos): | |||
There was a problem hiding this comment.
could you add type information here given the rewrite of this function?
seanbudd
approved these changes
May 8, 2024
XLTechie
pushed a commit
to XLTechie/xlnvda
that referenced
this pull request
May 10, 2024
Fixup of nvaccess#16477 Summary of the issue: When writing nvaccess#16477, I hadn't realized that TextInfoRegion compensated for characters that aren't part of the text info. This mapping is stored in the _rawToContentPos attribute. This compensation is necessary for character movement based routing, but not for code point offset based routing. The following would happen without this pr. In Word, write a list item, e.g. 1. Hello Route to the fourth cell (offset 3) The third raw position (h) would correspond with content position zero (1) With moveToCodepointOffset, moving to offset 0 means moving to 1 not to h. Basically, the cursor would end up at the first cell, not at the fourth. Description of user facing changes Routing works again in Word list items. Description of development approach When using moveToCodepointOffset, move to the raw position, not to the actual content position. moveToCodepointOffset expects raw positions, not content positions. Don't use moveToCodepointOffset when the content position is 0, since we can simply collapse the textInfo at the start of the reading unit and return that. Note that moveToCodepointOffset would fail for everything up to offset 3 anyway, since it wouldn't be able to move.
|
I have noticed this behavior change: When selecting text with shift+down arrow in wordpad atleast in windows 10, review position moves to next line after last selected line (to first unselected line). This does not happen in notepad or in word 2019. Could this change have been introduced with this pr? |
|
No. I think it's more likely a cause of the behavioral changes you introduced with the several prs of redundant handleUpdate calls. |
|
They were reverted. It seems to be #16455. |
|
Unfortunately in browse mode routing buttons seem to work inaccurately at least with firefox in windows 10:
|
|
I'm going the revert route for now |
LeonarddeR
added a commit
to LeonarddeR/nvda
that referenced
this pull request
May 13, 2024
This reverts commit 892f1e2.
seanbudd
pushed a commit
that referenced
this pull request
May 14, 2024
Reverts #16477 , #16497 Issues fixed None Issues reopened Reopens #10960 Reason for revert Feature turns out to be unstable. Can this PR be reimplemented? If so, what is required for the next attempt Use a feature flag Ensure issues reported in Fix routing to account for rawToContentPos #16497 (comment) and Fix routing to account for rawToContentPos #16497 (comment) are covered by the fix
5 tasks
seanbudd
pushed a commit
that referenced
this pull request
Jul 30, 2024
Fixes #10960 Replaces #16477, #16497 Summary of the issue: There are cases where moving one character on a textInfo instance actually moves more than one unicode point offset. This is described by @mltony in the doc string for textInfos.TextInfo.moveToCodepointOffset. This causes of by one errors when cursor routing, since we're asking the textInfo to move by 1 characters, that might be presented by two or even more characters within the liblouis mapping. Description of user facing changes Cursor routing should be more reliable. Description of development approach @mltony's creation of moveToCodepointOffset allows us to move x code points from the start of the reading unit. As we're using 32 bit encoding for liblouis, every character as presented by liblouis is equal to one code point. Therefore we can safely assume that this method to move is much more reliable than the previous method.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Link to issue number:
Fixup of #16477
Summary of the issue:
When writing #16477, I hadn't realized that
TextInfoRegioncompensated for characters that aren't part of the text info. This mapping is stored in the_rawToContentPosattribute. This compensation is necessary for character movement based routing, but not for code point offset based routing. The following would happen without this pr.1. HellomoveToCodepointOffset, moving to offset 0 means moving to1not toh. Basically, the cursor would end up at the first cell, not at the fourth.Description of user facing changes
Routing works again in Word list items.
Description of development approach
moveToCodepointOffset, move to the raw position, not to the actual content position.moveToCodepointOffsetexpects raw positions, not content positions.moveToCodepointOffsetwhen the content position is 0, since we can simply collapse the textInfo at the start of the reading unit and return that. Note thatmoveToCodepointOffsetwould fail for everything up to offset 3 anyway, since it wouldn't be able to move.Testing strategy:
Tested the steps to reproduce as mentioned above.
Known issues with pull request:
None known
Code Review Checklist: