Skip to content

Cleanup Paragraph class after having covered it with UT #1311

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.

Sign up for GitHub

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

Open
Symeon94 wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Cleanup Paragraph class after having covered it with UT #1311

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2ba8bc4
Cleaning concat
Dec 29, 2025
b5ff9af
Test
Dec 29, 2025
f73a993
Cleaning up trim
Dec 29, 2025
cd28c1f
Cleanup subsequence
Dec 29, 2025
13ea95a
Restyle cleanup
Dec 29, 2025
4ea3ccc
Add a test
Dec 29, 2025
d243d88
More tests
Dec 29, 2025
82d49bb
Last minute cleanup
Dec 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
134 changes: 74 additions & 60 deletions richtextfx/src/main/java/org/fxmisc/richtext/model/Paragraph.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,43 +195,55 @@ public Paragraph<PS, SEG, S> concat(Paragraph<PS, SEG, S> p) {
if(p.length() == 0) {
return this;
}

if(length() == 0) {
return p;
}
List<SEG> concatSegments = concatSegmentsOf(p);
StyleSpans<S> concatStyles = concatStylesOf(p);
return new Paragraph<>(paragraphStyle, segmentOps, concatSegments, concatStyles);
}

List<SEG> updatedSegs;
SEG leftSeg = segments.get(segments.size() - 1);
SEG rightSeg = p.segments.get(0);
Optional<SEG> joined = segmentOps.joinSeg(leftSeg, rightSeg);
if(joined.isPresent()) {
SEG segment = joined.get();
updatedSegs = new ArrayList<>(segments.size() + p.segments.size() - 1);
updatedSegs.addAll(segments.subList(0, segments.size()-1));
updatedSegs.add(segment);
updatedSegs.addAll(p.segments.subList(1, p.segments.size()));
} else {
updatedSegs = new ArrayList<>(segments.size() + p.segments.size());
updatedSegs.addAll(segments);
updatedSegs.addAll(p.segments);
}

