On Mon, 15 Jun 2026 18:40:05 GMT, Andy Goryachev <[email protected]> wrote:
>> ### Summary >> >> This PR adds support for controlling tab stops: the `TAB_STOPS` paragraph >> attribute and the `defaultTabStops` property in the `RichTextModel`. >> >> While adding the paragraph attribute is a trivial process, adding a >> model-wide property requires adding of a mechanism to support document >> properties (something that was originally omitted in the first incubating >> release). Using the document properties, we can now persist not only the >> default tab stops, but also provide the version umber for the storage format >> provided by the `RichTextFormatHandler`, which will enable support the >> format evolution in the future [0]. >> >> To showcase the feature, the `RichEditorDemoApp` gains a visual ruler and >> additional dialogs and menus. >> >> <img width="822" height="381" alt="Screenshot 2026-03-04 at 14 00 30" >> src="https://github.com/user-attachments/assets/32251846-f11a-4e87-b74a-44c21d629550"; >> /> >> >> >> ### Paragraph Attribute >> >> Paragraph-specific tab stops are enabled by: >> >> - `StyleAttributeMap.TAB_STOPS` constant >> - `StyleAttributeMap.Builder.setTabStops(double ... positions)` >> >> The tab stops are represented by the `TabStops` class which is basically an >> immutable list (we can't use List<TabStops> because it makes it impossible >> to offer compile-time type safety of `StyleAttribute`). >> >> ### Default Tab Stops >> >> After the last paragraph tab stop, or when no paragraph tab stops is set, >> the document provides a way to set default tab stops via the model's >> `defaultTabStops` property: >> >> These changes support the new property and other document properties: >> >> - document-wide properties support in the `StyledTextModel` base class >> - `defaultTabStops` property in the `RichTextModel` and >> `RichTextFormatHandler` >> - document properties `VERSION` and `DEFAULT_TAB_STOPS` >> - `StyledSegment`: `ofDocumentProperties()` factory, >> `getDocumentProperties()` >> >> ### Other Improvements >> >> A number of other improvements were made along with the tab stop related >> changes: >> >> - `character()`, `paragraph()`, and `document()` factory methods in the >> `StyleAttribute` class >> - `isCharacterAttribute()`, `isParagraphAttribute()`, and >> `isDocumentAttribute()` methods in the `StyleAttribute` class >> - `RichTextArea`: `documentArea` read-only property >> >> >> ### Testing >> >> In addition to the `RichEditorDemoApp`, the standalone Monkey Tester [1] >> provides additional options via context menu in the `CodeArea` and >> `RichTextArea` pages. >> >> >> ### Questions to the... > > Andy Goryachev has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 194 commits: > > - test standard attributes > - Merge branch 'master' into 8356042.ruler > - ws > - test doc props > - cleanup > - tab stops > - Merge branch 'master' into 8356042.ruler > - renamed doc > - docs > - Merge branch 'master' into 8356042.ruler > - ... and 184 more: https://git.openjdk.org/jfx/compare/738be0f1...87241d12 The updated API looks good with a couple naming suggestions. I left a few doc comments as well. I also left a couple implementation comments. The main one being how the version number is treated. As I mentioned in one of the inline comments, I'm not sold on the notion that the Version string should be treated as "just another section in the docs" and as a type of StyleAttribute. That latter especially seems odd, since the version is only something you should encounter when reading or writing a file. It has relevance to the I/O of the file, not to the processing of the document once read. I think this could be handled as a follow-up if you prefer, perhaps at the same time the Version string is made mandatory. doc-files/controls/RichTextArea/RichTextArea_DataFormat_Incubator.md line 54: > 52: > 53: The document is persisted as a UTF-8 encoded plain text file which > contains a sequence of segments. > 54: The first segment is the document properties segment, followed by > segments representing the paragraphs. You need to say something about the version here, which is before the document properties. modules/jfx.incubator.richtext/src/main/java/com/sun/jfx/incubator/scene/control/richtext/Converters.java line 203: > 201: private static TabStops toTabStops(String text) { > 202: if (text.length() == 0) { > 203: return null; An empty list might be cleaner unless there is a need for `null` (e.g., if `null` is logically distinct from an empty list). modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/RichTextFormatHandler.java line 544: > 542: index += 2; > 543: // TODO check if already exists > 544: String ver = parseString(); In order for it to be effective, the version needs to be the first thing in the file in addition to there being only one of them (and it eventually needs to be mandatory). I'm still not sold on treating the version string as "just another section", but at a minimum, you need to check whether this is the first thing seen (not just the first version block), and fail if it isn't. modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/RichTextModel.java line 56: > 54: * @since 27 > 55: */ > 56: public static final double DEFAULT_TAB_STOPS_8 = 0.0; When I suggested using named static final fields (constants), I hadn't realized that this was a floating-point attribute. It still seems OK to do this, but we should be careful checking for equality (other than equality with 0). I don't much care for the use of `8` in the name. That seems odd. Maybe `DEFAULT_TAB_STOPS_FIXED` or similar? modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/StyleAttributeMap.java line 362: > 360: * This convenience method returns the value of {@link #TAB_STOPS} > attribute, or null. > 361: * @return the paragraph alignment attribute value > 362: */ Missing `@since 27` modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/StyleAttributeMap.java line 672: > 670: for (int i = 0; i < positions.length; i++) { > 671: ts[i] = new TabStop(positions[i]); > 672: } The `ts` variable is unused and unneeded now, so you can removed these lines. modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/StyledTextModel.java line 791: > 789: case DOCUMENT_PROPERTIES: > 790: handleDocumentProperties(seg.getDocumentProperties(), > completeReplacement); > 791: break; Doesn't this need to handle VERSION (in any case, this seems like another reason not to treat version as just another attribute type). modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/TabStops.java line 40: > 38: * @since 27 > 39: */ > 40: public class TabStops implements List<TabStop> { This seems like a good solution to allow using a list of stops (rather than the array you used to have). Since the list of stops is unmodifiable after constructions, perhaps the docs should say that? Suggestion: Make this class `final` unless you have a a compelling reason to extend it. modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/TabStops.java line 45: > 43: > 44: /** > 45: * Constructor. I would use the same language as the `of` method -- it's a little terse as it stands/ Something like: Creates a {@code TabStops} instance from tab stop positions. modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/TabStops.java line 57: > 55: * @return the new instance > 56: */ > 57: public static TabStops of(double ... positions) { Minor: we usually omit the space before the `...` ------------- PR Review: https://git.openjdk.org/jfx/pull/1800#pullrequestreview-4510117786 PR Review Comment: https://git.openjdk.org/jfx/pull/1800#discussion_r3424125008 PR Review Comment: https://git.openjdk.org/jfx/pull/1800#discussion_r3423996073 PR Review Comment: https://git.openjdk.org/jfx/pull/1800#discussion_r3424071413 PR Review Comment: https://git.openjdk.org/jfx/pull/1800#discussion_r3424016921 PR Review Comment: https://git.openjdk.org/jfx/pull/1800#discussion_r3424187790 PR Review Comment: https://git.openjdk.org/jfx/pull/1800#discussion_r3424132311 PR Review Comment: https://git.openjdk.org/jfx/pull/1800#discussion_r3424111163 PR Review Comment: https://git.openjdk.org/jfx/pull/1800#discussion_r3423845095 PR Review Comment: https://git.openjdk.org/jfx/pull/1800#discussion_r3423894939 PR Review Comment: https://git.openjdk.org/jfx/pull/1800#discussion_r3423904986
