Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v2]
On Wed, 27 Mar 2024 13:41:21 GMT, Nir Lisker wrote: > That's still 2 iterations. Yes, but one advantage here: We currently do `final List removed = new ArrayList<>(c.getRemovedSize());`, where we allocate a list with a size, that is probably too big since we filter the removed items. So with `toList`, we at least get back a list with the correct size. But true, that we technically iterate twice then. It probably does not matter too much. - PR Review Comment: https://git.openjdk.org/jfx/pull/1430#discussion_r1542147474
Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v2]
On Wed, 27 Mar 2024 23:18:43 GMT, Marius Hanl wrote: >> modules/javafx.controls/src/main/java/javafx/scene/control/MultipleSelectionModelBase.java >> line 773: >> >>> 771: .collect(Collectors.toList()); >>> 772: >>> 773: sortedNewIndices.forEach(this::set); >> >> Why do the double-iteration pattern here and not do the `peek` operation in >> a `forEach` like in the other 2 places? > > `forEach` is void, so we can not return a list afterwards. You don't need to return a list, you create it ahead of time like was done in line 167 List indices = new ArrayList<>(); and the add the elements in `forEach`. - PR Review Comment: https://git.openjdk.org/jfx/pull/1430#discussion_r1542145635
Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v2]
On Wed, 27 Mar 2024 13:51:46 GMT, Nir Lisker wrote: >> drmarmac has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Remove outdated comment > > modules/javafx.controls/src/main/java/javafx/scene/control/MultipleSelectionModelBase.java > line 773: > >> 771: .collect(Collectors.toList()); >> 772: >> 773: sortedNewIndices.forEach(this::set); > > Why do the double-iteration pattern here and not do the `peek` operation in a > `forEach` like in the other 2 places? `forEach` is void, so we can not return a list afterwards. - PR Review Comment: https://git.openjdk.org/jfx/pull/1430#discussion_r1542143935
Re: RFR: JDK-8186188: TableColumHeader: initial auto-size broken if has graphic [v7]
> This PR fixes the issue that the initial column autosizing is wrong under > some circumstances. > The following things will break the initial autosizing: > - Bold Column text (that is where I initially found this problem) > - Another font / font size > - Graphic > > The reason is actually quite simple: The CSS is not (yet) applied initially, > we therefore ALWAYS take the default font into account + the graphic is not > yet layouted as well. > > _It was not so easy to write tests for this, also for me the > `test_resizeColumnToFitContentHeader` is always failing locally. I don't know > what happens here, but he seems to not find a (Stub?) `Font` for me._ > **EDIT: Found out the cause and fixed it. I will check if I can write more > tests since it works now. :)** > > The test I wrote now is checking if the css is applied after we triggered the > autosize, which is what we would expect here since we measure text. > > I also copied the `TableColumnHeaderTest` and rewrote the tests for > `TreeTableView` as well, so we can catch any errors here as well since they > both use different code (although it is technically the same - C errors can > happen very easy). Marius Hanl has updated the pull request incrementally with one additional commit since the last revision: use snapped insets - Changes: - all: https://git.openjdk.org/jfx/pull/1405/files - new: https://git.openjdk.org/jfx/pull/1405/files/b4d03d44..9c516962 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1405=06 - incr: https://webrevs.openjdk.org/?repo=jfx=1405=05-06 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jfx/pull/1405.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1405/head:pull/1405 PR: https://git.openjdk.org/jfx/pull/1405
Re: RFR: 8242553: IntegerSpinner and DoubleSpinner do not wrap around values correctly in some cases
On Wed, 27 Mar 2024 19:19:36 GMT, Andy Goryachev wrote:
> Now, when wrapping occurs, should max/min be considered as valid steps?
For integer values (including list indices), yes, since each integer (or list
index) is a discrete value that can have the value min, max, or any value in
between. And a step of one when you are at "max" should wrap around to "min"
For double... that's a good question. I need to think about it a bit. Often
when doing wrap-around for floating-point values (think rotation transforms) we
treat max and min the same -- although usually that means treating the range as
exclusive of max, and that isn't what is current done for double values of
spinner.
> Example, try both integer and double spinners with {min=0, max=10, step=7}
> starting with value=0. try incrementing.
>
> integer: 0, 7, 3, 10
> double: 0, 7, 10, 0, 7, 10
The integer is working as I would expect (and the next value should be "6").
The double is not -- it looks like it is clamping rather than wrapping unless
the value is already at the max.
> And another question: should the new behavior (whatever everyone agrees to
> eventually) be documented? Where? This PR? JBS? Javadoc? A markdown file in
> doc-files/behavior/?
If we want to specify the functionality, it needs to be documented in the
javadoc-generated API docs. That's the specification. Given that this is a
clarification of what we mean by "wrapping" and not a fundamental change, we
could either do that as part of this enhancement request or a separate one.
> Does it need a CSR?
The current spec is sufficiently vague on what the wrapping algorithm is that
as long as we aren't changing anything fundamental, I don't think we do need a
CSR. If we end up changing the spec to document the wrapping algorithm (either
as part of this PR or separately), then the change in spec would need a CSR.
-
PR Comment: https://git.openjdk.org/jfx/pull/1431#issuecomment-2024136091
Re: RFR: JDK-8186188: TableColumHeader: initial auto-size broken if has graphic [v6]
> This PR fixes the issue that the initial column autosizing is wrong under > some circumstances. > The following things will break the initial autosizing: > - Bold Column text (that is where I initially found this problem) > - Another font / font size > - Graphic > > The reason is actually quite simple: The CSS is not (yet) applied initially, > we therefore ALWAYS take the default font into account + the graphic is not > yet layouted as well. > > _It was not so easy to write tests for this, also for me the > `test_resizeColumnToFitContentHeader` is always failing locally. I don't know > what happens here, but he seems to not find a (Stub?) `Font` for me._ > **EDIT: Found out the cause and fixed it. I will check if I can write more > tests since it works now. :)** > > The test I wrote now is checking if the css is applied after we triggered the > autosize, which is what we would expect here since we measure text. > > I also copied the `TableColumnHeaderTest` and rewrote the tests for > `TreeTableView` as well, so we can catch any errors here as well since they > both use different code (although it is technically the same - C errors can > happen very easy). Marius Hanl has updated the pull request incrementally with one additional commit since the last revision: improve tests + JUnit 5 for new test - Changes: - all: https://git.openjdk.org/jfx/pull/1405/files - new: https://git.openjdk.org/jfx/pull/1405/files/53acbc5c..b4d03d44 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1405=05 - incr: https://webrevs.openjdk.org/?repo=jfx=1405=04-05 Stats: 78 lines in 2 files changed: 32 ins; 16 del; 30 mod Patch: https://git.openjdk.org/jfx/pull/1405.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1405/head:pull/1405 PR: https://git.openjdk.org/jfx/pull/1405
Re: RFR: JDK-8186188: TableColumHeader: initial auto-size broken if has graphic [v5]
> This PR fixes the issue that the initial column autosizing is wrong under > some circumstances. > The following things will break the initial autosizing: > - Bold Column text (that is where I initially found this problem) > - Another font / font size > - Graphic > > The reason is actually quite simple: The CSS is not (yet) applied initially, > we therefore ALWAYS take the default font into account + the graphic is not > yet layouted as well. > > _It was not so easy to write tests for this, also for me the > `test_resizeColumnToFitContentHeader` is always failing locally. I don't know > what happens here, but he seems to not find a (Stub?) `Font` for me._ > **EDIT: Found out the cause and fixed it. I will check if I can write more > tests since it works now. :)** > > The test I wrote now is checking if the css is applied after we triggered the > autosize, which is what we would expect here since we measure text. > > I also copied the `TableColumnHeaderTest` and rewrote the tests for > `TreeTableView` as well, so we can catch any errors here as well since they > both use different code (although it is technically the same - C errors can > happen very easy). Marius Hanl has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains eight commits: - Merge branch 'master' of https://github.com/openjdk/jfx into JDK-8186188-tc-init-size # Conflicts: # modules/javafx.graphics/src/test/java/test/com/sun/javafx/pgstub/StubFontLoader.java - JDK-8186188: copyright - JDK-8186188: fix tests - JDK-8186188: add more tests, fix existing tests which were failing as the font is now always there - JDK-8186188: prefWidth(-1) for TreeTableView as well - JDK-8186188: fix tests by considering the system font (was always mixed with Amble) - JDK-8186188: Use label prefWidth(-1) instead of manually calculating the width - JDK-8186188: TableColumHeader: initial auto-size broken if has graphic - Changes: https://git.openjdk.org/jfx/pull/1405/files Webrev: https://webrevs.openjdk.org/?repo=jfx=1405=04 Stats: 501 lines in 4 files changed: 489 ins; 4 del; 8 mod Patch: https://git.openjdk.org/jfx/pull/1405.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1405/head:pull/1405 PR: https://git.openjdk.org/jfx/pull/1405
Re: RFR: 8327179: Update the 3D lighting application [v6]
On Wed, 27 Mar 2024 21:22:47 GMT, Nir Lisker wrote:
>> Update for the 3D lighting test tool as described in the JBS issue.
>
> Nir Lisker has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Whitespace?
build.gradle line 4049:
> 4047: project(":3DLighting") {
> 4048:
> 4049: apply plugin: "application"
I hadn't meant to suggest adding it to the main build.gradle. I think this is
not the best place for it. I was thinking of something along the lines of a
build.gradle or build.xml in the`tests/performance/3DLighting` dir which is
what the other manual tests do -- at least those that are more complicated than
just a collection of files in a single directory.
tests/system/src/test/.classpath line 43:
> 41:
> 42:
> 43:path="org.eclipse.jdt.junit.JUNIT_CONTAINER/5">
The changes in this file seem unrelated to your PR.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1387#discussion_r1542126594
PR Review Comment: https://git.openjdk.org/jfx/pull/1387#discussion_r1542127007
Integrated: JDK-8328750: [TestBug] Improve Stub Font Support
On Thu, 21 Mar 2024 22:06:42 GMT, Marius Hanl wrote: > In https://github.com/openjdk/jfx/pull/1405, I identified some shortcomings > of the stub font implementation. As I don't want to clutter the PR with that, > I decided to cherrypick the improvements I did to a new ticket and PR. > > The current implementation has the following shortcomings: > - It does not reliable detect the System Font, as a consequence, tests in > TableColumnHeaderTest.java are failing on my local machine > - Another consequence of this is, that the font size is always estimated with > 0, as it is not detected > - One shortcoming currently is, that the stub font siie estimate is not > considering bold fonts. That would improve writing tests for some scenarios, > e.g. for TableColumnHeader, where we would expect that the size of the header > is bigger since it is bold > > Some tests were failing for the following reasons: > - `AreaChartTest.java` - `expected -30.0, was -30.004` - I added > rounding to the data. > - `StackedBarChartTest.java` - since we now calculate correctly, the path > changed > - A test tried to load `Helvetica`, which is not supported in the stub font > loader. I changed it > - The default System font is considered a `Regular` one (style) - just like > in JavaFX > > I wrote tests and documented the stub behaviour. > I did some minor changes here: > - System font is now detected, also in bold and italic > - A bold font will be calculated with a little bit more width (1px). Checkout > the test as well for that > > Note: This only changes test setup, no 'production' code. This pull request has now been integrated. Changeset: 6adbcffa Author:Marius Hanl URL: https://git.openjdk.org/jfx/commit/6adbcffafd15f9f771c09afb03649e83e9e0b02a Stats: 359 lines in 8 files changed: 238 ins; 21 del; 100 mod 8328750: [TestBug] Improve Stub Font Support Reviewed-by: angorya - PR: https://git.openjdk.org/jfx/pull/1422
Re: RFR: JDK-8186188: TableColumHeader: initial auto-size broken if has graphic [v4]
On Wed, 20 Mar 2024 19:53:40 GMT, Marius Hanl wrote: >> This PR fixes the issue that the initial column autosizing is wrong under >> some circumstances. >> The following things will break the initial autosizing: >> - Bold Column text (that is where I initially found this problem) >> - Another font / font size >> - Graphic >> >> The reason is actually quite simple: The CSS is not (yet) applied initially, >> we therefore ALWAYS take the default font into account + the graphic is not >> yet layouted as well. >> >> _It was not so easy to write tests for this, also for me the >> `test_resizeColumnToFitContentHeader` is always failing locally. I don't >> know what happens here, but he seems to not find a (Stub?) `Font` for me._ >> **EDIT: Found out the cause and fixed it. I will check if I can write more >> tests since it works now. :)** >> >> The test I wrote now is checking if the css is applied after we triggered >> the autosize, which is what we would expect here since we measure text. >> >> I also copied the `TableColumnHeaderTest` and rewrote the tests for >> `TreeTableView` as well, so we can catch any errors here as well since they >> both use different code (although it is technically the same - C errors >> can happen very easy). > > Marius Hanl has updated the pull request incrementally with two additional > commits since the last revision: > > - JDK-8186188: copyright > - JDK-8186188: fix tests @effad Since you benchmarked this method in #1358, could you do that again with this changes? - PR Comment: https://git.openjdk.org/jfx/pull/1405#issuecomment-2024095127
Re: RFR: JDK-8328750: [TestBug] Improve Stub Font Support [v3]
On Wed, 27 Mar 2024 22:20:42 GMT, Marius Hanl wrote: >> In https://github.com/openjdk/jfx/pull/1405, I identified some shortcomings >> of the stub font implementation. As I don't want to clutter the PR with >> that, I decided to cherrypick the improvements I did to a new ticket and PR. >> >> The current implementation has the following shortcomings: >> - It does not reliable detect the System Font, as a consequence, tests in >> TableColumnHeaderTest.java are failing on my local machine >> - Another consequence of this is, that the font size is always estimated >> with 0, as it is not detected >> - One shortcoming currently is, that the stub font siie estimate is not >> considering bold fonts. That would improve writing tests for some scenarios, >> e.g. for TableColumnHeader, where we would expect that the size of the >> header is bigger since it is bold >> >> Some tests were failing for the following reasons: >> - `AreaChartTest.java` - `expected -30.0, was -30.004` - I added >> rounding to the data. >> - `StackedBarChartTest.java` - since we now calculate correctly, the path >> changed >> - A test tried to load `Helvetica`, which is not supported in the stub font >> loader. I changed it >> - The default System font is considered a `Regular` one (style) - just like >> in JavaFX >> >> I wrote tests and documented the stub behaviour. >> I did some minor changes here: >> - System font is now detected, also in bold and italic >> - A bold font will be calculated with a little bit more width (1px). >> Checkout the test as well for that >> >> Note: This only changes test setup, no 'production' code. > > Marius Hanl has updated the pull request incrementally with one additional > commit since the last revision: > > improve javadoc Marked as reviewed by angorya (Reviewer). - PR Review: https://git.openjdk.org/jfx/pull/1422#pullrequestreview-1964866535
Re: RFR: JDK-8328750: [TestBug] Improve Stub Font Support [v2]
On Tue, 26 Mar 2024 20:53:17 GMT, Andy Goryachev wrote: >> Marius Hanl has updated the pull request incrementally with one additional >> commit since the last revision: >> >> improve (stub) font tests, fallback and documentation > > modules/javafx.graphics/src/test/java/test/com/sun/javafx/pgstub/StubTextLayout.java > line 40: > >> 38: * >> 39: * Can calculate the bounds of text by simply using the size of the font. >> 40: * If the text is bold, the font will be 1 pixel bigger. > > maybe change to "somewhat wider" instead, since the height is the same? sure! Fun fact: I also was not happy with the javadoc but couldn't come up with a better wording. Fixed now. :) - PR Review Comment: https://git.openjdk.org/jfx/pull/1422#discussion_r1542106527
Re: RFR: JDK-8328750: [TestBug] Improve Stub Font Support [v3]
> In https://github.com/openjdk/jfx/pull/1405, I identified some shortcomings > of the stub font implementation. As I don't want to clutter the PR with that, > I decided to cherrypick the improvements I did to a new ticket and PR. > > The current implementation has the following shortcomings: > - It does not reliable detect the System Font, as a consequence, tests in > TableColumnHeaderTest.java are failing on my local machine > - Another consequence of this is, that the font size is always estimated with > 0, as it is not detected > - One shortcoming currently is, that the stub font siie estimate is not > considering bold fonts. That would improve writing tests for some scenarios, > e.g. for TableColumnHeader, where we would expect that the size of the header > is bigger since it is bold > > Some tests were failing for the following reasons: > - `AreaChartTest.java` - `expected -30.0, was -30.004` - I added > rounding to the data. > - `StackedBarChartTest.java` - since we now calculate correctly, the path > changed > - A test tried to load `Helvetica`, which is not supported in the stub font > loader. I changed it > - The default System font is considered a `Regular` one (style) - just like > in JavaFX > > I wrote tests and documented the stub behaviour. > I did some minor changes here: > - System font is now detected, also in bold and italic > - A bold font will be calculated with a little bit more width (1px). Checkout > the test as well for that > > Note: This only changes test setup, no 'production' code. Marius Hanl has updated the pull request incrementally with one additional commit since the last revision: improve javadoc - Changes: - all: https://git.openjdk.org/jfx/pull/1422/files - new: https://git.openjdk.org/jfx/pull/1422/files/f8909afc..1788ad68 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1422=02 - incr: https://webrevs.openjdk.org/?repo=jfx=1422=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/1422.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1422/head:pull/1422 PR: https://git.openjdk.org/jfx/pull/1422
Re: RFR: JDK-8328750: [TestBug] Improve Stub Font Support [v2]
On Tue, 26 Mar 2024 20:47:37 GMT, Marius Hanl wrote: >> In https://github.com/openjdk/jfx/pull/1405, I identified some shortcomings >> of the stub font implementation. As I don't want to clutter the PR with >> that, I decided to cherrypick the improvements I did to a new ticket and PR. >> >> The current implementation has the following shortcomings: >> - It does not reliable detect the System Font, as a consequence, tests in >> TableColumnHeaderTest.java are failing on my local machine >> - Another consequence of this is, that the font size is always estimated >> with 0, as it is not detected >> - One shortcoming currently is, that the stub font siie estimate is not >> considering bold fonts. That would improve writing tests for some scenarios, >> e.g. for TableColumnHeader, where we would expect that the size of the >> header is bigger since it is bold >> >> Some tests were failing for the following reasons: >> - `AreaChartTest.java` - `expected -30.0, was -30.004` - I added >> rounding to the data. >> - `StackedBarChartTest.java` - since we now calculate correctly, the path >> changed >> - A test tried to load `Helvetica`, which is not supported in the stub font >> loader. I changed it >> - The default System font is considered a `Regular` one (style) - just like >> in JavaFX >> >> I wrote tests and documented the stub behaviour. >> I did some minor changes here: >> - System font is now detected, also in bold and italic >> - A bold font will be calculated with a little bit more width (1px). >> Checkout the test as well for that >> >> Note: This only changes test setup, no 'production' code. > > Marius Hanl has updated the pull request incrementally with one additional > commit since the last revision: > > improve (stub) font tests, fallback and documentation looks good, one minor javadoc comment. if you decide to update, I'll re-approve. - Marked as reviewed by angorya (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/1422#pullrequestreview-1964856689
Re: RFR: 8092102: Labeled: truncated property [v6]
On Wed, 27 Mar 2024 21:54:37 GMT, Andy Goryachev wrote: >> Adds **Labeled.textTruncated** property which indicates when the text is >> visually truncated (and the ellipsis string is inserted) in order to fit the >> available width. >> >> The new property reacts to changes in the following properties: >> - ellipsisString >> - font >> - height >> - text >> - width >> - wrapText >> >> I don't think it's worth creating a headful test (headless won't work) due >> to relative simplicity of the code. >> >> **Alternative** >> >> The desired functionality can be just as easily achieved on an application >> level, by adding a similar property to a subclass. What is the benefit of >> adding this functionality to the core? >> >> UPDATE 2024/03/07: turns out Labeled in a TableView (in a TreeTableView as >> well) lives by different rules (TableCellSkinBase:152, >> TreeTableCellSkin:126). The consequence of this is that the new >> functionality **cannot** be fully implemented with the public APIs alone. >> >> **See Also** >> >> * [JDK-8327483](https://bugs.openjdk.org/browse/JDK-8327483) TreeView: Allow >> for tooltip when cell text is truncated >> * [JDK-8205211](https://bugs.openjdk.org/browse/JDK-8205211) Ability to show >> Tooltip only when text is shown with ellipsis (...) > > Andy Goryachev has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains ten commits: > > - test > - Merge remote-tracking branch 'origin/master' into 8092102.truncated > - Merge branch 'master' into 8092102.truncated > - labeled helper > - handle tree/table view cells > - Merge remote-tracking branch 'origin/master' into 8092102.truncated > - review comments > - Merge remote-tracking branch 'origin/master' into 8092102.truncated > - 8092102 Labeled: truncated property modules/javafx.controls/src/test/java/test/javafx/scene/control/LabeledTruncatedTest.java line 153: > 151: > 152: // FIX fails > 153: //assertEquals(Boolean.TRUE, truncated.get()); still WIP, I can't figure out why the table cells have width 0 - PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1542092066
Re: RFR: 8092102: Labeled: truncated property [v6]
> Adds **Labeled.textTruncated** property which indicates when the text is > visually truncated (and the ellipsis string is inserted) in order to fit the > available width. > > The new property reacts to changes in the following properties: > - ellipsisString > - font > - height > - text > - width > - wrapText > > I don't think it's worth creating a headful test (headless won't work) due to > relative simplicity of the code. > > **Alternative** > > The desired functionality can be just as easily achieved on an application > level, by adding a similar property to a subclass. What is the benefit of > adding this functionality to the core? > > UPDATE 2024/03/07: turns out Labeled in a TableView (in a TreeTableView as > well) lives by different rules (TableCellSkinBase:152, > TreeTableCellSkin:126). The consequence of this is that the new > functionality **cannot** be fully implemented with the public APIs alone. > > **See Also** > > * [JDK-8327483](https://bugs.openjdk.org/browse/JDK-8327483) TreeView: Allow > for tooltip when cell text is truncated > * [JDK-8205211](https://bugs.openjdk.org/browse/JDK-8205211) Ability to show > Tooltip only when text is shown with ellipsis (...) Andy Goryachev has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains ten commits: - test - Merge remote-tracking branch 'origin/master' into 8092102.truncated - Merge branch 'master' into 8092102.truncated - labeled helper - handle tree/table view cells - Merge remote-tracking branch 'origin/master' into 8092102.truncated - review comments - Merge remote-tracking branch 'origin/master' into 8092102.truncated - 8092102 Labeled: truncated property - Changes: https://git.openjdk.org/jfx/pull/1389/files Webrev: https://webrevs.openjdk.org/?repo=jfx=1389=05 Stats: 357 lines in 5 files changed: 329 ins; 19 del; 9 mod Patch: https://git.openjdk.org/jfx/pull/1389.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1389/head:pull/1389 PR: https://git.openjdk.org/jfx/pull/1389
Re: RFR: 8327179: Update the 3D lighting application [v5]
On Wed, 27 Mar 2024 20:38:58 GMT, Nir Lisker wrote: >> Update for the 3D lighting test tool as described in the JBS issue. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Added gradle script Looks like the line specified by jcheck was off. - PR Comment: https://git.openjdk.org/jfx/pull/1387#issuecomment-2024008625
Re: RFR: 8327179: Update the 3D lighting application [v6]
> Update for the 3D lighting test tool as described in the JBS issue. Nir Lisker has updated the pull request incrementally with one additional commit since the last revision: Whitespace? - Changes: - all: https://git.openjdk.org/jfx/pull/1387/files - new: https://git.openjdk.org/jfx/pull/1387/files/f063d088..ae471f2e Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1387=05 - incr: https://webrevs.openjdk.org/?repo=jfx=1387=04-05 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/1387.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1387/head:pull/1387 PR: https://git.openjdk.org/jfx/pull/1387
Re: RFR: 8327179: Update the 3D lighting application [v5]
On Wed, 27 Mar 2024 20:38:58 GMT, Nir Lisker wrote: >> Update for the 3D lighting test tool as described in the JBS issue. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Added gradle script I added the application to the gradle build file so it can be hooked up with its module dependencies. You can now produce a jar that bundles the resource with the classes. You still need to point to the modules at runtime. Let me know if this is a good enough solution. The way the project is configured in the main build file is not the best, but it will require large changes to do it better. Not sure what jcheck wants. It's complaining about empty spaces that have a `}` appear after them. This is the error: https://github.com/openjdk/jfx/pull/1387/files#annotation_19798318258. - PR Comment: https://git.openjdk.org/jfx/pull/1387#issuecomment-2023946848 PR Comment: https://git.openjdk.org/jfx/pull/1387#issuecomment-2023948839
Re: RFR: 8327179: Update the 3D lighting application [v5]
> Update for the 3D lighting test tool as described in the JBS issue. Nir Lisker has updated the pull request incrementally with one additional commit since the last revision: Added gradle script - Changes: - all: https://git.openjdk.org/jfx/pull/1387/files - new: https://git.openjdk.org/jfx/pull/1387/files/9b3f0c5e..f063d088 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1387=04 - incr: https://webrevs.openjdk.org/?repo=jfx=1387=03-04 Stats: 26 lines in 1 file changed: 26 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/1387.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1387/head:pull/1387 PR: https://git.openjdk.org/jfx/pull/1387
Re: RFR: 8242553: IntegerSpinner and DoubleSpinner do not wrap around values correctly in some cases
On Wed, 27 Mar 2024 19:05:40 GMT, Kevin Rushforth wrote:
> the number of discrete positions is `max - min + 1`
you are right! my earlier comment is invalid. please ignore.
Now, when wrapping occurs, should max/min be considered as valid steps?
Example, try both integer and double spinners with {min=0, max=10, step=7}
starting with value=0. try incrementing.
integer: 0, 7, 3, 10
double: 0, 7, 10, 0, 7, 10
And another question: should the new behavior (whatever everyone agrees to
eventually) be documented? Where? This PR? JBS? Javadoc? A markdown file
in doc-files/behavior/?
Does it need a CSR?
-
PR Comment: https://git.openjdk.org/jfx/pull/1431#issuecomment-2023788836
Re: RFR: 8242553: IntegerSpinner and DoubleSpinner do not wrap around values correctly in some cases
On Wed, 27 Mar 2024 18:30:24 GMT, Andy Goryachev wrote: > the end result of stepping once by `amountToStepBy` must be equivalent of > incrementing by one a total of `amountToStepBy` times, wouldn't you agree? Yes, this is the expectaion. > so the formula for positive `amountToStepBy` values should look like > > `((val + amountToStepBy) % (max - min)) + min` > > or something along these lines. Not quite. Because the range is inclusive on both ends, the number of discrete positions is `max - min + 1`. In your above example , where `min=0` and `max=100`, there are 101 discrete positions. A single step by 101 or 101 steps by 1 will both bring you back to the same position you started from. - PR Comment: https://git.openjdk.org/jfx/pull/1431#issuecomment-2023752621
Re: RFR: 8242553: IntegerSpinner and DoubleSpinner do not wrap around values correctly in some cases
On Sun, 24 Mar 2024 15:11:16 GMT, drmarmac wrote: > This PR should fix the issue and cover all relevant cases with new tests. > > Note: This involves a small behavior change, as can be seen in > dblSpinner_testWrapAround_decrement_twoSteps() in SpinnerTest.java:749. With > this change the wraparound behavior is similar to that of the IntegerSpinner. the end result of stepping once by `amountToStepBy` must be equivalent of incrementing by one a total of `amountToStepBy` times, wouldn't you agree? so the formula for positive `amountToStepBy` values should look like `((val + amountToStepBy) % (max - min)) + min` or something along these lines. - PR Comment: https://git.openjdk.org/jfx/pull/1431#issuecomment-2023596265
Re: RFR: 8242553: IntegerSpinner and DoubleSpinner do not wrap around values correctly in some cases
On Sun, 24 Mar 2024 15:11:16 GMT, drmarmac wrote: > This PR should fix the issue and cover all relevant cases with new tests. > > Note: This involves a small behavior change, as can be seen in > dblSpinner_testWrapAround_decrement_twoSteps() in SpinnerTest.java:749. With > this change the wraparound behavior is similar to that of the IntegerSpinner. The code, as currently suggested, effectively calculates `x(i+1) = (x(i) + amountToStepBy) MOD (max - min + 1)`. I'd say this is the modulo value we actually want, because x=min and x=max should both be included. Look at a simpler example, which would not work with a `MOD (max-min)` formula: min=0; max=2; amountToStepBy=1, initial=0 => 0, 1, 2, 0, 1, ... min=0; max=2; amountToStepBy=2, initial=0 => 0, 2, 0, 2, 0, ... min=0; max=2; amountToStepBy=3, initial=0 => 0, 0, 0, 0, 0, ... min=0; max=2; amountToStepBy=4, initial=0 => 0, 1, 2, 0, 1, ... So your example should result in `min=0; max=100; amountToStepBy=101, initial=0 => 0, 0, 0, 0...`, which it currently does. - PR Comment: https://git.openjdk.org/jfx/pull/1431#issuecomment-2023513947
Re: RFR: 8242553: IntegerSpinner and DoubleSpinner do not wrap around values correctly in some cases
On Sun, 24 Mar 2024 15:11:16 GMT, drmarmac wrote: > This PR should fix the issue and cover all relevant cases with new tests. > > Note: This involves a small behavior change, as can be seen in > dblSpinner_testWrapAround_decrement_twoSteps() in SpinnerTest.java:749. With > this change the wraparound behavior is similar to that of the IntegerSpinner. As a user, I would be very surprised with the behavior based on modulo arithmetic. But I do acknowledge the point made by Kevin - if this is what the application developers wanted, then it is their fault. - PR Comment: https://git.openjdk.org/jfx/pull/1431#issuecomment-2023260253
Re: RFR: 8325900: Emit a warning on macOS if AWT has set the NSAppearance [v4]
On Fri, 23 Feb 2024 21:58:37 GMT, Michael Strauß wrote: >> Platform preferences detection doesn't pick up effective macOS system >> preferences if AWT owns the NSApplication and has set its NSAppearance to a >> fixed value. >> >> The workaround is to set the system property >> "apple.awt.application.appearance=system". >> >> If this property is not set, the following warning will be emitted if a >> JavaFX application attempts to use the platform preferences API: >> >> >> WARNING: Reported preferences may not reflect the macOS system preferences >> unless the sytem >> property apple.awt.application.appearance=system is set. This warning can be >> disabled by >> setting javafx.preferences.suppressAppleAwtWarning=true. > > Michael Strauß has updated the pull request incrementally with one additional > commit since the last revision: > > Break warning message into separate lines I can review this. - PR Comment: https://git.openjdk.org/jfx/pull/1367#issuecomment-2023250420
Re: RFR: 8242553: IntegerSpinner and DoubleSpinner do not wrap around values correctly in some cases
On Wed, 27 Mar 2024 15:17:29 GMT, Andy Goryachev wrote: > (0 + 101) % (100 - 0) + 0 = 1 > > the code in this PR produces no movement (0). Same for the decrement. > which means that the modulo arithmetic is not yet quite correct (_not_ that the modulo behavior as such is wrong ;) nice test case, btw ;) - PR Comment: https://git.openjdk.org/jfx/pull/1431#issuecomment-2023222179
Re: RFR: 8242553: IntegerSpinner and DoubleSpinner do not wrap around values correctly in some cases
On Sun, 24 Mar 2024 15:11:16 GMT, drmarmac wrote:
> This PR should fix the issue and cover all relevant cases with new tests.
>
> Note: This involves a small behavior change, as can be seen in
> dblSpinner_testWrapAround_decrement_twoSteps() in SpinnerTest.java:749. With
> this change the wraparound behavior is similar to that of the IntegerSpinner.
If everyone is ok with modulo arithmetic.
Please clarify the expected behavior in the case of an integer spinner with
{min=0; max=100; amountToStepBy=101} and initial value of 0. What should the
increment action result in?
(0 + 101) % (100 - 0) + 0 = 1
the code in this PR produces no movement (0). Same for the decrement.
Is this correct?
-
PR Comment: https://git.openjdk.org/jfx/pull/1431#issuecomment-2023034904
Re: RFR: 8328746: Remove unused imports in demo apps
On Thu, 21 Mar 2024 21:50:37 GMT, Andy Goryachev wrote: > Using Eclipse IDE to remove unused imports in **demo apps** (3D, Ensemble, > etc.) and update the copyright year to 2024. Using wildcard for more than 10 > static imports. > > > -- > > This is a trivial change (though fairly large), 1 reviewer is probably enough. I would highly suggest to use the current guidelines in CONTRIBUTING.md * Don't worry too much about import order. Try not to change it but don't worry about fighting your IDE to stop it from doing so. as not doing so generates unnecessary (in my opinion) discussion. - PR Comment: https://git.openjdk.org/jfx/pull/1420#issuecomment-2022971911
Re: RFR: 8328746: Remove unused imports in demo apps
On Thu, 21 Mar 2024 21:50:37 GMT, Andy Goryachev wrote: > Using Eclipse IDE to remove unused imports in **demo apps** (3D, Ensemble, > etc.) and update the copyright year to 2024. Using wildcard for more than 10 > static imports. > > > -- > > This is a trivial change (though fairly large), 1 reviewer is probably enough. I find it helpful to separate the imports by their high-level domain name, "java.", "javafx.", "com." etc. It makes it easier to see the "span" or requirements of the class. - PR Comment: https://git.openjdk.org/jfx/pull/1420#issuecomment-2022856424
Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v2]
On Wed, 27 Mar 2024 09:11:56 GMT, drmarmac wrote: >> This PR removes potentially incorrect usages of Stream.peek(). >> The changed code should be covered by the tests that are already present. > > drmarmac has updated the pull request incrementally with one additional > commit since the last revision: > > Remove outdated comment modules/javafx.controls/src/main/java/javafx/scene/control/MultipleSelectionModelBase.java line 773: > 771: .collect(Collectors.toList()); > 772: > 773: sortedNewIndices.forEach(this::set); Why do the double-iteration pattern here and not do the `peek` operation in a `forEach` like in the other 2 places? - PR Review Comment: https://git.openjdk.org/jfx/pull/1430#discussion_r1541157874
Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v2]
On Wed, 27 Mar 2024 12:23:47 GMT, Marius Hanl wrote: >> I'd say .forEach() is used correctly here, according to docs, it guarantees >> execution of the side-effects (add to removed list & clear index), just not >> in any particular order. This way we avoid multiple iteration. > > Another idea is to use `toList()`, which is a very efficient operation and > then iterate over it. > In the java.util.stream package > [docs](https://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/util/stream/package-summary.html#SideEffects) > it is mentioned that `forEach()` method operates only via side-effects. So > do you think we should avoid using `forEach()` here and iterate the generated > list separately to clear selected index? `forEach` is used correctly here. From the docs: > With the exception of terminal operations > [forEach](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/util/stream/Stream.html#forEach(java.util.function.Consumer)) > and > [forEachOrdered](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/util/stream/Stream.html#forEachOrdered(java.util.function.Consumer)), > side-effects of behavioral parameters may not always be executed when the > stream implementation can optimize away the execution of behavioral > parameters without affecting the result of the computation. > Another idea is to use `toList()`, which is a very efficient operation and > then iterate over it. That's still 2 iterations. If the code is not performance-critical it doesn't matter. - PR Review Comment: https://git.openjdk.org/jfx/pull/1430#discussion_r1541140317
Re: RFR: 8242553: IntegerSpinner and DoubleSpinner do not wrap around values correctly in some cases
On Sun, 24 Mar 2024 15:11:16 GMT, drmarmac wrote: > This PR should fix the issue and cover all relevant cases with new tests. > > Note: This involves a small behavior change, as can be seen in > dblSpinner_testWrapAround_decrement_twoSteps() in SpinnerTest.java:749. With > this change the wraparound behavior is similar to that of the IntegerSpinner. Let's proceed with the current plan of correctly-implemented modulo arithmetic. I can't think of a compelling reason to do otherwise. - PR Comment: https://git.openjdk.org/jfx/pull/1431#issuecomment-2022700341
Integrated: 8306322: JDK8130122Test fails intermittently
On Mon, 25 Mar 2024 12:25:33 GMT, Jayathirth D V wrote: > This test has failed once and we are not seeing its failure after that > instance in our test systems. > > This test verifies whether bounds of GridPane gets updated properly on adding > an invisible node. > Initial test has 8 nodes in GridPane and then we update it with another node > with larger bounds without making it visible. After adding additional node we > make the scenegraph visible and check for colors of the newly added node. > > We are making this test robust to make sure we don't see any of these single > instance failures. > Test is updated to: > 1) To always show on top, so that we pick proper color. > 2) Add additional sleep so that we give more time for test to be visible. > 3) Pick color exactly at mid point in y axis, so that we pick the green color > properly. This pull request has now been integrated. Changeset: 0541f371 Author:Jayathirth D V Committer: Kevin Rushforth URL: https://git.openjdk.org/jfx/commit/0541f37179ff4a672a40f3c4976e6019b8ecf7c2 Stats: 4 lines in 1 file changed: 1 ins; 0 del; 3 mod 8306322: JDK8130122Test fails intermittently Reviewed-by: kcr, angorya - PR: https://git.openjdk.org/jfx/pull/1433
Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v2]
On Wed, 27 Mar 2024 09:07:15 GMT, drmarmac wrote:
>> modules/javafx.controls/src/main/java/javafx/scene/control/ControlUtils.java
>> line 176:
>>
>>> 174: .distinct()
>>> 175: .filter(removeRowFilter)
>>> 176: .forEach(row -> {
>>
>> In the java.util.stream package
>> [docs](https://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/util/stream/package-summary.html#SideEffects)
>> it is mentioned that `forEach()` method operates only via side-effects. So
>> do you think we should avoid using `forEach()` here and iterate the
>> generated list separately to clear selected index?
>
> I'd say .forEach() is used correctly here, according to docs, it guarantees
> execution of the side-effects (add to removed list & clear index), just not
> in any particular order. This way we avoid multiple iteration.
Another idea is to use `toList()`, which is a very efficient operation and then
iterate over it.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1430#discussion_r1541003650
Integrated: 8267565: Support "@3x" and greater high-density image naming convention
On Fri, 22 Mar 2024 16:17:29 GMT, drmarmac wrote: > This PR extends the range of hi-res images that are loaded via naming > convention, now including scale factors higher than `@2x`. > Supporting these is already being > [recommended](https://developer.apple.com/design/human-interface-guidelines/images#Best-practices) > for some platforms. > > I tested this manually on Windows with "300%" UI scale factor, and verified > `@2x` still works on macOS. This pull request has now been integrated. Changeset: 5d886f82 Author:drmarmac <6900949+drmar...@users.noreply.github.com> Committer: Michael Strauß URL: https://git.openjdk.org/jfx/commit/5d886f82260ee508c0da2dfee5d3ace1a199a675 Stats: 50 lines in 5 files changed: 19 ins; 0 del; 31 mod 8267565: Support "@3x" and greater high-density image naming convention Reviewed-by: kcr, mstrauss - PR: https://git.openjdk.org/jfx/pull/1429
Re: RFR: 8242553: IntegerSpinner and DoubleSpinner do not wrap around values correctly in some cases
On Sun, 24 Mar 2024 15:11:16 GMT, drmarmac wrote: > This PR should fix the issue and cover all relevant cases with new tests. > > Note: This involves a small behavior change, as can be seen in > dblSpinner_testWrapAround_decrement_twoSteps() in SpinnerTest.java:749. With > this change the wraparound behavior is similar to that of the IntegerSpinner. afair, we already had extensive discussions in https://github.com/openjdk/jfx/pull/174 (and the related bug and the mailing list - too lazy to read up on all that) agreeing that modulo math (correctly done!) fits common client programmers' expectations most - doesn't matter the step size. - PR Comment: https://git.openjdk.org/jfx/pull/1431#issuecomment-2022504622
Re: RFR: 8267565: Support "@3x" and greater high-density image naming convention [v2]
On Tue, 26 Mar 2024 21:27:49 GMT, drmarmac wrote: >> This PR extends the range of hi-res images that are loaded via naming >> convention, now including scale factors higher than `@2x`. >> Supporting these is already being >> [recommended](https://developer.apple.com/design/human-interface-guidelines/images#Best-practices) >> for some platforms. >> >> I tested this manually on Windows with "300%" UI scale factor, and verified >> `@2x` still works on macOS. > > drmarmac has updated the pull request incrementally with one additional > commit since the last revision: > > Change name2x to nameScaled Looks good to me. - Marked as reviewed by mstrauss (Committer). PR Review: https://git.openjdk.org/jfx/pull/1429#pullrequestreview-1962957040
Re: RFR: 8242553: IntegerSpinner and DoubleSpinner do not wrap around values correctly in some cases
On Sun, 24 Mar 2024 15:11:16 GMT, drmarmac wrote: > This PR should fix the issue and cover all relevant cases with new tests. > > Note: This involves a small behavior change, as can be seen in > dblSpinner_testWrapAround_decrement_twoSteps() in SpinnerTest.java:749. With > this change the wraparound behavior is similar to that of the IntegerSpinner. I don't see that much value in any threshold like `step >= max-min`, no matter what the behavior beyond this threshold would be. It would be almost as weird already for less extreme cases: Take `min=0, max=100, step=99, initial=50`, leading to `50-49-48..`, which even appears to move backwards. Not sure if we can come up with any adjustments that cover all cases nicely - as an app developer I'd prefer either good logic that always works well, or simple logic that is easy to override (which it would be here). - PR Comment: https://git.openjdk.org/jfx/pull/1431#issuecomment-2022298388
Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v2]
On Tue, 26 Mar 2024 16:32:53 GMT, Karthik P K wrote:
>> drmarmac has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Remove outdated comment
>
> modules/javafx.controls/src/main/java/javafx/scene/control/ControlUtils.java
> line 176:
>
>> 174: .distinct()
>> 175: .filter(removeRowFilter)
>> 176: .forEach(row -> {
>
> In the java.util.stream package
> [docs](https://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/util/stream/package-summary.html#SideEffects)
> it is mentioned that `forEach()` method operates only via side-effects. So
> do you think we should avoid using `forEach()` here and iterate the generated
> list separately to clear selected index?
I'd say .forEach() is used correctly here, according to docs, it guarantees
execution of the side-effects (add to removed list & clear index), just not in
any particular order. This way we avoid multiple iteration.
> modules/javafx.controls/src/main/java/javafx/scene/control/ControlUtils.java
> line 185:
>
>> 183: .mapToInt(TablePositionBase::getRow)
>> 184: .distinct()
>> 185: .forEach(row -> {
>
> Similar comment as above. Here if we do not use `forEach()` on streams, we
> can also avoid using array of size one for keeping count as well right?
We'd need to iterate twice as well (select index & count), with forEach it's
just once.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1430#discussion_r1540712652
PR Review Comment: https://git.openjdk.org/jfx/pull/1430#discussion_r1540712756
Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v2]
> This PR removes potentially incorrect usages of Stream.peek(). > The changed code should be covered by the tests that are already present. drmarmac has updated the pull request incrementally with one additional commit since the last revision: Remove outdated comment - Changes: - all: https://git.openjdk.org/jfx/pull/1430/files - new: https://git.openjdk.org/jfx/pull/1430/files/676ec3cf..1b285b5b Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1430=01 - incr: https://webrevs.openjdk.org/?repo=jfx=1430=00-01 Stats: 4 lines in 1 file changed: 0 ins; 4 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/1430.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1430/head:pull/1430 PR: https://git.openjdk.org/jfx/pull/1430
Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed
On Sun, 24 Mar 2024 15:10:22 GMT, drmarmac wrote: > This PR removes potentially incorrect usages of Stream.peek(). > The changed code should be covered by the tests that are already present. I also removed a code comment that explained the usage of peek() here, which would be outdated now. - PR Comment: https://git.openjdk.org/jfx/pull/1430#issuecomment-2022265808
Re: RFR: 8328746: Remove unused imports in demo apps
On Tue, 26 Mar 2024 16:32:41 GMT, Kevin Rushforth wrote: > static imports (sorted alphabetically) > one blank line > ordinary imports (sorted alphabetically) > > No wildcard imports (I favor an exception for static imports). +1 for something like this. It is a simple rule and pretty sure that it works for all IDEs. Swing is also ordered alphabetically, but with groups usually. Otherwise it looks the order in Swing is the other way around: ordinary imports (sorted alphabetically) one blank line static imports (sorted alphabetically) For me, both are good and I understand and agree to have some king of guideline. That will make reviewing all the import PRs easier as well. Also +1 for no wildcard imports other than static imports. - PR Comment: https://git.openjdk.org/jfx/pull/1420#issuecomment-2022108806
Re: RFR: JDK-8328750: [TestBug] Improve Stub Font Support [v2]
On Tue, 26 Mar 2024 20:36:02 GMT, Andy Goryachev wrote: >> I can also use `round` here. Seems to be a bit safer, I agree. > > thank you. > > one more question - is it possible to set fractional scale with the > StubToolkit, and how would that work in the StubFontLoader? Does it mean > we'll be rounding the scaled values? You mean for the font size? That will as far as I can see just work, although not 100% sure what the rendering, e.g. a Label will do with it - PR Review Comment: https://git.openjdk.org/jfx/pull/1422#discussion_r1540576884
