On Tue, 10 Dec 2019 18:38:04 GMT, Scott Palmer <swpal...@openjdk.org> wrote:
>> Added tabSize property to Text and TextFlow and -fx-tab-size CSS attribute >> to both. TextFlow's tab size overrides that of contained Text nodes. > > The pull request has been updated with 1 additional commit. Overall this looks good to me with one "must fix" API issue and one additional minor comment. In addition to the automated unit test, it might be nice to have a simple app (in `apps/toys`) with a slider to control the tab size of a Text and/or TextFlow. This could be done as a follow-on issue if you prefer. Once you fix the public API issue, you can add ithe API to the CSR and I'll review it. The javadoc, public methods, and CSS additions for the three classes should go into the CSR in the specification section. See [JDK-8195139](https://bugs.openjdk.java.net/browse/JDK-8195139) for a good example. @prrace still needs to review the implementation and API as well. modules/javafx.graphics/src/main/java/com/sun/javafx/scene/text/TextLayout.java line 78: > 77: > 78: static final int DEFAULT_TAB_SIZE = 8; > 79: Although it doesn't matter, since fields of public interfaces are public by default, it might be clearer to add `public` here for consistency with the other members. modules/javafx.graphics/src/main/java/javafx/scene/text/TextFlow.java line 509: > 508: > 509: final IntegerProperty tabSizeProperty() { > 510: if (tabSize == null) { The `tabSizeProperty` method needs to be public in order for it to be part of the API and for `textSize` to be a property (which is what we want). Also, please move `tabSizeProperty` above the set / get methods (leaving the javadoc comment block where it is). I realize that we aren't consistent in how we do this, but for new properties, we are trying to have the javadoc comments be on the property method (it's OK if the comment block is above the private instance of the `Property`), followed by the setter and getter. ------------- Changes requested by kcr (Lead). PR: https://git.openjdk.java.net/jfx/pull/32
