Re: [Integrated] RFR: 8130738: TextFlow's tab width is static
Changeset: 8367e1a6 Author:Scott Palmer Committer: Kevin Rushforth Date: 2019-12-31 18:34:33 + URL: https://git.openjdk.java.net/jfx/commit/8367e1a6 8130738: Add tabSize property to Text and TextFlow Reviewed-by: prr, kcr ! modules/javafx.graphics/src/main/docs/javafx/scene/doc-files/cssref.html ! modules/javafx.graphics/src/main/java/com/sun/javafx/scene/text/TextLayout.java ! modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java ! modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java ! modules/javafx.graphics/src/main/java/javafx/scene/text/TextFlow.java ! modules/javafx.graphics/src/test/java/test/com/sun/javafx/pgstub/StubTextLayout.java + modules/javafx.graphics/src/test/java/test/javafx/scene/text/TextFlowTest.java ! modules/javafx.graphics/src/test/java/test/javafx/scene/text/TextTest.java ! modules/javafx.graphics/src/test/java/test/javafx/scene/text/Text_cssMethods_Test.java
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: RFR: 8130738: TextFlow's tab width is static
On Tue, 26 Nov 2019 18:40:10 GMT, Scott Palmer wrote: > On Thu, 7 Nov 2019 14:56:58 GMT, Kevin Rushforth wrote: > >> On Wed, 6 Nov 2019 19:11:48 GMT, Scott Palmer 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. >>> >>> >>> >>> Commits: >>> - 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.00 >>> Issue: https://bugs.openjdk.java.net/browse/JDK-8130738 >>> Stats: 324 lines in 8 files changed: 260 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 should clamp so it reads back as 1 - Fixed. >> >> This has implications for binding, etc., so I suspect this is not what we >> want. I'll look into this more when I review it. >> >> NOTE: This will need a CSR in addition to two reviewers. You can file the >> CSR any time, but I suggest leaving it in Draft state until Phil and I have >> had a first pass look at the API. > > The indenting was wrong in that section and I had to make some minor changes, > so I corrected the bad indenting. Kevin indicated, "In your specific case, > reformatting the methods in the StyleableProperties nested class that are > adjacent to code you add or modify seems fine, as long as you only change the > indentation and not the line wrapping." > > I didn't intend to trigger an update to this pull request...I find Git > awkward to work with (Mercurial makes more sense to me) so I'm making > mistakes as I bumble around. > > I do expect an update to address the way I've clamped to 1. I agree with > Kevin that it is probable wrong. Just waiting for feedback. I'll take a preliminary look today. I also plan to change the bug title to make it more clear that it is an enhancement request, since it currently reads like a bug report (I'll also change the CSR and PR title to match). PR: https://git.openjdk.java.net/jfx/pull/32
Re: RFR: 8130738: TextFlow's tab width is static
On Thu, 7 Nov 2019 14:56:58 GMT, Kevin Rushforth wrote: > On Wed, 6 Nov 2019 19:11:48 GMT, Scott Palmer 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. >> >> >> >> Commits: >> - 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.00 >> Issue: https://bugs.openjdk.java.net/browse/JDK-8130738 >> Stats: 324 lines in 8 files changed: 260 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 should clamp so it reads back as 1 - Fixed. > > This has implications for binding, etc., so I suspect this is not what we > want. I'll look into this more when I review it. > > NOTE: This will need a CSR in addition to two reviewers. You can file the CSR > any time, but I suggest leaving it in Draft state until Phil and I have had a > first pass look at the API. The indenting was wrong in that section and I had to make some minor changes, so I corrected the bad indenting. Kevin indicated, "In your specific case, reformatting the methods in the StyleableProperties nested class that are adjacent to code you add or modify seems fine, as long as you only change the indentation and not the line wrapping." I didn't intend to trigger an update to this pull request...I find Git awkward to work with (Mercurial makes more sense to me) so I'm making mistakes as I bumble around. I do expect an update to address the way I've clamped to 1. I agree with Kevin that it is probable wrong. Just waiting for feedback. 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
Re: RFR: 8130738: TextFlow's tab width is static
Yes, I suppose this should behave like anything else that uses an IntegerProperty as far as bindings are concerned. I'll wait for further review before reverting that last change. If tabSize=0 worked reasonably it would be much simpler. I'm not sure if I will have the time to hunt down why things go wrong in that case (everything after a tab appears at x offset 0). I've created the CSR, hopefully I did it correctly. https://bugs.openjdk.java.net/browse/JDK-8233810 Scott On Thu, Nov 7, 2019 at 9:57 AM Kevin Rushforth wrote: > On Wed, 6 Nov 2019 19:11:48 GMT, Scott Palmer > 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. > > > > > > > > Commits: > > - 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.00 > > Issue: https://bugs.openjdk.java.net/browse/JDK-8130738 > > Stats: 324 lines in 8 files changed: 260 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 should clamp so it reads back as 1 - Fixed. > > This has implications for binding, etc., so I suspect this is not what we > want. I'll look into this more when I review it. > > NOTE: This will need a CSR in addition to two reviewers. You can file the > CSR any time, but I suggest leaving it in Draft state until Phil and I have > had a first pass look at the API. > > PR: https://git.openjdk.java.net/jfx/pull/32 >
Re: RFR: 8130738: TextFlow's tab width is static
On Wed, 6 Nov 2019 19:11:48 GMT, Scott Palmer 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. > > > > Commits: > - 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.00 > Issue: https://bugs.openjdk.java.net/browse/JDK-8130738 > Stats: 324 lines in 8 files changed: 260 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 should clamp so it reads back as 1 - Fixed. This has implications for binding, etc., so I suspect this is not what we want. I'll look into this more when I review it. NOTE: This will need a CSR in addition to two reviewers. You can file the CSR any time, but I suggest leaving it in Draft state until Phil and I have had a first pass look at the API. PR: https://git.openjdk.java.net/jfx/pull/32
Re: RFR: 8130738: TextFlow's tab width is static
setValue(null) is interpreted as set(0) by IntegerProperty. This in turn will be clamped to 1 by the TextLayout. The property on the Text node will still read as 0. So now that you mention it, I don’t like where the clamping is implemented. I should clamp so it reads back as 1 - Fixed. Scott > On Nov 6, 2019, at 11:23 PM, David Grieve wrote: > > What happens if you do text.tabSizeProperty().setValue(null) ? > >> -Original Message- >> From: openjfx-dev On Behalf Of >> Scott Palmer >> Sent: Wednesday, November 6, 2019 11:12 AM >> To: openjfx-dev@openjdk.java.net >> Subject: RFR: 8130738: TextFlow's tab width is static >> >> 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.
Re: [Rev 01] RFR: 8130738: TextFlow's tab width is static
The pull request has been updated with additional changes. Added commits: - a670c4f8: 8130738: TextFlow's tab width is static Changes: - all: https://git.openjdk.java.net/jfx/pull/32/files - new: https://git.openjdk.java.net/jfx/pull/32/files/68d221c7..a670c4f8 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/32/webrev.01 - incr: https://webrevs.openjdk.java.net/jfx/32/webrev.00-01 Issue: https://bugs.openjdk.java.net/browse/JDK-8130738 Stats: 3 lines in 2 files changed: 1 ins; 0 del; 2 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
RFR: 8130738: TextFlow's tab width is static
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. Commits: - 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.00 Issue: https://bugs.openjdk.java.net/browse/JDK-8130738 Stats: 324 lines in 8 files changed: 260 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