StyleSpans<S> updatedStyles;
private StyleSpans<S> concatStylesOf(Paragraph<PS, SEG, S> p) {
StyleSpan<S> leftSpan = styles.getStyleSpan(styles.getSpanCount() - 1);
StyleSpan<S> rightSpan = p.styles.getStyleSpan(0);
Optional<S> merge = segmentOps.joinStyle(leftSpan.getStyle(), rightSpan.getStyle());
if (merge.isPresent()) {
int startOfMerge = styles.position(styles.getSpanCount() - 1, 0).toOffset();
StyleSpans<S> updatedLeftSpan = styles.subView(0, startOfMerge);
int endOfMerge = p.styles.position(1, 0).toOffset();
StyleSpans<S> updatedRightSpan = p.styles.subView(endOfMerge, p.styles.length());
updatedStyles = updatedLeftSpan
.append(merge.get(), leftSpan.getLength() + rightSpan.getLength())
Optional<S> mergedStyle = segmentOps.joinStyle(leftSpan.getStyle(), rightSpan.getStyle());
if (mergedStyle.isPresent()) {
StyleSpans<S> updatedLeftSpan = getSubStyle(0, styles.getSpanCount() - 1);
StyleSpans<S> updatedRightSpan = getSubStyle(1, styles.getSpanCount());
return updatedLeftSpan
.append(mergedStyle.get(), leftSpan.getLength() + rightSpan.getLength())
.concat(updatedRightSpan);
}
return styles.concat(p.styles);
}

private StyleSpans<S> getSubStyle(int from, int to) {
int start = styles.position(from, 0).toOffset();
int end = styles.position(to, 0).toOffset();
return styles.subView(start, end);
}

private List<SEG> concatSegmentsOf(Paragraph<PS, SEG, S> par) {
List<SEG> updatedSegments;
Optional<SEG> joined = concatLastSegmentWith(par);
if(joined.isPresent()) {
SEG segment = joined.get();
updatedSegments = new ArrayList<>(segments.size() + par.segments.size() - 1); // -2 + 1
updatedSegments.addAll(segments.subList(0, segments.size()-1));
updatedSegments.add(segment);
updatedSegments.addAll(par.segments.subList(1, par.segments.size()));
} else {
updatedStyles = styles.concat(p.styles);
updatedSegments = new ArrayList<>(segments.size() + par.segments.size());
updatedSegments.addAll(segments);
updatedSegments.addAll(par.segments);
}
return new Paragraph<>(paragraphStyle, segmentOps, updatedSegs, updatedStyles);
return updatedSegments;
}

private Optional<SEG> concatLastSegmentWith(Paragraph<PS, SEG, S> par) {
SEG leftLastSegment = segments.get(segments.size() - 1);
SEG rightFirstSegment = par.segments.get(0);
return segmentOps.joinSeg(leftLastSegment, rightFirstSegment);
}

/**
Expand All @@ -248,20 +260,13 @@ public Paragraph<PS, SEG, S> subSequence(int start, int end) {
return trim(end).subSequence(start);
}

public Paragraph<PS, SEG, S> trim(int length) {
if(length >= length()) {
return this;
} else {
Position pos = navigator.offsetToPosition(length, Backward);
int segIdx = pos.getMajor();
List<SEG> segs = new ArrayList<>(segIdx + 1);
segs.addAll(segments.subList(0, segIdx));
segs.add(segmentOps.subSequence(segments.get(segIdx), 0, pos.getMinor()));
if (segs.isEmpty()) {
segs.add(segmentOps.createEmptySeg());
}
return new Paragraph<>(paragraphStyle, segmentOps, segs, styles.subView(0, length));
public Paragraph<PS, SEG, S> trim(int end) {
end = Math.max(0, end);
Copy link
Copy Markdown
Collaborator Author

@Symeon94 Symeon94 Dec 29, 2025

Choose a reason for hiding this comment

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

This is a behaviour change (you can see the adaptation in the test for it), any trim with negative number should equal to trimming with 0 (it was already the case for some negative values, but not all).

if(end < length()) {
List<SEG> segments = trimSegmentsTo(navigator.offsetToPosition(end, Backward));
return new Paragraph<>(paragraphStyle, segmentOps, segments, styles.subView(0, end));
}
return this;
}

public Paragraph<PS, SEG, S> subSequence(int start) {
Expand All @@ -274,18 +279,27 @@ public Paragraph<PS, SEG, S> subSequence(int start) {
// to use the left ops' default empty seg, not the right one's empty seg
return new Paragraph<>(paragraphStyle, segmentOps, segmentOps.createEmptySeg(), styles.subView(start,start));
} else if(start < length()) {
Position pos = navigator.offsetToPosition(start, Forward);
int segIdx = pos.getMajor();
List<SEG> segs = new ArrayList<>(segments.size() - segIdx);
segs.add(segmentOps.subSequence(segments.get(segIdx), pos.getMinor()));
segs.addAll(segments.subList(segIdx + 1, segments.size()));
if (segs.isEmpty()) {
segs.add(segmentOps.createEmptySeg());
}
return new Paragraph<>(paragraphStyle, segmentOps, segs, styles.subView(start, styles.length()));
} else {
throw new IndexOutOfBoundsException(start + " not in [0, " + length() + "]");
Position position = navigator.offsetToPosition(start, Forward);
List<SEG> segments = trimSegmentsFrom(position);
return new Paragraph<>(paragraphStyle, segmentOps, segments, styles.subView(start, styles.length()));
}
throw new IndexOutOfBoundsException(start + " not in [0, " + length() + "]");
}

private List<SEG> trimSegmentsTo(Position position) {
int index = position.getMajor();
List<SEG> segments = new ArrayList<>(index + 1);
segments.addAll(this.segments.subList(0, index));
segments.add(segmentOps.subSequence(this.segments.get(index), 0, position.getMinor()));
return segments;
}

private List<SEG> trimSegmentsFrom(Position position) {
int index = position.getMajor();
List<SEG> segments = new ArrayList<>(this.segments.size() - index);
segments.add(segmentOps.subSequence(this.segments.get(index), position.getMinor()));
segments.addAll(this.segments.subList(index + 1, this.segments.size()));
return segments;
}

public Paragraph<PS, SEG, S> delete(int start, int end) {
Expand All @@ -306,14 +320,13 @@ public Paragraph<PS, SEG, S> restyle(S style) {
}

public Paragraph<PS, SEG, S> restyle(int from, int to, S style) {
if(from >= length()) {
return this;
} else {
if(from < length()) {
StyleSpans<S> left = styles.subView(0, from);
StyleSpans<S> right = styles.subView(to, length());
StyleSpans<S> updatedStyles = left.append(style, to - from).concat(right);
return new Paragraph<>(paragraphStyle, segmentOps, segments, updatedStyles);
}
return this;
}

public Paragraph<PS, SEG, S> restyle(int from, StyleSpans<? extends S> styleSpans) {
Expand All @@ -324,14 +337,15 @@ public Paragraph<PS, SEG, S> restyle(int from, StyleSpans<? extends S> styleSpan
if(length() == 0) {
return new Paragraph<>(paragraphStyle, segmentOps, segments, (StyleSpans<S>) styleSpans);
}
StyleSpans<S> updatedStyles = restyleExistingFrom(from, styleSpans);
return new Paragraph<>(paragraphStyle, segmentOps, segments, updatedStyles);
}

private StyleSpans<S> restyleExistingFrom(int from, StyleSpans<? extends S> styleSpans) {
int len = styleSpans.length();
StyleSpans<S> left = styles.subView(0, from);
StyleSpans<S> right = styles.subView(from + len, length());

// type issue with concat
StyleSpans<S> castedSpans = (StyleSpans<S>) styleSpans;
StyleSpans<S> updatedStyles = left.concat(castedSpans).concat(right);
return new Paragraph<>(paragraphStyle, segmentOps, segments, updatedStyles);
return left.concat((StyleSpans<S>) styleSpans).concat(right);
}

/**
Expand Down
34 changes: 30 additions & 4 deletions richtextfx/src/test/java/org/fxmisc/richtext/model/ParagraphTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

import javafx.scene.control.IndexRange;

import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;

public class ParagraphTest {
Expand All @@ -20,9 +21,9 @@ private <T> void checkStyle(Paragraph<?, ?, T> paragraph, int length, T[] styles
assertEquals(ranges.length/2, styleSpans.getSpanCount(), "Style segment count invalid");
for (int i = 0; i < ranges.length/2 ; i++) {
StyleSpan<T> style = styleSpans.getStyleSpan(i);
assertEquals(styles[i], style.getStyle(), "Incorrect style for " + i);
assertEquals(ranges[i*2], style.getStart(), "Start not matching for " + i);
assertEquals(ranges[i*2 + 1] - ranges[i*2], style.getLength(), "Length not matching for " + i);
assertEquals(styles[i], style.getStyle(), "Incorrect style for " + i);
}
}

Expand Down Expand Up @@ -155,10 +156,21 @@ public void multiStyleParagraphReturnsCorrect_subSequenceOfLength() {
}

@Test
@DisplayName("Trim on empty text should return empty")
public void trimOnEmpty() {
Paragraph<Void, String, Void> p1 = createTextParagraph(SegmentOps.styledTextOps(), "");
assertEquals("", p1.trim(-1).getText());
assertEquals("", p1.trim(0).getText());
assertEquals("", p1.trim(1).getText());
assertEquals("", p1.trim(Integer.MAX_VALUE).getText());
}

@Test
@DisplayName("Trim text should return the text until the provided position")
public void trimParagraph() {
Paragraph<Void, String, String> p1 = createTextParagraph(SegmentOps.styledTextOps(), "Alpha", "MyStyle");
// Not very consistent that MIN_VALUE is throwing an exception while other negative numbers work
assertThrows(StringIndexOutOfBoundsException.class, () -> p1.trim(Integer.MIN_VALUE).getText());
assertEquals("", p1.trim(Integer.MIN_VALUE).getText());
assertEquals("", p1.trim(-10).getText());
assertEquals("", p1.trim(-1).getText());
assertEquals("Alpha", p1.trim(Integer.MAX_VALUE).getText());
Expand Down Expand Up @@ -186,7 +198,7 @@ public void deletePartOfParagraph() {
assertThrows(IndexOutOfBoundsException.class, () -> paragraph.delete(4, 10).getText()); // Not consistent with -1
assertEquals("gated", paragraph.delete(0, 4).getText());
assertEquals("gated", paragraph.delete(-1, 4).getText());
assertThrows(StringIndexOutOfBoundsException.class, () -> paragraph.delete(Integer.MIN_VALUE, 4).getText()); // Not very consistent with -1
assertEquals("gated", paragraph.delete(Integer.MIN_VALUE, 4).getText());

// Check style too
Paragraph<Void, String, String> p2 = paragraph.delete(2, 5);
Expand Down Expand Up @@ -232,6 +244,10 @@ public void multipleStyle() {
Paragraph<Void, String, String> p1 = createTextParagraph("To be or not to be", "text");
checkStyle(p1, 18, new String[] {"text"}, 0, 18);

// Restyle with same style
Paragraph<Void, String, String> p1b = p1.restyle(0, 18, "text");
checkStyle(p1b, 18, new String[] {"text"}, 0, 18);

// P1 is immutable, its style hasn't changed, but P2 has now three styles
Paragraph<Void, String, String> p2 = p1.restyle(9, 12, "keyword");
checkStyle(p1, 18, new String[] {"text"}, 0, 18);
Expand All @@ -241,8 +257,18 @@ public void multipleStyle() {
Paragraph<Void, String, String> p3 = p2.restyle(3, 10, "unknown");
checkStyle(p3, 18, new String[] {"text", "unknown", "keyword", "text"}, 0, 3, 3, 10, 10, 12, 12, 18);

// Restyle up to the end
checkStyle(p3.restyle(11, 17, "out"), 18,
new String[] {"text", "unknown", "keyword", "out", "text"},
0, 3, 3, 10, 10, 11, 11, 17, 17, 18);

// Restyle up to the end
// Bug
checkStyle(p3.restyle(11, 18, "out"), 18,
Copy link
Copy Markdown
Collaborator Author

@Symeon94 Symeon94 Dec 29, 2025

Choose a reason for hiding this comment

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

I thought the bug was with out-of-bound indexes, but it is actually a bug when you go up to the length. I created #1310 to handle that one.

new String[] {"text", "unknown", "keyword", "out"},
0, 3, 3, 10, 0, 1, 0, 7);

// Restyle out of bound
// Bug: the styles are totally off
checkStyle(p3.restyle(11, 19, "out"), 19,
new String[] {"text", "unknown", "keyword", "out"},
0, 3, 3, 10, 0, 1, 0, 8);
Expand Down