Hotfix maxlength #2
Conversation
koppor
commented
Apr 16, 2024
•
koppor
commented
- Stackoverflow report: https://stackoverflow.com/q/78332309/873282
- JDK issue: https://bugs.openjdk.org/browse/JDK-8330462
- JabRef issue: Uncaught exception occurred in Thread[#53,JavaFX Application Thread,5,main] (-> Run JabRef as Admin as a fix) JabRef/jabref#11151
|
Upload action downgrade because of actions/upload-artifact#485 |
| @@ -362,6 +362,7 @@ private String GetText(int maxLength) { | |||
| String text = (String)getAttribute(TEXT); | |||
| if (text == null) return null; | |||
| validateRange(text); | |||
| maxLength = Math.max(-1, maxLength); | |||
There was a problem hiding this comment.
@koppor when the issue at hand happens, GetText(int maxLength) is called from native (GlassTextRangeProvider::GetText), and gets an unexpected value, which is maxLength = Integer.MAX_VALUE (so maxLength = +2147483647).
Note that if you add to that number any start positive value, it gives Integer.MIN_VALUE+start (that is -2147483647+start), and that is what leads to think of "negative" values for maxLength, but that is not the case.
Therefore, a better fix is either:
maxLength = maxLength == Integer.MAX_VALUE ? -1 : maxLength;
or:
int endOffset = maxLength < Integer.MAX_VALUE && maxLength != -1 ? Math.min(end, start + maxLength) : end;
The patch/workaround is easy in this case.
However, the hard part is finding out why it is happening for some Windows users and not for everybody.
And also, if WinTextRangeProvider should be called only when some accessibility feature is enabled (like Narrator), why are there reports from users without any such feature (at least that they are aware of)?
There was a problem hiding this comment.
@jperedadnr Thank you so much for the explanation!
However, the hard part is finding out why it is happening for some Windows users and not for everybody.
Fortunately, there is a reproducer now: Install the DeepL app (https://www.deepl.com/app). I am (unfortunately) serious on that. - https://stackoverflow.com/a/78340343/873282 contains more details for the reproducer.
why are there reports from users without any such feature (at least that they are aware of)
I would assume that helper apps trying to read from components from the screen use accessibility features to get the text content.
There was a problem hiding this comment.
I saw your answer, and I've tried it myself, but I'm on those that can't reproduce this issue in any way on my Windows machine (I have latest Windows 11 23H2).
Using DeepL doesn't trigger calls to WinRangeTextProvider for me.
I get these for instance using Narrator. However, even then, all maxLength values are "normal".
There was a problem hiding this comment.
I am using Windows 11 23H2 Build 22631.3447 with AMD graphics card. On my Windows 10 machine, it also appeared. Thus, I thought, it would be reproducible.
(Need to leave now. I will fire up a VM on Azure tonight and try to reproduce it there. In parallel, I asked JabRef developers to test and post their setup here. Maybe, we can narrow down the setups somehow.)
There was a problem hiding this comment.
It is important to switch on/off the deepl app in the residential area (system tray, lower right corner). In jabref, I can even recover from the crash by closing the deepl app in the system tray, then getting back to it by opening the app again. Pretty funny.
There was a problem hiding this comment.
Minor comment: -1 and Integer.MAX_VALUE both seem to be reasonable values for an unlimited length. Maybe, the accessibility developers of Microsoft like the latter more.
The code part start + maxLength could be changed to Math.max(maxLength, start + maxLength). Then any value around Integer.MAX_VALUE would work.
IMHO WinTextProvider#GetText should also be able to handle Integer.MAX_VALUE-2 - the caller cannot know the values of start used, because it does not pass start and should not track state of the called instance of WinTextProvider.
There was a problem hiding this comment.
My proposal is in 5f2a296 (#2)
- int endOffset = maxLength != -1 ? Math.min(end, start + maxLength) : end;
+ int endOffset = maxLength >= 0 ? Math.min(end, Math.max(start + maxLength, maxLength)) : end;There was a problem hiding this comment.
@jperedadnr The above change leads to a working JabRef. (Thank you for sharing your thoughts with Integer.MAX_VALUE.) Now, how to get this into JFX?
There was a problem hiding this comment.
I just filed openjdk#1442. If you came up with a better solution, I just withdraw. Just wanted to share it more visible (also (hopefully) to reduce load from you).
There was a problem hiding this comment.
Great, thanks, now let's see what reviewers say about it.
While I guess that your fix is as good as what I also suggested, if you follow my comments in the JBS issue, it might be that the change should be needed in the native side after all, discarding this MAX_VALUE for instance, before forwarding the call to (java) GetText.
As in: why do we have accessibility calls for something we don't expect?
|
Reproducers
|
|
yes: Edition Windows 10 Pro 22H2, build 19045.4291 |
|
Windows 11 23H2 (build 22631.3447)
|
That finally helped... |
|
yes: Win11, 23H2, version=22631.3447 |
|
I contacted DeepL regarding that issue. I have support "Request no. 405861". |
# Conflicts: # modules/javafx.graphics/src/main/java/com/sun/glass/ui/win/WinTextRangeProvider.java
