Re: [Rev 02] RFR: 8130738: TextFlow's tab width is static
On Wed, 27 Nov 2019 00:58:22 GMT, Kevin Rushforth wrote:
> On Tue, 26 Nov 2019 17:26:33 GMT, Scott Palmer 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:
>> 3035:
>> 3036: -fx-text-alignment
>
> the `` 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/Property 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
Re: [Rev 02] RFR: 8130738: TextFlow's tab width is static
On Tue, 26 Nov 2019 17:26:33 GMT, Scott Palmer 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:
> 3035:
> 3036: -fx-text-alignment
the `` 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/Property 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
Re: [Rev 02] RFR: 8130738: TextFlow's tab width is static
On Tue, 26 Nov 2019 17:26:33 GMT, Scott Palmer 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 Why do we have a ton of extraneous white space changes in the Stylesheet Handling sections of Text.java and TextFlow.java ? PR: https://git.openjdk.java.net/jfx/pull/32
Re: [Rev 02] RFR: 8130738: TextFlow's tab width is static
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 PR: https://git.openjdk.java.net/jfx/pull/32
