-
Notifications
You must be signed in to change notification settings - Fork 236
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
IndexOutOfBoundsException in RichText demo #449
Comments
@clartaq Does this also occur using the |
The same error occurs when using 0.7-M3, whether I paste the contents of the 0.6.10 README.md or the 0.7-M3 version. Also, using the new command in the demo to read a file produces an OutOfMemoryError when trying to load either of these files. |
I can't reproduce the bug on my end in either of those versions.
Could you create a new issue specifically for that and with more details as to the issue? I'm not sure which command to which you're referring. |
Can someone else test this on a Windows environment? |
I can confirm that bug, Windows 10 Enterprise, RichTextFX demo 0.6.10 Stacktrace:
Searching for
on Google shows that this is not an uncommon error... |
Also happens on Ubuntu. Environment:
|
Originally, I ran my test through my IDE which AFAIK delegates to Gradle, and never had the issue arise. The issue does appear on my end when I test it via a terminal. |
Ah, I never tried it through an IDE, just terminals. |
I think this just proves once again that tests (and demos when debugging them) should be run via terminal, not IDE. I'm sure the error occurs even when run via the IDE; the resulting window just probably didn't display it. |
Note: this issue also arises when |
I'm labeling this as both a JDK bug and a regular bug because, although it's caused by the first, we could still rewrite the code to get around it. |
I'm getting an occasional IOOB exception being reported using RichTextFX 0.8.2 and decided to look into this bug as part of my research. So as already noted this is both a JDK bug and a regular bug. The JDK bug only manifests when:
The exception is triggered by this line of code inside RichTextFX/richtextfx-demos/src/main/java/org/fxmisc/richtext/demo/richtext/RichText.java Line 251 in f5f1748
However this line of code being executed under these conditions is actually the regular bug. This is because it's only supposed to be executed if there is more than one font size (or family) in the selected text. Since the currently selected text has just been set to be a certain font size (or family) there should only be 1 font size (or family) style in the selection - but there isn't !? The reason for this is because the new font style doesn't get set on or applied to paragraph breaks (i.e. the empty line between paragraphs) due to empty lines being skipped. The relevant line of code primarily responsible for this is: RichTextFX/richtextfx/src/main/java/org/fxmisc/richtext/model/Paragraph.java Lines 321 to 322 in f5f1748
As can be seen the paragraph is returned as is when the length is zero. If a space is added to the paragraph break then the exception "goes away" because now the style is being applied to the paragraph break. The solution I think is to replace the above code with something like:
|
Your proposed solution does resolve the issue. I'm not sure what the implications are (if any) as to what other issues this might produce since I'm not sure what the entire call stack is. Mind submitting another PR for that? |
So one implication is that it will cause this test to fail: RichTextFX/richtextfx/src/test/java/org/fxmisc/richtext/model/ParagraphTest.java Lines 23 to 36 in f5f1748
specifically on line 35: assertEquals(p, restyledP);
Am I right in saying that if the test is to see whether an exception is thrown under these conditions then the assertion can be removed and the test will still be valid. If so then I can go ahead with the proposed change, modify the above test, and write another to check that styles are being applied to empty paragraphs. |
mm.... I think the assertion should still hold... If one uses ... then I think the paragraph's style does matter and will produce different results. |
Sorry I don't follow your reasoning here ? The proposed change is going to change what Bear in mind that the current behavior is actually incorrect in that if you have selected multiple paragraphs and change the font size for example, and then decide to insert something at one of the paragraph breaks using getTextStyleForInsertionAt then if I'm not mistaken you will get the incorrect style returned. The font size in this case will be what it was before the change and not what that section of the document currently is. At least that's how I understand it, or did you have something different in mind ? |
Mm... Those are good points. I don't think I fully understood the entirety of the problem, hence, my response. In response to your original question then, I think that would be a good modification to make. |
Closed by #685 |
When changing the font of the entire contents of the demo editor pane, an IndexOutOfBounds exception is thrown in the "JavaFX Application Thread".
Steps to reproduce:
ComboBox
] (edited by Jordan)Environment:
The text was updated successfully, but these errors were encountered: