Use moveToCodepointOffset for cursor routing#16477
Merged
Merged
Conversation
937ed9f to
d55ead7
Compare
Collaborator
Author
|
This is ready for review. Only the changelog entry is missing, because there's no 2024.3 section yet. |
seanbudd
approved these changes
May 7, 2024
Collaborator
Author
|
I added a changelog entry. Note that i also credited @mltony, since this pr is basically a very small one that completely builds on his work for unicode point offset navigation. |
5 tasks
seanbudd
pushed a commit
that referenced
this pull request
May 8, 2024
Fixup of #16477 Summary of the issue: When writing #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.
LeonarddeR
added a commit
to LeonarddeR/nvda
that referenced
this pull request
May 13, 2024
This reverts commit a98855d.
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.
This file contains hidden or 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
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:
Fixes #10960
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
moveToCodepointOffsetallows us to move x code points from the start of the reading unit. As we're using 32 bit 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.Testing strategy:
Known issues with pull request:
None known
Code Review Checklist: