[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Re: [Rev 02] RFR: 8130738: TextFlow's tab width is static
From:       Jeanette Winzenburg <fastegal () openjdk ! java ! net>
Date:       2019-11-27 10:51:24
Message-ID: Bxi0nXEHvko4vGnDCIkPnwTyqVAsAKLqgcdOdYMNjr0=.11a7e106-02e4-4e85-8fb9-ba3d869823b3 () github ! com
[Download RAW message or body]

On Wed, 27 Nov 2019 00:58:22 GMT, Kevin Rushforth <kcr@openjdk.org> wrote:

> On Tue, 26 Nov 2019 17:26:33 GMT, Scott Palmer <swpalmer@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>&nbsp;</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.

Hmm ... so you are saying the clamping is the responsibility of client code (internal \
as well as external)?

PR: https://git.openjdk.java.net/jfx/pull/32


[prev in list] [next in list] [prev in thread] [next in thread]