Re: [Rev 03] RFR: 8130738: Add tabSize property to Text and TextFlow
On Fri, 20 Dec 2019 00:19:43 GMT, Kevin Rushforth wrote: >> I was thinking of deferring the `apps/toys` demo to avoid any delays in >> getting the new API into JavaFX 14. I have something for it, I just don't >> want any feedback on it to hold up the review of this issue. Is there >> anything else I need to do? Is the CSR in good shape? > > I agree that that addition of a demo program in `apps/toys` can be decoupled > from this. Please file a follow-up issue for this. > > I reviewed the CSR. It looks ready to go. > > In addition to an approved CSR, both @prrace and I will need to review this > PR. I have created https://bugs.openjdk.java.net/browse/JDK-8236438 to track the creation of a demo program. - PR: https://git.openjdk.java.net/jfx/pull/32
Re: [Rev 03] RFR: 8130738: Add tabSize property to Text and TextFlow
On Thu, 19 Dec 2019 13:48:11 GMT, Scott Palmer wrote: >> > > I was thinking of deferring the `apps/toys` demo to avoid any delays in > getting the new API into JavaFX 14. I have something for it, I just don't > want any feedback on it to hold up the review of this issue. Is there > anything else I need to do? Is the CSR in good shape? I agree that that addition of a demo program in `apps/toys` can be decoupled from this. Please file a follow-up issue for this. I reviewed the CSR. It looks ready to go. In addition to an approved CSR, both @prrace and I will need to review this PR. - PR: https://git.openjdk.java.net/jfx/pull/32
Re: [Rev 03] RFR: 8130738: Add tabSize property to Text and TextFlow
On Thu, 12 Dec 2019 21:51:56 GMT, Nir Lisker wrote: >> The pull request has been updated with 1 additional commit. > > I was thinking of deferring the `apps/toys` demo to avoid any delays in getting the new API into JavaFX 14. I have something for it, I just don't want any feedback on it to hold up the review of this issue. Is there anything else I need to do? Is the CSR in good shape? - PR: https://git.openjdk.java.net/jfx/pull/32
Re: [Rev 03] RFR: 8130738: Add tabSize property to Text and TextFlow
On Fri, 13 Dec 2019 01:10:48 GMT, Scott Palmer wrote:
>> modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 1450:
>>
>>> 1449: private static final CssMetaData TAB_SIZE =
>>> 1450: new CssMetaData("-fx-tab-size",
>>> 1451: SizeConverter.getInstance(),
>>> TextLayout.DEFAULT_TAB_SIZE) {
>>
>> I think that type parameters should have space separation after a comma:
>> `Text, Number`, though I've seen without too.
>
> Most of the instances where CssMetaData is used in the rest of the file don't
> have a space. The exceptions seem to be where there is a wildcard in the type
> parameter. It could be left as-is simply to maintain consistency within the
> file. Though I tend to agree that a space makes sense.
Either is fine with me in this case.
-
PR: https://git.openjdk.java.net/jfx/pull/32
Re: [Rev 03] RFR: 8130738: Add tabSize property to Text and TextFlow
On Fri, 13 Dec 2019 01:29:47 GMT, Phil Race wrote: >> The terms "tab character" or "horizontal tab" refer to the ASCII tab >> character itself. Since a tab character isn't a fixed number of spaces, >> changing it to "size of a tab character" could be misleading. I'd be fine >> with another alternative, though, if someone could come up with a better one. > > We are referring to the character here aren't we ? ie the actual character > and the rest of it is about how it renders. > Paraphrasing the java doc it says : > If you display it will display as N instances of space character> I find "size of the tab character" to be a bit confusing. The number of space you advance is not a constant for one, so tab characters don't really have a fixed size in that sense. The javadoc could be rephrased as "the distance between tab stops". However, that keeps the unfamiliar, but correct IMO, "tab stop" wording. For the record the top google hit for "tab stop" results in: "A tab stop is a term used to describe the location the cursor stops after the Tab key is pressed." Wikipedia says something similar and adds, "In text editors on a computer, the same concept is implemented simplistically with automatic, fixed tab stops." - PR: https://git.openjdk.java.net/jfx/pull/32
Re: [Rev 03] RFR: 8130738: Add tabSize property to Text and TextFlow
On Thu, 12 Dec 2019 22:02:53 GMT, Kevin Rushforth wrote: >> modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 1273: >> >>> 1272: /** >>> 1273: * The size of a tab stop in spaces. >>> 1274: * Values less than 1 are treated as 1. >> >> "tab stop" seems to be an uncommon term. Better are "horizontal tab" (used >> in the >> [JLS](https://docs.oracle.com/javase/specs/jls/se13/html/jls-3.html#jls-3.10.6)), >> "tab character" (used in >> [`Pattern`](https://docs.oracle.com/en/java/javase/13/docs/api/java.base/java/util/regex/Pattern.html), >> "horizontal tabulation" or the like. > > The terms "tab character" or "horizontal tab" refer to the ASCII tab > character itself. Since a tab character isn't a fixed number of spaces, > changing it to "size of a tab character" could be misleading. I'd be fine > with another alternative, though, if someone could come up with a better one. We are referring to the character here aren't we ? ie the actual character and the rest of it is about how it renders. Paraphrasing the java doc it says : If you display it will display as N instances of - PR: https://git.openjdk.java.net/jfx/pull/32
Re: [Rev 03] RFR: 8130738: Add tabSize property to Text and TextFlow
On Thu, 12 Dec 2019 21:41:12 GMT, Nir Lisker wrote:
>> The pull request has been updated with 1 additional commit.
>
> modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 1883:
>
>> 1882: }
>> 1883: @Override protected void invalidated() {
>> 1884: TextLayout layout = getTextLayout();
>
> I think that annotations tend to go on a line above what they annotate,
I'm following the pattern established elsewhere in the file for other property
overrides. (underline, strikethrough, textAlignment, selectionEnd, etc.). I
would keep this for consistency and compactness, unless there are more votes
against it.
-
PR: https://git.openjdk.java.net/jfx/pull/32
Re: [Rev 03] RFR: 8130738: Add tabSize property to Text and TextFlow
On Thu, 12 Dec 2019 21:29:06 GMT, Nir Lisker wrote:
>> The pull request has been updated with 1 additional commit.
>
> modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 1450:
>
>> 1449: private static final CssMetaData TAB_SIZE =
>> 1450: new CssMetaData("-fx-tab-size",
>> 1451: SizeConverter.getInstance(),
>> TextLayout.DEFAULT_TAB_SIZE) {
>
> I think that type parameters should have space separation after a comma:
> `Text, Number`, though I've seen without too.
Most of the instances where CssMetaData is used in the rest of the file don't
have a space. The exceptions seem to be where there is a wildcard in the type
parameter. It could be left as-is simply to maintain consistency within the
file. Though I tend to agree that a space makes sense.
-
PR: https://git.openjdk.java.net/jfx/pull/32
Re: [Rev 03] RFR: 8130738: Add tabSize property to Text and TextFlow
On Thu, 12 Dec 2019 21:40:34 GMT, Nir Lisker wrote:
>> The pull request has been updated with 1 additional commit.
>
> modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 1882:
>
>> 1881: return StyleableProperties.TAB_SIZE;
>> 1882: }
>> 1883: @Override protected void invalidated() {
>
> The `SimpleStyleableIntegerProperty` subclass can be used instead of
> `StyleableIntegerProperty` to make the code cleaner:
> new SimpleStyleableIntegerProperty(StyleableProperties.TAB_SIZE, Text.this,
> "tabSize", TextLayout.DEFAULT_TAB_SIZE);
>
> (This can be done for many places in the library)
I'd probably leave it as is for this PR, since it matches other attributes in
this file.
-
PR: https://git.openjdk.java.net/jfx/pull/32
Re: [Rev 03] RFR: 8130738: Add tabSize property to Text and TextFlow
On Thu, 12 Dec 2019 21:16:32 GMT, Nir Lisker wrote: >> The pull request has been updated with 1 additional commit. > > modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 1273: > >> 1272: /** >> 1273: * The size of a tab stop in spaces. >> 1274: * Values less than 1 are treated as 1. > > "tab stop" seems to be an uncommon term. Better are "horizontal tab" (used in > the > [JLS](https://docs.oracle.com/javase/specs/jls/se13/html/jls-3.html#jls-3.10.6)), > "tab character" (used in > [`Pattern`](https://docs.oracle.com/en/java/javase/13/docs/api/java.base/java/util/regex/Pattern.html), > "horizontal tabulation" or the like. The terms "tab character" or "horizontal tab" refer to the ASCII tab character itself. Since a tab character isn't a fixed number of spaces, changing it to "size of a tab character" could be misleading. I'd be fine with another alternative, though, if someone could come up with a better one. - PR: https://git.openjdk.java.net/jfx/pull/32
Re: [Rev 03] RFR: 8130738: Add tabSize property to Text and TextFlow
On Thu, 12 Dec 2019 21:52:05 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.
>
> The pull request has been updated with 1 additional commit.
modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 1273:
> 1272: /**
> 1273: * The size of a tab stop in spaces.
> 1274: * Values less than 1 are treated as 1.
"tab stop" seems to be an uncommon term. Better are "horizontal tab" (used in
the
[JLS](https://docs.oracle.com/javase/specs/jls/se13/html/jls-3.html#jls-3.10.6)),
"tab character" (used in
[`Pattern`](https://docs.oracle.com/en/java/javase/13/docs/api/java.base/java/util/regex/Pattern.html),
"horizontal tabulation" or the like.
modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 1276:
> 1275: *
> 1276: * @defaultValue {@code 8}
> 1277: *
No need for `@code` with numeric constants.
modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 1450:
> 1449: private static final CssMetaData TAB_SIZE =
> 1450: new CssMetaData("-fx-tab-size",
> 1451: SizeConverter.getInstance(),
> TextLayout.DEFAULT_TAB_SIZE) {
I think that type parameters should have space separation after a comma: `Text,
Number`, though I've seen without too.
modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 1882:
> 1881: return StyleableProperties.TAB_SIZE;
> 1882: }
> 1883: @Override protected void invalidated() {
The `SimpleStyleableIntegerProperty` subclass can be used instead of
`StyleableIntegerProperty` to make the code cleaner:
new SimpleStyleableIntegerProperty(StyleableProperties.TAB_SIZE, Text.this,
"tabSize", TextLayout.DEFAULT_TAB_SIZE);
(This can be done for many places in the library)
modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 1883:
> 1882: }
> 1883: @Override protected void invalidated() {
> 1884: TextLayout layout = getTextLayout();
I think that annotations tend to go on a line above what they annotate,
modules/javafx.graphics/src/main/java/javafx/scene/text/TextFlow.java line 575:
> 574: private static final CssMetaData TAB_SIZE =
> 575: new CssMetaData("-fx-tab-size",
> 576: SizeConverter.getInstance(),
> TextLayout.DEFAULT_TAB_SIZE) {
Same comment as for `Text`.
modules/javafx.graphics/src/main/java/javafx/scene/text/TextFlow.java line 517:
> 516: }
> 517: @Override protected void invalidated() {
> 518: TextLayout layout = getTextLayout();
Same comments as for `Text`.
modules/javafx.graphics/src/main/java/javafx/scene/text/TextFlow.java line 495:
> 494: *
> 495: * @defaultValue {@code 8}
> 496: *
Same comments as for `Text`.
-
Changes requested by nlisker (Committer).
PR: https://git.openjdk.java.net/jfx/pull/32
Re: [Rev 03] RFR: 8130738: Add tabSize property to Text and TextFlow
On Thu, 12 Dec 2019 19:48:37 GMT, Scott Palmer wrote: >> In that case, I recommend just doing the API get/set tests for `TextFlow` >> without creating a `Scene` or `Stage`. This won't need anything from the >> `StubToolkit`. > > In my attempts to address the issue with StubTextLayout I discovered bugs. > TextFlow is NOT properly overriding the tabSize of the Text nodes. If you > set the Text node tab size later, the layout reacts and adjusts to the > tabSize of the Text node even though it is contained in a TextFlow. > Whichever node changes the tabSize last affects the layout. I'm going to > have to study this more, but this change isn't ready at this point. Looks like I was missing a check for isSpan() in the invalidated method of the tabSize property of Text. I've fixed StubTextLayout to work with TextFlow now. - PR: https://git.openjdk.java.net/jfx/pull/32
Re: [Rev 03] RFR: 8130738: Add tabSize property to Text and TextFlow
On Thu, 12 Dec 2019 15:52:23 GMT, Kevin Rushforth wrote:
>> Good catch. Yes, please update it as you suggested.
>
> The following comment from @swpalmer sent to the openjfx-dev mailing list
> wasn't mirrored in the PR (I have alerted the Skara team about this).
>
>
> From: Scott Palmer
>
>> I think a new `TextFlowTest.java` would be a good place for those tests.
>
> My first attempt at unit tests for TextFlow are failing. I believe the
> StubTextLayout is not equipped to handle TextFlow. This may be a bigger job…
>
> This fragment triggers a NPE:
> ...
> Toolkit tk = (StubToolkit) Toolkit.getToolkit();
> HBox root = new HBox();
> Scene scene = new Scene(root);
> Stage stage = new Stage();
> stage.setScene(scene);
> stage.setWidth(300);
> stage.setHeight(200);
>
> try {
> Text text1 = new Text("\tfirst");
> Text text2 = new Text("\tsecond");
> TextFlow textFlow = new TextFlow(text1, text2);
> root.getChildren().addAll(textFlow);
> stage.show();
> ...
>
> test.javafx.scene.text.TextFlowTest > testTabSize FAILED
> java.lang.NullPointerException
> at
> test.com.sun.javafx.pgstub.StubTextLayout.getBounds(StubTextLayout.java:88)
> at
> test.com.sun.javafx.pgstub.StubTextLayout.getBounds(StubTextLayout.java:82)
> at
> javafx.graphics/javafx.scene.text.TextFlow.computePrefWidth(TextFlow.java:254)
> at javafx.graphics/javafx.scene.Parent.prefWidth(Parent.java:1019)
> at
> javafx.graphics/javafx.scene.layout.Region.prefWidth(Region.java:1543)
> at
> javafx.graphics/javafx.scene.layout.Region.computeChildPrefAreaWidth(Region.java:1946)
> at
> javafx.graphics/javafx.scene.layout.HBox.getAreaWidths(HBox.java:465)
> at
> javafx.graphics/javafx.scene.layout.HBox.computeContentWidth(HBox.java:540)
> at
> javafx.graphics/javafx.scene.layout.HBox.computePrefWidth(HBox.java:433)
> at javafx.graphics/javafx.scene.Parent.prefWidth(Parent.java:1019)
> at
> javafx.graphics/javafx.scene.layout.Region.prefWidth(Region.java:1543)
> at
> javafx.graphics/javafx.scene.Scene.getPreferredWidth(Scene.java:1799)
> at
> javafx.graphics/javafx.scene.Scene.resizeRootToPreferredSize(Scene.java:1779)
> at javafx.graphics/javafx.scene.Scene.preferredSize(Scene.java:1747)
> at javafx.graphics/javafx.scene.Scene$2.preferredSize(Scene.java:393)
> at
> javafx.graphics/com.sun.javafx.scene.SceneHelper.preferredSize(SceneHelper.java:66)
> at
> javafx.graphics/javafx.stage.Window$12.invalidated(Window.java:1086)
> at
> javafx.base/javafx.beans.property.BooleanPropertyBase.markInvalid(BooleanPropertyBase.java:110)
> at
> javafx.base/javafx.beans.property.BooleanPropertyBase.set(BooleanPropertyBase.java:145)
> at javafx.graphics/javafx.stage.Window.setShowing(Window.java:1174)
> at javafx.graphics/javafx.stage.Window.show(Window.java:1189)
> at javafx.graphics/javafx.stage.Stage.show(Stage.java:273)
> at
> test.javafx.scene.text.TextFlowTest.testTabSize(TextFlowTest.java:60)
>
> Scott
In that case, I recommend just doing the API get/set tests for `TextFlow`
without creating a `Scene` or `Stage`. This won't need anything from the
`StubToolkit`.
-
PR: https://git.openjdk.java.net/jfx/pull/32
Re: [Rev 03] RFR: 8130738: Add tabSize property to Text and TextFlow
On Thu, 12 Dec 2019 15:19:39 GMT, Kevin Rushforth wrote:
>> I just noticed while updating the CSR that, while I mentioned it on the
>> mailing list, the fact that TextFlow's tabSize override that of any
>> contained Text nodes is not documented.
>> Shall I add to the javadoc for tabSize in TextFlow : "This value overrides
>> the tabSize of contained Text nodes." ?
>
> Good catch. Yes, please update it as you suggested.
The following comment from @swpalmer sent to the openjfx-dev mailing list
wasn't mirrored in the PR (I have alerted the Skara team about this).
From: Scott Palmer
> I think a new `TextFlowTest.java` would be a good place for those tests.
My first attempt at unit tests for TextFlow are failing. I believe the
StubTextLayout is not equipped to handle TextFlow. This may be a bigger job…
This fragment triggers a NPE:
...
Toolkit tk = (StubToolkit) Toolkit.getToolkit();
HBox root = new HBox();
Scene scene = new Scene(root);
Stage stage = new Stage();
stage.setScene(scene);
stage.setWidth(300);
stage.setHeight(200);
try {
Text text1 = new Text("\tfirst");
Text text2 = new Text("\tsecond");
TextFlow textFlow = new TextFlow(text1, text2);
root.getChildren().addAll(textFlow);
stage.show();
...
test.javafx.scene.text.TextFlowTest > testTabSize FAILED
java.lang.NullPointerException
at
test.com.sun.javafx.pgstub.StubTextLayout.getBounds(StubTextLayout.java:88)
at
test.com.sun.javafx.pgstub.StubTextLayout.getBounds(StubTextLayout.java:82)
at
javafx.graphics/javafx.scene.text.TextFlow.computePrefWidth(TextFlow.java:254)
at javafx.graphics/javafx.scene.Parent.prefWidth(Parent.java:1019)
at
javafx.graphics/javafx.scene.layout.Region.prefWidth(Region.java:1543)
at
javafx.graphics/javafx.scene.layout.Region.computeChildPrefAreaWidth(Region.java:1946)
at javafx.graphics/javafx.scene.layout.HBox.getAreaWidths(HBox.java:465)
at
javafx.graphics/javafx.scene.layout.HBox.computeContentWidth(HBox.java:540)
at
javafx.graphics/javafx.scene.layout.HBox.computePrefWidth(HBox.java:433)
at javafx.graphics/javafx.scene.Parent.prefWidth(Parent.java:1019)
at
javafx.graphics/javafx.scene.layout.Region.prefWidth(Region.java:1543)
at javafx.graphics/javafx.scene.Scene.getPreferredWidth(Scene.java:1799)
at
javafx.graphics/javafx.scene.Scene.resizeRootToPreferredSize(Scene.java:1779)
at javafx.graphics/javafx.scene.Scene.preferredSize(Scene.java:1747)
at javafx.graphics/javafx.scene.Scene$2.preferredSize(Scene.java:393)
at
javafx.graphics/com.sun.javafx.scene.SceneHelper.preferredSize(SceneHelper.java:66)
at javafx.graphics/javafx.stage.Window$12.invalidated(Window.java:1086)
at
javafx.base/javafx.beans.property.BooleanPropertyBase.markInvalid(BooleanPropertyBase.java:110)
at
javafx.base/javafx.beans.property.BooleanPropertyBase.set(BooleanPropertyBase.java:145)
at javafx.graphics/javafx.stage.Window.setShowing(Window.java:1174)
at javafx.graphics/javafx.stage.Window.show(Window.java:1189)
at javafx.graphics/javafx.stage.Stage.show(Stage.java:273)
at test.javafx.scene.text.TextFlowTest.testTabSize(TextFlowTest.java:60)
Scott
-
PR: https://git.openjdk.java.net/jfx/pull/32
Re: [Rev 03] RFR: 8130738: Add tabSize property to Text and TextFlow
On Thu, 12 Dec 2019 15:15:38 GMT, Scott Palmer wrote: >> I think a new `TextFlowTest.java` would be a good place for those tests. >> >> Our build is set up to use `ant` so if you want to wire it up to the build, >> you'll need that (it should be as simple as having `ANT_HOME` set to >> `apache-ant-1.10.5`). It would be fine to defer this if you can't get it >> working. > > I just noticed while updating the CSR that, while I mentioned it on the > mailing list, the fact that TextFlow's tabSize override that of any contained > Text nodes is not documented. > Shall I add to the javadoc for tabSize in TextFlow : "This value overrides > the tabSize of contained Text nodes." ? Good catch. Yes, please update it as you suggested. - PR: https://git.openjdk.java.net/jfx/pull/32
Re: [Rev 03] RFR: 8130738: Add tabSize property to Text and TextFlow
On Wed, 11 Dec 2019 01:22:54 GMT, Kevin Rushforth wrote: >> The unit tests that were already added to `TextTest.java` cover the new >> methods on Text, but not in every combination. I'll add a couple more to >> ensure all combinations are covered. `TextFlow` has no existing unit tests >> that I could find! Shall I add a new `TextFlowText.java` file for just >> these tests? >> >> The apps/toys folder seems to use ant-based projects. The existing ones >> don't build for me. If I made a new project there could I use the JavaFX >> Gradle plugin and configure it to get JavaFX from the build/sdk folder? > > I think a new `TextFlowTest.java` would be a good place for those tests. > > Our build is set up to use `ant` so if you want to wire it up to the build, > you'll need that (it should be as simple as having `ANT_HOME` set to > `apache-ant-1.10.5`). It would be fine to defer this if you can't get it > working. I just noticed while updating the CSR that, while I mentioned it on the mailing list, the fact that TextFlow's tabSize override that of any contained Text nodes is not documented. Shall I add to the javadoc for tabSize in TextFlow : "This value overrides the tabSize of contained Text nodes." ? - PR: https://git.openjdk.java.net/jfx/pull/32
Re: [Rev 03] RFR: 8130738: Add tabSize property to Text and TextFlow
> On Dec 10, 2019, at 8:23 PM, Kevin Rushforth wrote:
...
>
> I think a new `TextFlowTest.java` would be a good place for those tests.
My first attempt at unit tests for TextFlow are failing. I believe the
StubTextLayout is not equipped to handle TextFlow. This may be a bigger job…
This fragment triggers a NPE:
...
Toolkit tk = (StubToolkit) Toolkit.getToolkit();
HBox root = new HBox();
Scene scene = new Scene(root);
Stage stage = new Stage();
stage.setScene(scene);
stage.setWidth(300);
stage.setHeight(200);
try {
Text text1 = new Text("\tfirst");
Text text2 = new Text("\tsecond");
TextFlow textFlow = new TextFlow(text1, text2);
root.getChildren().addAll(textFlow);
stage.show();
...
test.javafx.scene.text.TextFlowTest > testTabSize FAILED
java.lang.NullPointerException
at
test.com.sun.javafx.pgstub.StubTextLayout.getBounds(StubTextLayout.java:88)
at
test.com.sun.javafx.pgstub.StubTextLayout.getBounds(StubTextLayout.java:82)
at
javafx.graphics/javafx.scene.text.TextFlow.computePrefWidth(TextFlow.java:254)
at javafx.graphics/javafx.scene.Parent.prefWidth(Parent.java:1019)
at
javafx.graphics/javafx.scene.layout.Region.prefWidth(Region.java:1543)
at
javafx.graphics/javafx.scene.layout.Region.computeChildPrefAreaWidth(Region.java:1946)
at javafx.graphics/javafx.scene.layout.HBox.getAreaWidths(HBox.java:465)
at
javafx.graphics/javafx.scene.layout.HBox.computeContentWidth(HBox.java:540)
at
javafx.graphics/javafx.scene.layout.HBox.computePrefWidth(HBox.java:433)
at javafx.graphics/javafx.scene.Parent.prefWidth(Parent.java:1019)
at
javafx.graphics/javafx.scene.layout.Region.prefWidth(Region.java:1543)
at javafx.graphics/javafx.scene.Scene.getPreferredWidth(Scene.java:1799)
at
javafx.graphics/javafx.scene.Scene.resizeRootToPreferredSize(Scene.java:1779)
at javafx.graphics/javafx.scene.Scene.preferredSize(Scene.java:1747)
at javafx.graphics/javafx.scene.Scene$2.preferredSize(Scene.java:393)
at
javafx.graphics/com.sun.javafx.scene.SceneHelper.preferredSize(SceneHelper.java:66)
at javafx.graphics/javafx.stage.Window$12.invalidated(Window.java:1086)
at
javafx.base/javafx.beans.property.BooleanPropertyBase.markInvalid(BooleanPropertyBase.java:110)
at
javafx.base/javafx.beans.property.BooleanPropertyBase.set(BooleanPropertyBase.java:145)
at javafx.graphics/javafx.stage.Window.setShowing(Window.java:1174)
at javafx.graphics/javafx.stage.Window.show(Window.java:1189)
at javafx.graphics/javafx.stage.Stage.show(Stage.java:273)
at test.javafx.scene.text.TextFlowTest.testTabSize(TextFlowTest.java:60)
Scott
Re: [Rev 03] RFR: 8130738: Add tabSize property to Text and TextFlow
On Wed, 11 Dec 2019 01:04:15 GMT, Scott Palmer wrote: >> As a follow-on point to the missing public method in TextFlow, can you add >> unit tests for the API methods on both `Text` and `TextFlow`? A good way to >> do that is to have a test for all combinations of setting the value via the >> setter and the property method, and getting the value via the getter and the >> property method. > > The unit tests that were already added to `TextTest.java` cover the new > methods on Text, but not in every combination. I'll add a couple more to > ensure all combinations are covered. `TextFlow` has no existing unit tests > that I could find! Shall I add a new `TextFlowText.java` file for just these > tests? > > The apps/toys folder seems to use ant-based projects. The existing ones > don't build for me. If I made a new project there could I use the JavaFX > Gradle plugin and configure it to get JavaFX from the build/sdk folder? I think a new `TextFlowTest.java` would be a good place for those tests. Our build is set up to use `ant` so if you want to wire it up to the build, you'll need that (it should be as simple as having `ANT_HOME` set to `apache-ant-1.10.5`). It would be fine to defer this if you can't get it working. - PR: https://git.openjdk.java.net/jfx/pull/32
Re: [Rev 03] RFR: 8130738: Add tabSize property to Text and TextFlow
On Tue, 10 Dec 2019 23:51:48 GMT, Kevin Rushforth wrote: >>> Should this PR also add a tabSize property to controls such as TextArea? Or >>> should that be a different PR after this one is merged? >> >> This would need to be a new enhancement and would first need to be discussed >> on the openjfx-dev mailing list. There are additional things to consider in >> order to support tab size for controls, and it isn't clear whether there is >> enough benefit in doing it. > > As a follow-on point to the missing public method in TextFlow, can you add > unit tests for the API methods on both `Text` and `TextFlow`? A good way to > do that is to have a test for all combinations of setting the value via the > setter and the property method, and getting the value via the getter and the > property method. The unit tests that were already added to `TextTest.java` cover the new methods on Text, but not in every combination. I'll add a couple more to ensure all combinations are covered. `TextFlow` has no existing unit tests that I could find! Shall I add a new `TextFlowText.java` file for just these tests? The apps/toys folder seems to use ant-based projects. The existing ones don't build for me. If I made a new project there could I use the JavaFX Gradle plugin and configure it to get JavaFX from the build/sdk folder? - PR: https://git.openjdk.java.net/jfx/pull/32
Re: [Rev 03] RFR: 8130738: Add tabSize property to Text and TextFlow
On Tue, 10 Dec 2019 18:54:08 GMT, Kevin Rushforth wrote: >> 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 two >> 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. > >> Should this PR also add a tabSize property to controls such as TextArea? Or >> should that be a different PR after this one is merged? > > This would need to be a new enhancement and would first need to be discussed > on the openjfx-dev mailing list. There are additional things to consider in > order to support tab size for controls, and it isn't clear whether there is > enough benefit in doing it. As a follow-on point to the missing public method in TextFlow, can you add unit tests for the API methods on both `Text` and `TextFlow`? A good way to do that is to have a test for all combinations of setting the value via the setter and the property method, and getting the value via the getter and the property method. - PR: https://git.openjdk.java.net/jfx/pull/32
Re: [Rev 03] RFR: 8130738: Add tabSize property to Text and TextFlow
On Tue, 10 Dec 2019 18:37:50 GMT, Kevin Rushforth wrote: >> 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 two 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. > Should this PR also add a tabSize property to controls such as TextArea? Or > should that be a different PR after this one is merged? This would need to be a new enhancement and would first need to be discussed on the openjfx-dev mailing list. There are additional things to consider in order to support tab size for controls, and it isn't clear whether there is enough benefit in doing it. - PR: https://git.openjdk.java.net/jfx/pull/32
Re: [Rev 03] RFR: 8130738: Add tabSize property to Text and TextFlow
On Tue, 10 Dec 2019 18:38:04 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.
>
> 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
Re: [Rev 03] RFR: 8130738: Add tabSize property to Text and TextFlow
The pull request has been updated with additional changes. Added commits: - af959665: 8130738: Add tabSize property to Text and TextFlow Changes: - all: https://git.openjdk.java.net/jfx/pull/32/files - new: https://git.openjdk.java.net/jfx/pull/32/files/254c40de..af959665 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/32/webrev.03 - incr: https://webrevs.openjdk.java.net/jfx/32/webrev.02-03 Issue: https://bugs.openjdk.java.net/browse/JDK-8130738 Stats: 57 lines in 6 files changed: 25 ins; 26 del; 6 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
