Skip to content

8357393: RichTextArea: fails to properly save text attributes #1813

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
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

8357393: RichTextArea: fails to properly save text attributes #1813

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7669815
fixed attribute serialization
andy-goryachev-oracle May 20, 2025
0173357
tests
andy-goryachev-oracle May 20, 2025
d0551e4
test
andy-goryachev-oracle May 21, 2025
1d2625e
Merge remote-tracking branch 'origin/master' into 8357393.attr.ser
andy-goryachev-oracle May 21, 2025
06b3181
javadoc
andy-goryachev-oracle May 21, 2025
147502b
Merge remote-tracking branch 'origin/master' into 8357393.attr.ser
andy-goryachev-oracle May 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
27 changes: 23 additions & 4 deletions ...t/src/main/java/com/sun/jfx/incubator/scene/control/richtext/StyleAttributeMapHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2024, 2025, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -52,20 +52,39 @@ public static void setAccessor(Accessor a) {
/**
* Returns a new StyleAttributeMap instance which contains only the character attributes,
* or null if no character attributes found.
*
* @param ss the style attribute map
* @return the instance
* @return the instance of StyleAttributeMap, or null
Copy link
Member

Choose a reason for hiding this comment

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

Is there a good reason to allow null here? Unless there is a semantic difference between null and the empty map, it might be easier to make this non-nullable. This could be done later, since the fact that it can return null is preexisting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've recorded this in the feedback document - this might be worth doing.

*/
public static StyleAttributeMap getCharacterAttrs(StyleAttributeMap ss) {
return accessor.filterAttributes(ss, false);
}

/**
* Returns a new StyleAttributeMap instance which contains only the paragraph attributes.,
* Returns a new StyleAttributeMap instance which contains only the paragraph attributes,
* or null if no paragraph attributes found.
*
* @param ss the style attribute map
* @return the instance
* @return the instance of StyleAttributeMap, or null
*/
public static StyleAttributeMap getParagraphAttrs(StyleAttributeMap ss) {
return accessor.filterAttributes(ss, true);
}

/**
* Returns a new StyleAttributeMap instance which contains only paragraph attributes
* when {@code forParagraph=true}, or character attributes when {@code forParagraph=false},
* or null when no attributes of the specified type are found.
*
* @param ss the style attribute map
* @param forParagraph determines which attributes to retain
* @return the instance of StyleAttributeMap, or null
*/
public static StyleAttributeMap filter(StyleAttributeMap ss, boolean forParagraph) {
if (forParagraph) {
return StyleAttributeMapHelper.getParagraphAttrs(ss);
} else {
return StyleAttributeMapHelper.getCharacterAttrs(ss);
}
}
}
35 changes: 21 additions & 14 deletions ...htext/src/main/java/jfx/incubator/scene/control/richtext/model/RichTextFormatHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2023, 2024, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2023, 2025, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -43,6 +43,7 @@
import javafx.util.converter.DoubleStringConverter;
import com.sun.jfx.incubator.scene.control.richtext.Converters;
import com.sun.jfx.incubator.scene.control.richtext.RichTextFormatHandlerHelper;
import com.sun.jfx.incubator.scene.control.richtext.StyleAttributeMapHelper;
import jfx.incubator.scene.control.richtext.StyleResolver;
import jfx.incubator.scene.control.richtext.TextPos;

Expand All @@ -55,8 +56,8 @@
* PARAGRAPH[]
*
* PARAGRAPH: {
* PARAGRAPH_ATTRIBUTE[]*,
* TEXT_SEGMENT[],
* PARAGRAPH_ATTRIBUTE[]*,
* "\n"
* }
*
Expand All @@ -80,10 +81,14 @@
* }
*
* TEXT_SEGMENT: {
* ATTRIBUTE[]*
* ATTRIBUTE[]* or {}
* (text string with escaped special characters)
* }
* </pre>
*
* A special token <code>{}</code> denotes an empty character attribute map,
* while <code>{!}</code> does the same for an empty paragraph attribute map.
* <p>
* Attribute sequences are further deduplicated, using a single {number} token
* which specifies the index into the list of unique sets of attributes.
* Paragraph attribute sets are treated as separate from the segment attrubite sets.
Expand Down Expand Up @@ -303,6 +308,7 @@ public void consume(StyledSegment seg) throws IOException {
}

