On Tue, 26 Nov 2019 17:26:33 GMT, Scott Palmer <swpal...@openjdk.org> wrote:
> The pull request has been updated with a complete new set of changes > (possibly due to a rebase). > > ---------------- > > Commits: > - 254c40de: Merge remote-tracking branch 'upstream/master' > - a670c4f8: 8130738: TextFlow's tab width is static > - 68d221c7: 8130738: TextFlow's tab width is static > > Changes: https://git.openjdk.java.net/jfx/pull/32/files > Webrev: https://webrevs.openjdk.java.net/jfx/32/webrev.02 > Issue: https://bugs.openjdk.java.net/browse/JDK-8130738 > Stats: 325 lines in 8 files changed: 261 ins; 0 del; 64 mod > Patch: https://git.openjdk.java.net/jfx/pull/32.diff > Fetch: git fetch https://git.openjdk.java.net/jfx pull/32/head:pull/32 I did a first pass review focusing mostly on the public API. Once we get that solidified I'll look at the implementation and tests more closely. Also, once the public API is done, you can update the CSR with the API. modules/javafx.graphics/src/main/docs/javafx/scene/doc-files/cssref.html line 3035: > 3034: <td> </td> > 3035: </tr> <tr> > 3036: <th class="propertyname" scope="row">-fx-text-alignment</th> the `<tr>` should be on the next line (once you do, you can remove the trailing white space that will then be present). modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java line 655: > 654: if (spaces < 1) > 655: spaces = 1; > 656: if (tabSize != spaces) { Please surround the statement with curly braces (our coding style is to always surround the target of a conditional in curly braces unless it is on the same line). modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 1289: > 1288: * @since 14 > 1289: */ > 1290: public final int getTabSize() { The javadoc property support only requires one of the setXxxx/getXxxx/xxxxProperty methods to have a javadoc comment block. The javadoc tool takes care of documenting the property itself and all three methods. See `wrappingWidth` for a good example. modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 1302: > 1301: * @since 14 > 1302: */ > 1303: public final void setTabSize(int spaces) { Same comment as on the set method. You can move the comment about values being clamped to 1 to the property method (which will then be the only method with javadoc comments). If we use my recommendation of clamping on usage, I recommend the following language, borrowed from `Node::opacity`: Values less than 1 are treated as 1. modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 1895: > 1894: } > 1895: @Override public void set(int v) { super.set((v < > 1) ? 1 : v); } > 1896: @Override protected void invalidated() { For mutable properties, we usually clamp on usage, so that we don't have problems binding to the value. This preserves the invariant that `set(val); get() == val` for all values. If that is what we end up doing, then this overridden method should be removed. modules/javafx.graphics/src/main/java/javafx/scene/text/TextFlow.java line 515: > 514: * @since 14 > 515: */ > 516: public final void setTabSize(int spaces) { Same comments about the javadoc comments as for `Text.java` modules/javafx.graphics/src/main/java/javafx/scene/text/TextFlow.java line 528: > 527: } > 528: @Override public void set(int v) { super.set((v < 1) ? 1 > : v); } > 529: @Override protected void invalidated() { Same clamp-on-use comment as in `Text.java` modules/javafx.graphics/src/test/java/test/com/sun/javafx/pgstub/StubTextLayout.java line 200: > 199: if (spaces < 1) > 200: spaces = 1; > 201: if (tabSize != spaces) { Either surround with braces or put onto the same line as the `if` test. modules/javafx.graphics/src/test/java/test/javafx/scene/text/TextTest.java line 271: > 270: } > 271: } > 272: } In addition to the above test, I recommend some (very basic) tests of the setter / getting / property methods. modules/javafx.graphics/src/test/java/test/javafx/scene/text/TextTest.java line 251: > 250: tk.firePulse(); > 251: assertEquals(text.getTabSize(),8); > 252: // initial width with default 8-space tab This is backwards. The expected value should be the first argument. PR: https://git.openjdk.java.net/jfx/pull/32
