-
-
Notifications
You must be signed in to change notification settings - Fork 638
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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.
@michaelDCurran The current implementation of |
Certainly worth trying.
No doubt on some implementation it either raised an error and or it
inappropriately moved start offset. But yes, I really can't remember now.
Message ID: ***@***.***>
|
Changing to
|
I had another quick look. It seems that |
Nope:
|
Co-authored-by: Leonard de Ruijter <[email protected]>
O wow, this is very weird. |
@@ -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 |
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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:
def getTextInfoForCodepointMovement(self) -> Self: | |
def _getTextInfoForCodepointMovement(self) -> Self: |
@mltony - do you plan to do any further work here? |
No this is not redundant unfortunately, and as said in #16484 (comment) this also applies to Wordpad so likely deserves a broader scope. |
Sorry, somehow I lost track of this PR. Would it be ok with you if I finish it around January 2025 as well? |
Yep no problem. I think we can leave this one open for now. |
Link to issue number:
N/A
Summary of the issue:
TextInfo.moveToCodepointOffset() somtimes raises exception in PoEdit:
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 thatcollapse(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 inITextDocumentTextInfo
, 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: