Re: [Rev 04] RFR: 8130738: Add tabSize property to Text and TextFlow
On Sat, 21 Dec 2019 20:35:10 GMT, Phil Race wrote: >> Link problem appears to just be a missing slash: >> https://github.com/openjdk/jfx/blob/master/CONTRIBUTING.md > >> Link problem appears to just be a missing slash: >> https://github.com/openjdk/jfx/blob/master/CONTRIBUTING.md > > Seems this was fixed yesterday : https://github.com/openjdk/skara/pull/344 @swpalmer The change still needs my completed review -- note that there is currently only one reviewer listed in the __Approvers__ section above. Once I approve, which I will do soon, I will sponsor it. I'm still looking for ways to make the CONTRIBUTING guidelines more clear as to when it is OK to integrate. I'll take a stab at it in the new year... - PR: https://git.openjdk.java.net/jfx/pull/32
Re: [Rev 04] RFR: 8130738: Add tabSize property to Text and TextFlow
On Sat, 21 Dec 2019 18:35:26 GMT, Scott Palmer wrote: >> I'm not sure if I'me supposed to try to integrate now that I've made that 10 >> -> 0 change, or if the new change resets the need for review... Also, note >> that the link in the bot msg for "project specific requirements" is giving >> me a 404 error. > > Link problem appears to just be a missing slash: > https://github.com/openjdk/jfx/blob/master/CONTRIBUTING.md > Link problem appears to just be a missing slash: > https://github.com/openjdk/jfx/blob/master/CONTRIBUTING.md Seems this was fixed yesterday : https://github.com/openjdk/skara/pull/344 - PR: https://git.openjdk.java.net/jfx/pull/32
Re: [Rev 04] RFR: 8130738: Add tabSize property to Text and TextFlow
On Sat, 21 Dec 2019 03:03:57 GMT, Scott Palmer wrote: >> Interesting. I was only running the tests in graphics (gradle >> :graphics:test) as when I run all the tests I always get this failure >> (unrelated to anything I've changed): >> >>> Task :base:test >> >> test.javafx.util.converter.LocalDateTimeStringConverterTest > >> toString_to_fromString_testRoundtrip[0] FAILED >> java.time.format.DateTimeParseException: Text '1985-01-12, 12:34 p.m.' >> could not be parsed, unparsed text found at index 19 >> at >> java.base/java.time.format.DateTimeFormatter.parseResolved0(DateTimeFormatter.java:2052) >> at >> java.base/java.time.format.DateTimeFormatter.parse(DateTimeFormatter.java:1877) >> at >> javafx.base/javafx.util.converter.LocalDateTimeStringConverter$LdtConverter.fromString(LocalDateTimeStringConverter.java:208) >> at >> javafx.base/javafx.util.converter.LocalDateTimeStringConverter.fromString(LocalDateTimeStringConverter.java:159) >> at >> test.javafx.util.converter.LocalDateTimeStringConverterTest.toString_to_fromString_testRoundtrip(LocalDateTimeStringConverterTest.java:131) >> >> 5222 tests completed, 1 failed, 27 skipped > > I'm not sure if I'me supposed to try to integrate now that I've made that 10 > -> 0 change, or if the new change resets the need for review... Also, note > that the link in the bot msg for "project specific requirements" is giving me > a 404 error. Link problem appears to just be a missing slash: https://github.com/openjdk/jfx/blob/master/CONTRIBUTING.md - PR: https://git.openjdk.java.net/jfx/pull/32
Re: [Rev 04] RFR: 8130738: Add tabSize property to Text and TextFlow
On 12/20/19, 7:04 PM, Scott Palmer wrote:
I'm not sure if I'me supposed to try to integrate now that I've made that 10 ->
0 change, or if the new change resets the need for review...
It shows ready, which surprises me.
Still learning skara .. I'd expect any change to reset as how can it
know if it is
a good or insignificant change or not no matter how the commenter
characterised the issue ?
Also, note that the link in the bot msg for "project specific requirements"
is giving me a 404 error.
I see that too https://github.com/openjdk/jfx/blob/masterCONTRIBUTING.md
I don't know how the change here :
https://github.com/openjdk/skara/pull/343/files
+ automated pre-integration checks. When the change also ");
+ message.append("fulfills all [project specific
requirements](https://github.com/";);
maps to the project, but it seems like there's a config bug somewhere
that may be just being
exposed
-phil
Re: [Rev 04] RFR: 8130738: Add tabSize property to Text and TextFlow
On Sat, 21 Dec 2019 02:50:54 GMT, Scott Palmer wrote: >> The fix looks good now. There is one problem in the test (in >> `StubTextLayout`) that needs to be fixed. > > Interesting. I was only running the tests in graphics (gradle > :graphics:test) as when I run all the tests I always get this failure > (unrelated to anything I've changed): > >> Task :base:test > > test.javafx.util.converter.LocalDateTimeStringConverterTest > > toString_to_fromString_testRoundtrip[0] FAILED > java.time.format.DateTimeParseException: Text '1985-01-12, 12:34 p.m.' > could not be parsed, unparsed text found at index 19 > at > java.base/java.time.format.DateTimeFormatter.parseResolved0(DateTimeFormatter.java:2052) > at > java.base/java.time.format.DateTimeFormatter.parse(DateTimeFormatter.java:1877) > at > javafx.base/javafx.util.converter.LocalDateTimeStringConverter$LdtConverter.fromString(LocalDateTimeStringConverter.java:208) > at > javafx.base/javafx.util.converter.LocalDateTimeStringConverter.fromString(LocalDateTimeStringConverter.java:159) > at > test.javafx.util.converter.LocalDateTimeStringConverterTest.toString_to_fromString_testRoundtrip(LocalDateTimeStringConverterTest.java:131) > > 5222 tests completed, 1 failed, 27 skipped I'm not sure if I'me supposed to try to integrate now that I've made that 10 -> 0 change, or if the new change resets the need for review... Also, note that the link in the bot msg for "project specific requirements" is giving me a 404 error. - PR: https://git.openjdk.java.net/jfx/pull/32
Re: [Rev 04] RFR: 8130738: Add tabSize property to Text and TextFlow
On Fri, 20 Dec 2019 23:43:53 GMT, Kevin Rushforth wrote: >> The pull request has been updated with 1 additional commit. > > The fix looks good now. There is one problem in the test (in > `StubTextLayout`) that needs to be fixed. Interesting. I was only running the tests in graphics (gradle :graphics:test) as when I run all the tests I always get this failure (unrelated to anything I've changed): > Task :base:test test.javafx.util.converter.LocalDateTimeStringConverterTest > toString_to_fromString_testRoundtrip[0] FAILED java.time.format.DateTimeParseException: Text '1985-01-12, 12:34 p.m.' could not be parsed, unparsed text found at index 19 at java.base/java.time.format.DateTimeFormatter.parseResolved0(DateTimeFormatter.java:2052) at java.base/java.time.format.DateTimeFormatter.parse(DateTimeFormatter.java:1877) at javafx.base/javafx.util.converter.LocalDateTimeStringConverter$LdtConverter.fromString(LocalDateTimeStringConverter.java:208) at javafx.base/javafx.util.converter.LocalDateTimeStringConverter.fromString(LocalDateTimeStringConverter.java:159) at test.javafx.util.converter.LocalDateTimeStringConverterTest.toString_to_fromString_testRoundtrip(LocalDateTimeStringConverterTest.java:131) 5222 tests completed, 1 failed, 27 skipped - PR: https://git.openjdk.java.net/jfx/pull/32
Re: [Rev 04] RFR: 8130738: Add tabSize property to Text and TextFlow
On Fri, 20 Dec 2019 23:44:07 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. The fix looks good now. There is one problem in the test (in `StubTextLayout`) that needs to be fixed. modules/javafx.graphics/src/test/java/test/com/sun/javafx/pgstub/StubTextLayout.java line 50: > 49: private int tabSize = DEFAULT_TAB_SIZE; > 50: private int nullFontSize = 10; > 51: Setting `nullFontSize` to 10 by default has broken one of the existing unit tests (a rather fragile test it seems). I now see the following failure: test.javafx.scene.chart.StackedBarChartTest > testSeriesAdd FAILED org.junit.ComparisonFailure: expected:<10 4[78 234 37 254 432 234 83 499 375 234 140] > but was:<10 4[60 218 35 238 415 218 80 465 360 218 135] > at org.junit.Assert.assertEquals(Assert.java:123) at org.junit.Assert.assertEquals(Assert.java:145) at test.javafx.scene.chart.StackedBarChartTest.testSeriesAdd(StackedBarChartTest.java:110) If you change this to 0, which restores the previous default, leaving in place the setting to 10 in ` setContent(TextSpan[])`, then all tests pass. - PR: https://git.openjdk.java.net/jfx/pull/32
Re: [Rev 04] RFR: 8130738: Add tabSize property to Text and TextFlow
On Sat, 14 Dec 2019 16:38:25 GMT, Kevin Rushforth wrote: >> 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? > > A follow-up issue would be fine. I've created https://bugs.openjdk.java.net/browse/JDK-8235958 as a follow-up javadoc enhancement. - PR: https://git.openjdk.java.net/jfx/pull/32
Re: [Rev 04] RFR: 8130738: Add tabSize property to Text and TextFlow
On Fri, 13 Dec 2019 01:20:47 GMT, Scott Palmer wrote:
>> 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?
A follow-up issue would be fine.
-
PR: https://git.openjdk.java.net/jfx/pull/32
Re: [Rev 04] RFR: 8130738: Add tabSize property to Text and TextFlow
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 04] RFR: 8130738: Add tabSize property to Text and TextFlow
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
Re: [Rev 04] RFR: 8130738: Add tabSize property to Text and TextFlow
> 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