private void emitAttributes(StyleAttributeMap attrs, boolean forParagraph) throws IOException {
attrs = StyleAttributeMapHelper.filter(attrs, forParagraph);
if ((attrs != null) && (!attrs.isEmpty())) {
Integer num = styles.get(attrs);
if (num == null) {
Expand Down Expand Up @@ -357,9 +363,14 @@ public int compare(StyleAttribute<?> a, StyleAttribute<?> b) {
wr.write(String.valueOf(num));
wr.write('}');
}
} else if (forParagraph) {
// this special token clears the paragraph attributes
wr.write("{!}");
} else {
if (forParagraph) {
// this special token clears the paragraph attributes
wr.write("{!}");
} else {
// special token indicates the next text segment
wr.write("{}");
}
}
}

Expand Down Expand Up @@ -462,7 +473,7 @@ public StyledSegment nextSegment() {
String text = decodeText();
return StyledSegment.of(text);
} catch (IOException e) {
err(e);
log(e);
return null;
}
}
Expand Down Expand Up @@ -498,13 +509,9 @@ private StyleAttributeMap parseAttributes(boolean forParagraph) throws IOExcepti
}
String s = text.substring(index, ix);
if (s.length() == 0) {
if (forParagraph) {
index = ix + 1;
// special token clears paragraph attributes
return StyleAttributeMap.EMPTY;
} else {
throw err("empty attribute name");
}
index = ix + 1;
// either {} or {!} clears signifies empty attributes
return StyleAttributeMap.EMPTY;
}
int n = parseStyleNumber(s);
if (n < 0) {
Expand Down
2 changes: 1 addition & 1 deletion ....richtext/src/main/java/jfx/incubator/scene/control/richtext/model/StyleAttributeMap.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,7 +428,7 @@ private static void initAccessor() {
StyleAttributeMapHelper.setAccessor(new StyleAttributeMapHelper.Accessor() {
@Override
public StyleAttributeMap filterAttributes(StyleAttributeMap ss, boolean isParagraph) {
return ss.filterAttributes(isParagraph);
return (ss == null ? null : ss.filterAttributes(isParagraph));
Copy link
Member

Choose a reason for hiding this comment

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

You could skip the null check if you disallowed a null map.

}
});
}
Expand Down
4 changes: 2 additions & 2 deletions ...or.richtext/src/test/java/test/jfx/incubator/scene/control/richtext/RichTextAreaTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,7 @@ public void copyDataFormat() {
String s = Clipboard.getSystemClipboard().getString();
assertEquals(null, s);
Object v = Clipboard.getSystemClipboard().getContent(fmt);
assertEquals("a{!}", v);
assertEquals("{}a{!}", v);
}

@Test
Expand Down Expand Up @@ -665,7 +665,7 @@ public void write() throws Exception {
ByteArrayOutputStream out = new ByteArrayOutputStream();
control.write(out);
byte[] b = out.toByteArray();
assertEquals("1 {b}bold{!}", new String(b, StandardCharsets.US_ASCII));
assertEquals("{}1 {b}bold{!}", new String(b, StandardCharsets.US_ASCII));
}

@Test
Expand Down
163 changes: 96 additions & 67 deletions .../test/java/test/jfx/incubator/scene/control/richtext/model/TestRichTextFormatHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2023, 2024, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2023, 2025, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -28,7 +28,6 @@
import java.io.IOException;
import java.io.StringWriter;
import java.util.ArrayList;
import java.util.List;
import javafx.scene.paint.Color;
import javafx.scene.text.TextAlignment;
import org.junit.jupiter.api.Assertions;
Expand All @@ -46,50 +45,65 @@
* Tests RichTextFormatHandler.
*/
public class TestRichTextFormatHandler {
private static final boolean DEBUG = true;
private static boolean DEBUG = false;

@Test
public void testRoundTrip() throws IOException {
Object[] ss = {
List.of(
p(
a(StyleAttributeMap.BACKGROUND, Color.RED),
a(StyleAttributeMap.BULLET, "⌘"),
a(StyleAttributeMap.FIRST_LINE_INDENT, 10.0),
a(StyleAttributeMap.LINE_SPACING, 11.0),
a(StyleAttributeMap.PARAGRAPH_DIRECTION, ParagraphDirection.RIGHT_TO_LEFT)
),
s("bold", StyleAttributeMap.BOLD),
s("font family", a(StyleAttributeMap.FONT_FAMILY, "Arial")),
s("font size", a(StyleAttributeMap.FONT_SIZE, 12.0)),
s("italic", StyleAttributeMap.ITALIC),
nl(),

p(
a(StyleAttributeMap.SPACE_ABOVE, 13.0),
a(StyleAttributeMap.SPACE_BELOW, 14.0),
a(StyleAttributeMap.SPACE_LEFT, 15.0),
a(StyleAttributeMap.SPACE_RIGHT, 16.0),
a(StyleAttributeMap.TEXT_ALIGNMENT, TextAlignment.CENTER),
a(StyleAttributeMap.PARAGRAPH_DIRECTION, ParagraphDirection.LEFT_TO_RIGHT)
),
s("strike through", StyleAttributeMap.STRIKE_THROUGH),
s("text color", a(StyleAttributeMap.TEXT_COLOR, Color.GREEN)),
s("underline", StyleAttributeMap.UNDERLINE),
nl(),

s("combined", StyleAttributeMap.ITALIC, a(StyleAttributeMap.TEXT_COLOR, Color.RED), StyleAttributeMap.UNDERLINE),
nl()

// TODO test escapes in text, attribute names, attribute values
)
};
public void testBasicAttributes() throws IOException {
testRoundTrip(
s("bold", StyleAttributeMap.BOLD),
s("font family", a(StyleAttributeMap.FONT_FAMILY, "Arial")),
s("font size", a(StyleAttributeMap.FONT_SIZE, 12.0)),
s("italic", StyleAttributeMap.ITALIC),
p(
a(StyleAttributeMap.BACKGROUND, Color.RED),
a(StyleAttributeMap.BULLET, "⌘"),
a(StyleAttributeMap.FIRST_LINE_INDENT, 10.0),
a(StyleAttributeMap.LINE_SPACING, 11.0),
a(StyleAttributeMap.PARAGRAPH_DIRECTION, ParagraphDirection.RIGHT_TO_LEFT)
),
nl(),

RichTextFormatHandler handler = RichTextFormatHandler.getInstance();
s("strike through", StyleAttributeMap.STRIKE_THROUGH),
s("text color", a(StyleAttributeMap.TEXT_COLOR, Color.GREEN)),
s("underline", StyleAttributeMap.UNDERLINE),
p(
a(StyleAttributeMap.SPACE_ABOVE, 13.0),
a(StyleAttributeMap.SPACE_BELOW, 14.0),
a(StyleAttributeMap.SPACE_LEFT, 15.0),
a(StyleAttributeMap.SPACE_RIGHT, 16.0),
a(StyleAttributeMap.TEXT_ALIGNMENT, TextAlignment.CENTER),
a(StyleAttributeMap.PARAGRAPH_DIRECTION, ParagraphDirection.LEFT_TO_RIGHT)
),
nl(),

for (Object x : ss) {
testRoundTrip(handler, (List<StyledSegment>)x);
}
s("combined", StyleAttributeMap.ITALIC, a(StyleAttributeMap.TEXT_COLOR, Color.RED), StyleAttributeMap.UNDERLINE),
nl()
);
}

// JDK-8357393
@Test
public void testEmptyCharAttributeToken() throws IOException {
testRoundTrip(
s("normal"),
s("BOLD", StyleAttributeMap.BOLD),
s("normal")
);
}

@Test
public void testEmptyParagraphAttributeToken() throws IOException {
testRoundTrip(
s("normal"),
p()
);
}

@Test
public void testUnknownAttributes() throws IOException {
testReadWrite("{unknown}text{!UNKNOWN}\n", "{}text{}\n");
testReadWrite("{unknown}text{!UNKNOWN}{!alignment|R}\n", "{}text{!alignment|R}\n");
testReadWrite("{unknown}{b}text\n", "{b}text\n");
}

@Test
Expand Down Expand Up @@ -119,7 +133,7 @@ public void testEscapes() throws IOException {
out.consume(StyledSegment.of("{|%}"));
out.flush();
String s = wr.toString();
String expected = "%7B%7C%25%7D";
String expected = "{}%7B%7C%25%7D";
Assertions.assertEquals(expected, s);
}

Expand Down Expand Up @@ -171,7 +185,32 @@ private static StyledSegment nl() {
return StyledSegment.LINE_BREAK;
}

private void testRoundTrip(RichTextFormatHandler handler, List<StyledSegment> input) throws IOException {
private void testReadWrite(String input, String expected) throws IOException {
RichTextFormatHandler handler = RichTextFormatHandler.getInstance();
ArrayList<StyledSegment> segments = new ArrayList<>();

StyledInput in = handler.createStyledInput(input, null);
StyledSegment seg;
while ((seg = in.nextSegment()) != null) {
if (DEBUG) {
System.out.println(seg);
}
segments.add(seg);
}

StringWriter wr = new StringWriter();
StyledOutput out = RichTextFormatHandlerHelper.createStyledOutput(handler, null, wr);
for (StyledSegment s : segments) {
out.consume(s);
}
out.flush();

String result = wr.toString();
Assertions.assertEquals(expected, result);
}

private void testRoundTrip(StyledSegment ... input) throws IOException {
RichTextFormatHandler handler = RichTextFormatHandler.getInstance();
// export to string
int ct = 0;
StringWriter wr = new StringWriter();
Expand Down Expand Up @@ -201,13 +240,17 @@ private void testRoundTrip(RichTextFormatHandler handler, List<StyledSegment> in
}

// check segments for equality
Assertions.assertEquals(input.size(), segments.size());
for (int i = 0; i < input.size(); i++) {
StyledSegment is = input.get(i);
int sz = input.length;
Assertions.assertEquals(sz, segments.size());
for (int i = 0; i < sz; i++) {
StyledSegment is = input[i];
StyledSegment rs = segments.get(i);
Assertions.assertEquals(is.getType(), rs.getType());
Assertions.assertEquals(is.getText(), rs.getText());
Assertions.assertEquals(is.getStyleAttributeMap(null), rs.getStyleAttributeMap(null));
// empty and null attributes are equivalent for this test
StyleAttributeMap im = normalize(is.getStyleAttributeMap(null));
StyleAttributeMap rm = normalize(rs.getStyleAttributeMap(null));
Assertions.assertEquals(im, rm);
}

// export to a string again
Expand All @@ -226,26 +269,12 @@ private void testRoundTrip(RichTextFormatHandler handler, List<StyledSegment> in
Assertions.assertEquals(exported, result);
}

private void testRoundTrip_DELETE(RichTextFormatHandler handler, String text) throws IOException {
ArrayList<StyledSegment> segments = new ArrayList<>();

StyledInput in = handler.createStyledInput(text, null);
StyledSegment seg;
while ((seg = in.nextSegment()) != null) {
segments.add(seg);
if (DEBUG) {
System.out.println(seg);
private StyleAttributeMap normalize(StyleAttributeMap a) {
if (a != null) {
if (a.isEmpty()) {
return null;
}
}

StringWriter wr = new StringWriter();
StyledOutput out = RichTextFormatHandlerHelper.createStyledOutput(handler, null, wr);
for (StyledSegment s : segments) {
out.consume(s);
}
out.flush();

String result = wr.toString();
Assertions.assertEquals(text, result);
return a;
}
}
Loading