Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix TextInfo.moveToCodepointOffset() in PoEdit #16484

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mltony
Copy link
Contributor

@mltony mltony commented May 5, 2024

Link to issue number:

N/A

Summary of the issue:

TextInfo.moveToCodepointOffset() somtimes raises exception in PoEdit:

RuntimeError: Inner textInfo text ' lock' doesn't match outer textInfo text 'Toggle input lock

Description of user facing changes

N/A

Description of development approach

This is caused by a bug (or peculiarity) of TextInfo implementation in PoEdit. It is using ITextDocumentTextInfo and when calling .collapse(True) in some cases it actually doesn't collapse to the very end. This seems to be happening when there is only a single line in the text box and when you expand to paragraph, it reports newline being the last character of that paragraph, however when collapsing to the end, it places textInfo right before that last newline character. This breaks my assumption that collapse(True) always collapses to the end.
In order to work around this peculiarity I created TextInfo.getTextInfoForCodepointMovement() method, which just returns a copy of current textInfo, except for overloaded implementation in ITextDocumentTextInfo, where it checks for collapse peculiarity, and, if detected, then trims that last fake newline character.

Testing strategy:

Tested manually in PoEdit.

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.

@mltony mltony changed the base branch from master to beta May 5, 2024 20:29
@mltony mltony marked this pull request as ready for review May 5, 2024 20:30
@mltony mltony requested a review from a team as a code owner May 5, 2024 20:30
@mltony mltony requested review from gerald-hartig and removed request for a team May 5, 2024 20:30
seanbudd
seanbudd previously approved these changes May 6, 2024
@seanbudd seanbudd added this to the 2024.2 milestone May 6, 2024
Copy link
Collaborator

@LeonarddeR LeonarddeR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pull request now addresses a symptom, while in reality it should address the actual problem IMO.
Shouldn't this pr address the issue in collapse itself? I.e. rather than fixing this only for code point offset, I'd rather see the collapse method revised to address it.

source/NVDAObjects/window/edit.py Outdated Show resolved Hide resolved
@LeonarddeR
Copy link
Collaborator

@michaelDCurran The current implementation of ITextDocumentTextInfo has a manual implementation to collapse, while there is in fact a Collapse method on ITextRange. You wrote the collapse method fifteen years ago, so I'm inclined to say you don't remember why you implemented it the way you you did😉.
I think we should consider adapting collapse to execute self._rangeObj.Collapse(not end)

@michaelDCurran
Copy link
Member

michaelDCurran commented May 6, 2024 via email

@mltony
Copy link
Contributor Author

mltony commented May 6, 2024

Changing to ITextRange::collapse() won't help as it exhibits exactly the same problem:

>>> t=focus.makeTextInfo('caret')
>>> t.expand('paragraph')
>>> t._rangeObj.start, t._rangeObj.end
(0, 10)
>>> tt=t.copy()
>>> tt.collapse(False)
>>> tt._rangeObj.start, tt._rangeObj.end
(0, 0)
>>> tt=t.copy()
>>> tt.collapse(True)
>>> tt._rangeObj.start, tt._rangeObj.end
(9, 9)

@seanbudd seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label May 7, 2024
@LeonarddeR
Copy link
Collaborator

I had another quick look. It seems that ITextRange.SetRange causes the off by one issue, i.e. it doesn't allow setting the range to exclusive offsets.
Note that when forcing UIA in PoEdit, this problem doesn't occur. This suggests that the UIA proxy either doesn't use ITextRange internally or properly accounts for this issue.
Could you try changing collapse to set self._rangeObj.Start and self._rangeObj.End directly? That's what setEndPoint does, so I guess that'll work here.

@mltony
Copy link
Contributor Author

mltony commented May 7, 2024

Nope:

>>> t=focus.makeTextInfo('caret')
>>> t.expand('paragraph')
>>> t._rangeObj.start, t._rangeObj.end
(0, 10)
>>> tt=t.copy()
>>> tt._rangeObj.start, tt._rangeObj.end
(0, 10)
>>> tt._rangeObj.start = 10
>>> tt._rangeObj.start, tt._rangeObj.end
(9, 9)

seanbudd
seanbudd previously approved these changes May 8, 2024
@seanbudd seanbudd dismissed their stale review May 8, 2024 02:11

pending questions

@LeonarddeR
Copy link
Collaborator

O wow, this is very weird.
This is also reproducible in Wordpad by the way, so I think it is a global ITextRange thing. Another reason to use UIA for these controls, unfortunately we had to revert that.
I understand that it is pretty hard to get this right, but I still feel we need to fix this in the textInfo implementation rather than only for code point offset fetching. May be we should do that in a follow up, though.

@@ -842,6 +843,19 @@ def updateSelection(self):
self.obj.ITextSelectionObject.start=self._rangeObj.start
self.obj.ITextSelectionObject.end=self._rangeObj.end

def getTextInfoForCodepointMovement(self) -> Self:
# In PoEdit ITextDocumentTextInfo sometimes cannot access the last character when that character is
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also reproducible in Wordpad, so seems to be ITextRange related in general. Could you please update this comment accordingly?

@@ -655,7 +655,10 @@ def getMathMl(self, field):
@raise LookupError: If MathML can't be retrieved for this field.
"""
raise NotImplementedError


def getTextInfoForCodepointMovement(self) -> Self:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be a private method:

Suggested change
def getTextInfoForCodepointMovement(self) -> Self:
def _getTextInfoForCodepointMovement(self) -> Self:

@seanbudd seanbudd marked this pull request as draft May 10, 2024 02:17
@seanbudd seanbudd removed this from the 2024.2 milestone May 24, 2024
@seanbudd seanbudd changed the base branch from beta to master May 24, 2024 00:24
@seanbudd
Copy link
Member

seanbudd commented Sep 1, 2024

@mltony - do you plan to do any further work here?
@LeonarddeR - is this perhaps redundant after #16889 / PoEdit 3.5+

@LeonarddeR
Copy link
Collaborator

No this is not redundant unfortunately, and as said in #16484 (comment) this also applies to Wordpad so likely deserves a broader scope.

@mltony
Copy link
Contributor Author

mltony commented Sep 3, 2024

Sorry, somehow I lost track of this PR. Would it be ok with you if I finish it around January 2025 as well?

@seanbudd
Copy link
Member

seanbudd commented Sep 3, 2024

Yep no problem. I think we can leave this one open for now.

@seanbudd seanbudd added blocked blocked/merge-conflicts Merge conflicts exist on this PR labels Sep 3, 2024
@seanbudd seanbudd removed the blocked label Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked/merge-conflicts Merge conflicts exist on this PR conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants