Improve layout performance of folded paragraphs by Col-E · Pull Request #1318 · FXMisc/RichTextFX

Col-E · 2026-07-17T10:29:18Z

Addressing #1317 - Description + video in the linked issue.

Symeon94 · 2026-07-17T13:10:48Z

+    private int getVirtualParagraphIndex(int allParagraphIndex) {
+        int virtualIndex = virtualFlowParagraphs.getViewIndex(allParagraphIndex);
+        if ( virtualIndex < 0 ) {
+            throw new IllegalArgumentException("Paragraph " + allParagraphIndex + " is folded");


It's a bit radical to throw an exception in that case, ins't there a better way?

Its what I saw a number of other places doing for negative values (like in Paragraph for negative positions).

I don't know if there's a great alternative. Clamping to 0 seems like it could lead to silent unintended behavior. Using Optional seems like it would just kick the issue to the call site plus is pointless object allocation / wrapping. In some places defaulting to 0 may work, but in others it may be wrong.

Also this method is used by getCell(int index) which is effectively @NonNull. We have to provide some valid value here that maps to a realized cell.

I can make visibleParToAllParIndex and allParToVisibleParIndex not throw, still don't think I can here.

I was wrong on this one, negative index is an invalid input, and it is right to throw it there and elsewhere. You cant ignore my comment (it's like indexing a non-existing index in an array, it's not allowed).

Symeon94 · 2026-07-17T14:31:00Z

Lots of public methods were modified but not tested (for instance lineIndex, but that is not the only one).

Pushed up a number of additional tests for folding, lmk if there's anything I missed

Your change touches a lot of some central class, so I hope you'll forgive me for insisting on this test part. I have listed many methods which are impacted and which it would be proper to test the behaviour in the circumstances of a fold.
Here is the list I have (I removed those methods which I have seen covered already):

removeSelection(Selection)

hit(double, double)

lineIndex(int, int)

getCharacterBoundsOnScreen(int, int)

getParagraphBoundsOnScreen(int)

getCaretBoundsOnScreen(T)

showParagraphInViewport(int)

showParagraphAtBottom(int)

showParagraphRegion(int, Bounds)

showParagraphAtCenter(int)

getCurrentLineStartInParargraph()

getCurrentLineEndInParargraph()

caretSelectionBind.paragraphIndexProperty().addListener( this::skipOverFoldedParagraphs ); (this one is impacted, but it's through private method).

double computePrefHeight(double) (that's a protected one)

updateIndex(int)

next/previous lines move is impacted by private method changes

getSelectionBoundsOnScreen is used in defining the bounds, not sure it can be tested.

Not sure all of them can be easily tested, but some are simpler to cover.

Pushed up some additional tests, I believe everything is now covered directly or indirectly in cases like computePrefHeight and skipOverFoldedParagraphs

Method Covering test

removeSelection(Selection) MultipleCaretSelectionTests.removing_selection_spanning_folded_paragraph_works

hit(double, double) FoldedParagraphTests.hit_on_visible_paragraph_after_folded_paragraph_uses_source_index

lineIndex(int, int) FoldedParagraphTests.line_index_works_for_visible_paragraph_and_rejects_folded_paragraph

getCharacterBoundsOnScreen(int, int) FoldedParagraphTests.character_bounds_are_empty_for_folded_paragraph

getParagraphBoundsOnScreen(int) FoldedParagraphTests.paragraph_bounds_are_empty_for_folded_paragraph_and_present_for_visible_paragraph

getCaretBoundsOnScreen(T) FoldedParagraphTests.caret_bounds_are_empty_for_caret_in_folded_paragraph

showParagraphInViewport(int) FoldedParagraphTests.show_paragraph_in_viewport_resolves_a_folded_target_to_a_visible_paragraph

showParagraphAtBottom(int) ParagraphIndexMappingTests.showing_a_folded_paragraph_at_the_end_uses_the_last_visible_paragraph

showParagraphRegion(int, Bounds) FoldedParagraphTests.show_paragraph_region_resolves_a_folded_target_to_a_visible_paragraph

showParagraphAtCenter(int) FoldedParagraphTests.show_paragraph_at_center_resolves_a_folded_target_to_a_visible_paragraph

getCurrentLineStartInParargraph() FoldedParagraphTests.line_index_works_for_visible_paragraph_and_rejects_folded_paragraph

getCurrentLineEndInParargraph() FoldedParagraphTests.line_index_works_for_visible_paragraph_and_rejects_folded_paragraph

skipOverFoldedParagraphs listener FoldedParagraphTests.navigation_skips_folded_paragraphs_in_both_directions

computePrefHeight(double) FoldedParagraphTests.auto_height_excludes_folded_paragraphs

updateIndex(int) Indirectly covered by FoldedParagraphTests.hit_on_visible_paragraph_after_folded_paragraph_uses_source_index and the paragraph-index mapping tests

Next/previous over folded paragraphs FoldedParagraphTests.navigation_skips_folded_paragraphs_in_both_directions

getSelectionBoundsOnScreen Indirectly covered by MultipleCaretSelectionTests.removing_selection_spanning_folded_paragraph_works, via selection.getSelectionBounds()

Thanks for taking the time to implement additional tests.

I have a comment before reviewing the changes fully, it came to me while looking at the first test from the new test class. It caught my attention, for the new code is throwing an exception if area.linedIndex(1,0) is called on folded paragraph. It appeared to me that it could depart from the existing behaviour. I therefor tried to run the new test class on the existing code and 7 out of 11 new tests failed on existing code (I didn’t try the ones from the other classes). This likely means the modifications changed the library’s behaviour. Someone could, for instance, be calling a lineIndex on a folded paragraph and discover its code starts throwing exceptions around.

The purpose of the overall change was to fix the jitter issue due to performance, which is an internal implementation problem. The change shouldn’t be visible to the end users.

lineIndex relies on the real ParagraphBox since it calls into the ParagraphText (A real TextFlow with FX layout handling) to query for line wrapping. With the changes its not possible to do this since those scene nodes no longer get created while their containing paragraph is folded.

I don't think there will be a perfect "invisible" change here. However, I think that for this case rather than an exception throw, falling back to 0 for folded lines can be a reasonable expectation. The text isn't visible since its folded, so what line wrapping is there to be done, right?

Pushed up a change modeling this fallback handling and updated the first test in FoldedParagraphTests to show the non-throwing behavior.

I understand there might be areas where changes compared to previous behaviour could be acceptable and even unavoidable. For instance getParagraphBoundsOnScreen is present only if the cell is visible, and it seems that your change on purpose made the cell invisible. However, we should be transparent about what these changes are and why they are there. I’ll review the test and see if I see any additional point to raise.

Symeon94 · 2026-07-20T10:24:39Z

-                    "Visible paragraphs' last index is [%s] but visibleParIndex was [%s]",
-                    getVisibleParagraphs().size() - 1, visibleParIndex)
-            );
+        if (visibleParIndex < 0 || (visibleParIndex > 0 && visibleParIndex >= getVisibleParagraphs().size())) {


This condition look strange (you kept previous code though, so was there an issue before?):

0 for some reason isIsgnored (if there is no visible paragraph it will go through)

If 0 being ignored is an error, it can be simplified to visibleParIndex <0 || visibleParIndex >= getVisibleParagraphs().size()

It might be another issue to open (I need to look into it), no fix on this PR

Symeon94 · 2026-07-20T10:29:19Z

-                    getVisibleParagraphs().size() - 1, visibleParIndex)
-            );
+        if (visibleParIndex < 0 || (visibleParIndex > 0 && visibleParIndex >= getVisibleParagraphs().size())) {
+            return -1;


You are changing an existing behaviour: It used to throw an exception and now it isn't.

I think your change make sense and should be an improvement, but we cannot assume all implementation will agree.

Symeon94 · 2026-07-20T10:31:47Z

-        return getCell(paragraphIndex).getCurrentLineIndex(columnPosition);
+        return getCellIfVisible(paragraphIndex)
+                .map(c -> c.getNode().getCurrentLineIndex(columnPosition))
+                .orElse(0);


I suspect a change of behaviour again, if getCell used to throw an exception (which I suspect), your new implementation will return 0. If you really are out of bound on this one, I think you should indeed have IndexOutOfBounds (as there used to be).

Where there shouldn't be an exception is for cell that are collapsed (where you proposed to return the first line of the collapsed entry).

Also, I see something else: your UT if I recall test collapse on line 0, which is not good because it's also the default value here, you should probably cover a case != 0.

Symeon94 · 2026-07-20T11:06:37Z

+            int start = area.getAbsolutePosition(0, 0);
+            assertTrue(area.getCharacterBoundsOnScreen(start, start + 1).isPresent());
+            start = area.getAbsolutePosition(1, 0);
+            assertFalse(area.getCharacterBoundsOnScreen(start, start + 1).isPresent());
+            start = area.getAbsolutePosition(2, 0);
+            assertTrue(area.getCharacterBoundsOnScreen(start, start + 1).isPresent());


Why do you limit your check on the first char? Why not loop on values that you know are there or not
Here it's [5;8) that are not visible, hence just do this to cover all cases:

Suggested change

int start = area.getAbsolutePosition(0, 0);

assertTrue(area.getCharacterBoundsOnScreen(start, start + 1).isPresent());

start = area.getAbsolutePosition(1, 0);

assertFalse(area.getCharacterBoundsOnScreen(start, start + 1).isPresent());

start = area.getAbsolutePosition(2, 0);

assertTrue(area.getCharacterBoundsOnScreen(start, start + 1).isPresent());

for(int i = 0 ; i < 12 ; i++) {

assertEquals(i < 5 || i >= 8, area.getCharacterBoundsOnScreen(i, i+ 1).isPresent());

}

Symeon94 · 2026-07-20T11:12:17Z

+        // create a caret in the folded paragraph, which should not have bounds on screen.
+        CaretNode caret = new CaretNode("folded caret", area, area.getAbsolutePosition(1, 0));
+        interact(() -> {
+            assertFalse(area.getCaretBoundsOnScreen(caret).isPresent());


Are you sure the bounds must not be present? (previous behaviour returned true) The caret must be somewhere, I would expect that it would be at the end of the folded line or somewhere else.
Isn't there are risk that if moved the caret might dissapear when moving on the folded line?

Symeon94 · 2026-07-20T11:15:59Z

I have noticed a common pattern in all the tests you are making: they only cover fold on index = 0. I think you should reconsider the cases, as folding on index = 0 is an edge case. What happens when there is a line before the fold is never covered in these different cases present here.
Optimally we should test fold at the start, in the middle and at the end.

Symeon94 · 2026-07-20T11:20:18Z

+        });
+
+        // getting/creating graphics for the folded paragraph throws an exception, since it is not visible.
+        assertThrows(IllegalArgumentException.class, () -> area.getParagraphGraphic(1));


Previous behaviour was sending null when there was no graphic set (and it was sending null for folded). Why not send null instead of throwing an exception?

Symeon94 · 2026-07-20T11:21:18Z

+        assertThrows(IllegalArgumentException.class, () -> area.getParagraphGraphic(1));
+        assertThrows(IllegalArgumentException.class, () -> area.recreateParagraphGraphic(1));


you are not testing for 0 and 2?

Symeon94 · 2026-07-20T13:31:38Z

+             assertEquals(Optional.empty(), area.allParToVisibleParIndex(area.getParagraphs().size()));
+
+             assertEquals(-1, area.visibleParToAllParIndex(-1));
+             assertEquals(-1, area.visibleParToAllParIndex(Integer.MAX_VALUE));


Both threw IllegalArgumentException before, which made sort of sense, as these are not valid values.
Now it's behaving differently

Symeon94 · 2026-07-20T13:33:29Z

+        });
+
+        assertEquals(1, area.getParagraphLinesCount(0));
+        assertEquals(0, area.getParagraphLinesCount(1));


That's probably the most dangerous change, it used to return 1 and the method has no mention of visible index. If anyone does any logic calculation on this, it will change the outcome.

Symeon94 · 2026-07-20T13:37:13Z

I just finished reviewing, I wanted to point the method changes I have seen.

These have changed but should be ok (mostly they refer to visibility, so it's normal it would change), maybe it should be documented somewhere still:

getParagraphBoundsOnScreen
getVisibleParagraphBoundsOnScreen
getCharacterBoundsOnScreen
showParagraphInViewport

This one the change could be risky if the caller supposed that empty() is the end of the visible lines (now empty() could be folded):
allParToVisibleParIndex

These three I'm not sure it is advisable to change:

getCaretBoundsOnScreen -> Caret needs still to get bounds, even on folded
getParagraphGraphic -> Maybe return null instead of empty
getParagraphLinesCount -> I wrote a comment where I have seen it, the issue is that any existing logic using this one might have not considered that it was referring to visible lines (and there is no reference in the name that let us think that).

Col-E · 2026-07-20T14:51:06Z

I've been trying to leave comments but github is not playing nice... Its putting comments in the "review mode" where you bulk comment then approve, despite me not ever starting a review. Then it just orphans the comments from the threads I was replying to so now they're floating without any proper context 😫

Symeon94 · 2026-07-20T15:03:49Z

@Col-E I cannot answer on your last comment, so I put it here. I didn't create this library, so as far as exception is concerned I'm like you left to guess.
My original comment was on your new addition of exception. During review I saw how it was done elsewhere in this code and I thought a bit about it. It seems to me that it makes sense to throw exception for invalid indexing (like you would get from referencing a non-existing reference in an array). That would mean -1 and >= size/length. So your original exception made sense (and was aligned with the existing code). My apologies for commenting there.

Optional.empty() or -1 should be kept for valid input that returns non-existing.

What I'd like to avoid is to change existing behaviour if not absolutely necessary (that also means exceptions). As this might impact library users. For visible cells it is most of the time intrinsically related to your change. So, there changing the behaviour is mostly acceptable (I have commented on those where I thought it is either not as clear or wrong).

Improve layout performance of folded paragraphs

d1755a7

Symeon94 reviewed Jul 17, 2026

View reviewed changes

Comment thread richtextfx/src/main/java/org/fxmisc/richtext/GenericStyledArea.java

Jugen linked an issue Jul 17, 2026 that may be closed by this pull request

Scrolling past large collapsed paragraphs can lead to slight visual stutter #1317

Open

Symeon94 requested changes Jul 17, 2026

View reviewed changes

Folding cleanup based on PR feedback

2433000

Symeon94 reviewed Jul 18, 2026

View reviewed changes

Comment thread richtextfx/src/main/java/org/fxmisc/richtext/GenericStyledArea.java

Symeon94 reviewed Jul 18, 2026

View reviewed changes

Comment thread richtextfx/src/integrationTest/java/org/fxmisc/richtext/api/ParagraphIndexMappingTests.java Outdated

Symeon94 reviewed Jul 18, 2026

View reviewed changes

Comment thread richtextfx/src/integrationTest/java/org/fxmisc/richtext/api/ParagraphIndexMappingTests.java Outdated

Symeon94 reviewed Jul 18, 2026

View reviewed changes

Comment thread richtextfx/src/integrationTest/java/org/fxmisc/richtext/api/ParagraphIndexMappingTests.java Outdated

Additional test coverage for paragraph folding

de154f5

Col-E force-pushed the master branch from 28dcdd4 to de154f5 Compare July 19, 2026 07:41

For folded paragraphs make ViewActions.lineIndex yield 0

d94b622

Symeon94 reviewed Jul 20, 2026

View reviewed changes

Method	Covering test
`removeSelection(Selection)`	`MultipleCaretSelectionTests.removing_selection_spanning_folded_paragraph_works`
`hit(double, double)`	`FoldedParagraphTests.hit_on_visible_paragraph_after_folded_paragraph_uses_source_index`
`lineIndex(int, int)`	`FoldedParagraphTests.line_index_works_for_visible_paragraph_and_rejects_folded_paragraph`
`getCharacterBoundsOnScreen(int, int)`	`FoldedParagraphTests.character_bounds_are_empty_for_folded_paragraph`
`getParagraphBoundsOnScreen(int)`	`FoldedParagraphTests.paragraph_bounds_are_empty_for_folded_paragraph_and_present_for_visible_paragraph`
`getCaretBoundsOnScreen(T)`	`FoldedParagraphTests.caret_bounds_are_empty_for_caret_in_folded_paragraph`
`showParagraphInViewport(int)`	`FoldedParagraphTests.show_paragraph_in_viewport_resolves_a_folded_target_to_a_visible_paragraph`
`showParagraphAtBottom(int)`	`ParagraphIndexMappingTests.showing_a_folded_paragraph_at_the_end_uses_the_last_visible_paragraph`
`showParagraphRegion(int, Bounds)`	`FoldedParagraphTests.show_paragraph_region_resolves_a_folded_target_to_a_visible_paragraph`
`showParagraphAtCenter(int)`	`FoldedParagraphTests.show_paragraph_at_center_resolves_a_folded_target_to_a_visible_paragraph`
`getCurrentLineStartInParargraph()`	`FoldedParagraphTests.line_index_works_for_visible_paragraph_and_rejects_folded_paragraph`
`getCurrentLineEndInParargraph()`	`FoldedParagraphTests.line_index_works_for_visible_paragraph_and_rejects_folded_paragraph`
`skipOverFoldedParagraphs` listener	`FoldedParagraphTests.navigation_skips_folded_paragraphs_in_both_directions`
`computePrefHeight(double)`	`FoldedParagraphTests.auto_height_excludes_folded_paragraphs`
`updateIndex(int)`	Indirectly covered by `FoldedParagraphTests.hit_on_visible_paragraph_after_folded_paragraph_uses_source_index` and the paragraph-index mapping tests
Next/previous over folded paragraphs	`FoldedParagraphTests.navigation_skips_folded_paragraphs_in_both_directions`
`getSelectionBoundsOnScreen`	Indirectly covered by `MultipleCaretSelectionTests.removing_selection_spanning_folded_paragraph_works`, via `selection.getSelectionBounds()`

		assertThrows(IllegalArgumentException.class, () -> area.getParagraphGraphic(1));
		assertThrows(IllegalArgumentException.class, () -> area.recreateParagraphGraphic(1));

Uh oh!

Conversation

Col-E commented Jul 17, 2026

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Col-E Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Symeon94 Jul 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Col-E Jul 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Col-E Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Symeon94 commented Jul 20, 2026

Uh oh!

Col-E commented Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Symeon94 commented Jul 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Col-E Jul 17, 2026 •

edited

Loading

Symeon94 Jul 18, 2026 •

edited

Loading

Col-E Jul 19, 2026 •

edited

Loading

Col-E Jul 20, 2026 •

edited

Loading

Col-E commented Jul 20, 2026 •

edited

Loading