Re: [Rev 03] RFR: 8130738: Add tabSize property to Text and TextFlow

2019-12-12 Thread Scott Palmer
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

2019-12-12 Thread Scott Palmer
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

2019-12-12 Thread Phil Race
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 04] RFR: 8130738: Add tabSize property to Text and TextFlow

2019-12-12 Thread Scott Palmer
On Thu, 12 Dec 2019 22:02:14 GMT, Nir Lisker  wrote:

>> The pull request has been updated with 1 additional commit.
> 
> modules/javafx.graphics/src/main/java/javafx/scene/text/TextFlow.java line 
> 494:
> 
>> 493:  * Values less than 1 are treated as 1. This value overrides the
>> 494:  * {@code tabSize} of contained {@link javafx.scene.text.Text Text} 
>> nodes.
>> 495:  *
> 
> `{@link Text}` is enough, but the FQN is also fine.
> 
> Does `Text`'s `tabSize` need a note that its value is overriden if the text 
> is contained in a `TextFlow`?

TextFlow isn't mentioned in the JavaDoc for any of the other Text properties 
where the same rule applies.  Perhaps that should be remedied with a follow-up 
task?

-

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


Re: [Rev 03] RFR: 8130738: Add tabSize property to Text and TextFlow

2019-12-12 Thread Scott Palmer
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: RFR: 8223296: NullPointerException in GlassScene.java at line 325

2019-12-12 Thread Kevin Rushforth
On Wed, 11 Dec 2019 17:10:37 GMT, Ambarish Rapte  wrote:

>> The change looks OK as far as it goes, meaning that it will fix the specific 
>> NPE reported by the bug and is looks like a safe fix.
>> 
>> Two questions:
>> 
>> 1. In addition to calling the synchronized `setTKScenePaintListener` method, 
>> you moved the call to the beginning of `dispose`. Is there a reason you 
>> needed to do this?
>> 2. Do any of the other listeners or variables that are set in `dispose` have 
>> the same problem (i.e., are any of the rest accessed from another thread)?
> 
>> 1. In addition to calling the synchronized `setTKScenePaintListener` 
>> method, you moved the call to the beginning of `dispose`. Is there a reason 
>> you needed to do this?
> 
> This is moved only to make first operation when disposing. However the race 
> condition is very rare, but this will just reduce the chances little more.
> 
>> 2. Do any of the other listeners or variables that are set in `dispose` 
>> have the same problem (i.e., are any of the rest accessed from another 
>> thread)?
> 
> Other variable do not have a race condition. All are set or accessed on 
> JavaFX Application thread.
> There are some other variable (not in dispose()) like entireSceneDirty, 
> painting, which are accessed on both threads but those are synchronized 
> access.

> This is moved only to make first operation when disposing. However the race 
> condition is very rare, but this will just reduce the chances little more.

Can you elaborate? Why would there still be a race condition?

-

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


Re: [Rev 03] RFR: 8130738: Add tabSize property to Text and TextFlow

2019-12-12 Thread Kevin Rushforth
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: RFR: 8232811: Dialog's preferred size no longer accommodates multi-line strings

2019-12-12 Thread Kevin Rushforth
On Mon, 9 Dec 2019 18:27:25 GMT, Thiago Milczarek Sayao  
wrote:

> https://bugs.openjdk.java.net/browse/JDK-8232811
> 
> This one was hard to tackle.
> 
> ./gradlew -PFULL_TEST=true -PUSE_ROBOT=true :systemTests:test --tests 
> test.robot.javafx.scene.dialog.DialogWithOwnerSizingTest

This will need two reviewers.

-

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


Re: [Rev 04] RFR: 8225571: Port DND source to use GTK instead of GDK

2019-12-12 Thread Thiago Milczarek Sayao
> https://bugs.openjdk.java.net/browse/JDK-8225571
> 
> To run tests (on the root of the source tree):
> ./gradlew apps
> java @build/run.args -cp apps/toys/DragDrop/dist/DragDrop.jar 
> dragdrop.DragDropWithControls
> java @build/run.args -cp apps/toys/DragDrop/dist/DragDrop.jar 
> dragdrop.DragDropColor
> 
> java -Djdk.gtk.version=2 @build/run.args -cp 
> apps/toys/DragDrop/dist/DragDrop.jar dragdrop.DragDropWithControls
> java -Djdk.gtk.version=2 @build/run.args -cp 
> apps/toys/DragDrop/dist/DragDrop.jar dragdrop.DragDropColor

The pull request has been updated with 1 additional commit.

-

Added commits:
 - b24f407f: Fix "GOT A dragExit when dndGesture is null!"

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/4/files
  - new: https://git.openjdk.java.net/jfx/pull/4/files/47155b08..b24f407f

Webrevs:
 - full: https://webrevs.openjdk.java.net/jfx/4/webrev.04
 - incr: https://webrevs.openjdk.java.net/jfx/4/webrev.03-04

  Stats: 17 lines in 1 file changed: 12 ins; 5 del; 0 mod
  Patch: https://git.openjdk.java.net/jfx/pull/4.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/4/head:pull/4

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


Re: [Rev 03] RFR: 8130738: Add tabSize property to Text and TextFlow

2019-12-12 Thread Kevin Rushforth
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

2019-12-12 Thread Kevin Rushforth
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

2019-12-12 Thread Kevin Rushforth
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

2019-12-12 Thread Scott Palmer
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: RFR: 8225571: Port DND source to use GTK instead of GDK

2019-12-12 Thread Kevin Rushforth
On Wed, 11 Dec 2019 19:05:54 GMT, Thiago Milczarek Sayao  
wrote:

>> With your latest patch, which reverts the changes to `GtkDnDClipboard.java`, 
>> I now get the DRAG_DONE event, but the following warning printed to the 
>> console in the case of a `DRAG_DONE` event with a null transfer mode:
>> 
>> GOT A dragExit when dndGesture is null!
>> 
>> This is coming from 
>> [Scene.java#L2925](https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/java/javafx/scene/Scene.java#L2925).
> 
> My mistake. I saw that and thought it was from the test app.
> 
> Will fix.

All my testing looks good now. I tried both of the programs you listed in the 
description, as well as `tests/manual/dnd/DndTest.java`, on Ubuntu 16.04, 
Ubuntu 19.04, and Oracle Linux 7.7. I ran with the default gtk-3 implementation 
as well as gtk-2.

I'll do a final code review, probably early next week.

This will need a second reviewer.

@pankaj-bansal can you also review this.

-

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


Re: [Rev 03] RFR: 8130738: Add tabSize property to Text and TextFlow

2019-12-12 Thread Scott Palmer
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 04] RFR: 8130738: Add tabSize property to Text and TextFlow

2019-12-12 Thread Scott Palmer
> 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.

-

Added commits:
 - f99a3aa9: 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/af959665..f99a3aa9

Webrevs:
 - full: https://webrevs.openjdk.java.net/jfx/32/webrev.04
 - incr: https://webrevs.openjdk.java.net/jfx/32/webrev.03-04

  Stats: 170 lines in 6 files changed: 147 ins; 9 del; 14 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: [Rev 03] RFR: 8130738: Add tabSize property to Text and TextFlow

2019-12-12 Thread Nir Lisker
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

2019-12-12 Thread Kevin Rushforth
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 04] RFR: 8130738: Add tabSize property to Text and TextFlow

2019-12-12 Thread Nir Lisker
On Thu, 12 Dec 2019 22:02:27 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/TextFlow.java line 494:

> 493:  * Values less than 1 are treated as 1. This value overrides the
> 494:  * {@code tabSize} of contained {@link javafx.scene.text.Text Text} 
> nodes.
> 495:  *

`{@link Text}` is enough, but the FQN is also fine.

Does `Text`'s `tabSize` need a note that its value is overriden if the text is 
contained in a `TextFlow`?

-

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