Integrated: 8277756: DatePicker listener might not be added when using second constructor
On Tue, 24 May 2022 21:35:15 GMT, Marius Hanl wrote: > The `valueProperty()` and `chronologyProperty()` listener are now added in > the second constructor of `DatePicker` > (`public DatePicker(LocalDate localDate)`) instead of the first one (`public > DatePicker()`). > Therefore, both listener are now added no matter which constructor is used. This pull request has now been integrated. Changeset: da5bd371 Author:Marius Hanl Committer: Ajit Ghaisas URL: https://git.openjdk.java.net/jfx/commit/da5bd3716441f201691377c52abba59ce03bc322 Stats: 116 lines in 2 files changed: 94 ins; 19 del; 3 mod 8277756: DatePicker listener might not be added when using second constructor Reviewed-by: aghaisas - PR: https://git.openjdk.java.net/jfx/pull/801
RFR: 8277756: DatePicker listener might not be added when using second constructor
The `valueProperty()` and `chronologyProperty()` listener are now added in the second constructor of `DatePicker` (`public DatePicker(LocalDate localDate)`) instead of the first one (`public DatePicker()`). Therefore, both listener are now added no matter which constructor is used. - Commit messages: - 8277756: DatePicker listener are now always added regardless of which constructor was used Changes: https://git.openjdk.java.net/jfx/pull/801/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=801=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8277756 Stats: 116 lines in 2 files changed: 94 ins; 19 del; 3 mod Patch: https://git.openjdk.java.net/jfx/pull/801.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/801/head:pull/801 PR: https://git.openjdk.java.net/jfx/pull/801
Re: RFR: 8218826: TableRowSkinBase: horizontal layout broken if row has padding
On Tue, 24 May 2022 21:25:23 GMT, Marius Hanl wrote:
> This PR fixes a problem, where the layout is broken when a `(Tree)TableRow`
> has padding.
> As also mentioned in the ticket, the `layoutChildren` method in
> `TableRowSkinBase` is implemented wrong.
>
> The `layoutChildren` method is responsible for layouting all the
> `(Tree)TableCells`.
> When the row has padding, it is subtracted on every `(Tree)TableCell` - which
> is wrong.
>
> Instead the `x` and `y` from `layoutChildren` should be used. (E.g. if `x` is
> 10, then only the first cell should start at 10 and the other cells follow as
> usual)
> Also the `compute...` methods needs to add the padding as well.
>
> **Example:**
> _Row padding left right 0:_
> [Cell1][Cell2][Cell3]
> _Row padding left right 10:_
> [10][Cell1][Cell2][Cell3][10]
> _Same for top bottom._
>
> When a `fixedCellSize` is set, the padding is currently ignored (also after
> this PR).
> Therefore, `y` in the `layoutChildren` method is set to 0 for `fixedCellSize`.
>
> This may can be discussed in the mailing list (Could be a follow up). To
> support padding also when a `fixedCellSize` is set, the `compute...` methods
> needs to also add the padding when a `fixedCellSize` is set (in the `if`
> clauses) and the `VirtualFlow` needs to add the padding to every row instead
> of using the `fixedCellSize` directly.
modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableRowSkinTest.java
line 255:
> 253: private void removedColumnsShouldRemoveCorrespondingCellsInRowImpl()
> {
> 254: // Remove the last 2 columns.
> 255: tableView.getColumns().remove(tableView.getColumns().size() - 2,
> tableView.getColumns().size());
Note: I added this test in PR: https://github.com/openjdk/jfx/pull/444. While
testing this PR I found out that this removes just one column, therefore I
fixed it right away.
-
PR: https://git.openjdk.java.net/jfx/pull/800
RFR: 8218826: TableRowSkinBase: horizontal layout broken if row has padding
This PR fixes a problem, where the layout is broken when a `(Tree)TableRow` has padding. As also mentioned in the ticket, the `layoutChildren` method in `TableRowSkinBase` is implemented wrong. The `layoutChildren` method is responsible for layouting all the `(Tree)TableCells`. When the row has padding, it is subtracted on every `(Tree)TableCell` - which is wrong. Instead the `x` and `y` from `layoutChildren` should be used. (E.g. if `x` is 10, then only the first cell should start at 10 and the other cells follow as usual) Also the `compute...` methods needs to add the padding as well. **Example:** _Row padding left right 0:_ [Cell1][Cell2][Cell3] _Row padding left right 10:_ [10][Cell1][Cell2][Cell3][10] _Same for top bottom._ When a `fixedCellSize` is set, the padding is currently ignored (also after this PR). Therefore, `y` in the `layoutChildren` method is set to 0 for `fixedCellSize`. This may can be discussed in the mailing list (Could be a follow up). To support padding also when a `fixedCellSize` is set, the `compute...` methods needs to also add the padding when a `fixedCellSize` is set (in the `if` clauses) and the `VirtualFlow` needs to add the padding to every row instead of using the `fixedCellSize` directly. - Commit messages: - 8218826: fixed horizontal layout broken if row has padding Changes: https://git.openjdk.java.net/jfx/pull/800/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=800=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8218826 Stats: 439 lines in 3 files changed: 417 ins; 10 del; 12 mod Patch: https://git.openjdk.java.net/jfx/pull/800.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/800/head:pull/800 PR: https://git.openjdk.java.net/jfx/pull/800
Re: RFR: 8285197: TableColumnHeader: calc of cell width must respect row styling (TreeTableView) [v2]
On Thu, 19 May 2022 14:23:55 GMT, Robert Lichtenberger wrote: >> Separate test class added for TreeTableView case. >> Fix is analogous to JDK-8251480. > > Robert Lichtenberger has updated the pull request incrementally with two > additional commits since the last revision: > > - 8285197: TableColumnHeader: calc of cell width must respect row styling > (TreeTableView) > >Test class cosmetic cleanups #2. > - 8285197: TableColumnHeader: calc of cell width must respect row styling > (TreeTableView) > >Test class cosmetic cleanups. Marked as reviewed by mhanl (Author). - PR: https://git.openjdk.java.net/jfx/pull/779
Integrated: 8286552: TextFormatter: UpdateValue/UpdateText is called, when no ValueConverter is set
On Tue, 10 May 2022 20:03:08 GMT, Marius Hanl wrote:
> A common reason for using the `TextFormatter` is the need for a `filter`.
> Therefore, the following constructor is typically used:
> `public TextFormatter(@NamedArg("filter") UnaryOperator filter) { ...
> }`
>
> With that, no `valueConverter` is set in the `TextFormatter`.
>
> When a `TextField` will commit his value, `TextFormatter.updateValue(...)` is
> called.
> Since `valueConverter` is null, an NPE is thrown and catched inside it and
> `TextFormatter.updateText()` is called (which will call
> `TextInputControl.updateText(...)`), which won't do anything either since
> `valueConverter` is still not set (=null).
>
> A minor improvement here is to just skip `TextFormatter.updateValue(...)` and
> therefore `TextFormatter.updateText()` (->
> `TextInputControl.updateText(...)`), since this methods won't do anything
> without a `valueConverter`.
> With that change, no exception (NPE) is thrown and catched, and therefore
> also exception breakpoints are not called (can be annoying ;)).
> This will also slightly increase the performance, as throwing exceptions is a
> (minor) bottleneck.
>
> Added tests for all `TextFormatter` functionalities, which pass before and
> after.
This pull request has now been integrated.
Changeset: 81e1cc3e
Author:Marius Hanl
Committer: Kevin Rushforth
URL:
https://git.openjdk.java.net/jfx/commit/81e1cc3edcbfda34479ee876cbd9eb74099a7e57
Stats: 78 lines in 2 files changed: 75 ins; 0 del; 3 mod
8286552: TextFormatter: UpdateValue/UpdateText is called, when no
ValueConverter is set
Reviewed-by: kcr
-
PR: https://git.openjdk.java.net/jfx/pull/794
Re: RFR: 8286552: TextFormatter: UpdateValue/UpdateText is called, when no ValueConverter is set [v2]
On Thu, 12 May 2022 22:13:38 GMT, Kevin Rushforth wrote:
>> Marius Hanl has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8286552: Added space and revert typo fix
>
> modules/javafx.controls/src/main/java/javafx/scene/control/TextFormatter.java
> line 40:
>
>> 38: *
>> 39: * A filter ({@link #getFilter()}) that can intercept and modify
>> user input. This helps to keep the text
>> 40: * in the desired format. A default text supplier can be used to
>> provide the initial text.
>
> I know this is a simple typo, but it is unrelated to your bug fix, and is in
> public API docs, so I'd like to see it go in separately under a "Fix mistakes
> in docs" bug. I filed
> [JDK-8286678](https://bugs.openjdk.java.net/browse/JDK-8286678) to track this
> and any other such issues that arise (as we've done for most recent releases).
Ah okay, alright. I reverted it.
> modules/javafx.controls/src/main/java/javafx/scene/control/TextFormatter.java
> line 202:
>
>> 200:
>> 201: void updateValue(String text) {
>> 202: if (valueConverter != null &&!value.isBound()) {
>
> Minor: please add a space between the `&&` and `!` operators.
done.
-
PR: https://git.openjdk.java.net/jfx/pull/794
Re: RFR: 8286552: TextFormatter: UpdateValue/UpdateText is called, when no ValueConverter is set [v2]
> A common reason for using the `TextFormatter` is the need for a `filter`.
> Therefore, the following constructor is typically used:
> `public TextFormatter(@NamedArg("filter") UnaryOperator filter) { ...
> }`
>
> With that, no `valueConverter` is set in the `TextFormatter`.
>
> When a `TextField` will commit his value, `TextFormatter.updateValue(...)` is
> called.
> Since `valueConverter` is null, an NPE is thrown and catched inside it and
> `TextFormatter.updateText()` is called (which will call
> `TextInputControl.updateText(...)`), which won't do anything either since
> `valueConverter` is still not set (=null).
>
> A minor improvement here is to just skip `TextFormatter.updateValue(...)` and
> therefore `TextFormatter.updateText()` (->
> `TextInputControl.updateText(...)`), since this methods won't do anything
> without a `valueConverter`.
> With that change, no exception (NPE) is thrown and catched, and therefore
> also exception breakpoints are not called (can be annoying ;)).
> This will also slightly increase the performance, as throwing exceptions is a
> (minor) bottleneck.
>
> Added tests for all `TextFormatter` functionalities, which pass before and
> after.
Marius Hanl has updated the pull request incrementally with one additional
commit since the last revision:
8286552: Added space and revert typo fix
-
Changes:
- all: https://git.openjdk.java.net/jfx/pull/794/files
- new: https://git.openjdk.java.net/jfx/pull/794/files/a171c370..bb157725
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jfx=794=01
- incr: https://webrevs.openjdk.java.net/?repo=jfx=794=00-01
Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
Patch: https://git.openjdk.java.net/jfx/pull/794.diff
Fetch: git fetch https://git.openjdk.java.net/jfx pull/794/head:pull/794
PR: https://git.openjdk.java.net/jfx/pull/794
RFR: 8286552: TextFormatter: UpdateValue/UpdateText is called, when no ValueConverter is set
A common reason for using the `TextFormatter` is the need for a `filter`.
Therefore, the following constructor is typically used:
`public TextFormatter(@NamedArg("filter") UnaryOperator filter) { ... }`
With that, no `valueConverter` is set in the `TextFormatter`.
When a `TextField` will commit his value, `TextFormatter.updateValue(...)` is
called.
Since `valueConverter` is null, an NPE is thrown and catched inside it and
`TextFormatter.updateText()` is called (which will call
`TextInputControl.updateText(...)`), which won't do anything either since
`valueConverter` is still not set (=null).
A minor improvement here is to just skip `TextFormatter.updateValue(...)` and
therefore `TextFormatter.updateText()`, since this methods won't do anything
without a `valueConverter`.
With that change, no exception is thrown and catched, and therefore also
exception breakpoints are not called.
This will also slightly increase the performance, as throwing exceptions is a
(minor) bottleneck.
-
Commit messages:
- 8286552: TextFormatter.updateValue(...) won't call updateText(), when no
valueConverter is set
Changes: https://git.openjdk.java.net/jfx/pull/794/files
Webrev: https://webrevs.openjdk.java.net/?repo=jfx=794=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8286552
Stats: 79 lines in 2 files changed: 75 ins; 0 del; 4 mod
Patch: https://git.openjdk.java.net/jfx/pull/794.diff
Fetch: git fetch https://git.openjdk.java.net/jfx pull/794/head:pull/794
PR: https://git.openjdk.java.net/jfx/pull/794
Re: RFR: 8285197: TableColumnHeader: calc of cell width must respect row styling (TreeTableView)
On Thu, 21 Apr 2022 08:38:20 GMT, Robert Lichtenberger wrote: > Separate test class added for TreeTableView case. > Fix is analogous to JDK-8251480. Looks good! Verified that the fix works. I can also confirm, that this fix is the same as previously done for `TableView` in PR: https://github.com/openjdk/jfx/pull/757 - Marked as reviewed by mhanl (Author). PR: https://git.openjdk.java.net/jfx/pull/779
Re: RFR: 8251480: TableColumnHeader: calc of cell width must respect row styling
On Tue, 19 Apr 2022 06:06:25 GMT, Robert Lichtenberger wrote: > Is there a pre-existing issue for this? Otherwise I'll try and come up with > an example and create a new issue... I don't think so, you may need to create a new issue. I think it's pretty much the same problem and solution as this ticket was, so you can probably copy some information. - PR: https://git.openjdk.java.net/jfx/pull/757
Re: RFR: 8251480: TableColumnHeader: calc of cell width must respect row styling
On Wed, 16 Mar 2022 13:01:45 GMT, Robert Lichtenberger wrote: >> Might make sense to also adjust the TreeTableView sizing implementation? > >> Might make sense to also adjust the TreeTableView sizing implementation? > > Yes I think that may be a good idea. True to the idea of specific, narrow > commits I've not integrated this into this PR but would be willing to open a > new issue / produce a separate PR for TreeTableView. @effad Do you have time and interest to take a look at this for the `TreeTableView` as well? Just asking as otherwise I will have a look :) - PR: https://git.openjdk.java.net/jfx/pull/757
Re: RFR: 8251480: TableColumnHeader: calc of cell width must respect row styling [v4]
On Wed, 30 Mar 2022 12:22:23 GMT, Robert Lichtenberger wrote: >> This fix respects a row factory, if present. >> It will put the cell that is used to measure the column width as child below >> the row. >> In that way the row's style will be used. > > Robert Lichtenberger has updated the pull request incrementally with one > additional commit since the last revision: > > 8251480: TableColumnHeader: calc of cell width must respect row styling > > Assertion removed. Looks good to me. - Marked as reviewed by mhanl (Author). PR: https://git.openjdk.java.net/jfx/pull/757
Re: RFR: 8251480: TableColumnHeader: calc of cell width must respect row styling [v2]
On Thu, 24 Mar 2022 06:14:42 GMT, Robert Lichtenberger
wrote:
>> This fix respects a row factory, if present.
>> It will put the cell that is used to measure the column width as child below
>> the row.
>> In that way the row's style will be used.
>
> Robert Lichtenberger has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8251480: TableColumnHeader: calc of cell width must respect row styling
>
> Old behaviour restored for table rows with custom skins.
> Unnecessary call to cell.applyCss() removed.
Just commented some minor things.
But overall it looks good.
I did some manual tests as well and can confirm that this works. :)
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableColumnHeader.java
line 715:
> 713: tableRow = createMeasureRow(tv, tableSkin, null);
> 714: }
> 715: assert tableRow.getSkin() instanceof SkinBase;
Not sure if there is some convention about code asserts, maybe some else can
tell. At least I never saw one in a PR here.
modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java
line 276:
> 274: }
> 275:
> 276: private TableRow createSmallRow(TableView tableView)
> {
Minor: Move this method to the other row method below?
modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java
line 295:
> 293: private TableRow createCustomRow(TableView
> tableView) {
> 294: TableRow row = new TableRow<>() {
> 295: protected javafx.scene.control.Skin createDefaultSkin() {
Very minor: `javafx.scene.control.` is not needed
-
Marked as reviewed by mhanl (Author).
PR: https://git.openjdk.java.net/jfx/pull/757
Re: RFR: 8251480: TableColumnHeader: calc of cell width must respect row styling [v2]
On Thu, 24 Mar 2022 04:57:27 GMT, Robert Lichtenberger wrote: >> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableColumnHeader.java >> line 650: >> >>> 648: } >>> 649: Callback, TableRow> rowFactory = >>> tv.getRowFactory(); >>> 650: TableRow tableRow = rowFactory != null ? >>> rowFactory.call(tv) : new TableRow<>(); >> >> When there is no row factory, we probably should just return like in line >> 632-633? > > Unlike cell factory, which always has to be present, the row factory is > optional and in fact most tables will not have a row factory. If we return in > that case, the algorithm will no longer work for these tables. > Compare with `javafx.scene.control.skin.TableViewSkin.createCell()`. You are right, thanks. - PR: https://git.openjdk.java.net/jfx/pull/757
Re: RFR: 8281723: Spinner with split horizontal arrows and a border places right arrow incorrectly [v2]
On Wed, 9 Mar 2022 07:48:53 GMT, John Hendrikx wrote: >> I added a test case for `SpinnerSkin` that checks the arrow positioning. >> >> While adding the tests I discovered more problems with the positioning aside >> from the one mentioned in the JBS ticket. >> >> 1) Vertical split arrow placement also forgot to take the padding into >> account while placing the decrement arrow button -- I've taken the liberty >> to fix that problem as well in the same PR. >> >> 2) When arrows are placed next to each other either on the right or left, >> the arrow widths are not normalized to be the width of the widest arrow. >> All other placements will normalize either the width or height, except for >> these two. Specifically, when the arrows are **split** on the left and >> right they **are** normalized to the same width. >> >> For point 2, here is the problem illustrated with actual widths on left and >> layout result on right: >> >> [ <- ] [ -> ] [ spinner ] --> [ <- ] [ -> ] [ >> spinner ] >> [ spinner ] [ <- ] [ -> ] --> [ spinner ] [ <- ] >> [ -> ] >> >> While for split horizontal it does normalize the width to that of the widest >> arrow, and so layout becomes: >> >> [ <- ] [ spinner ] [ -> ] --> [ <- ] [ spinner ] >> [ -> ] >> >> While I'm here I could fix this as well, and adjust the test case to match. > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Update copyright year Fix + Tests look good to me - Marked as reviewed by mhanl (Author). PR: https://git.openjdk.java.net/jfx/pull/748
Re: RFR: 8251480: TableColumnHeader: calc of cell width must respect row styling
On Wed, 16 Mar 2022 08:20:59 GMT, Robert Lichtenberger
wrote:
> This fix respects a row factory, if present.
> It will put the cell that is used to measure the column width as child below
> the row.
> In that way the row's style will be used.
The approach looks good. I left some comments and questions
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableColumnHeader.java
line 650:
> 648: }
> 649: Callback, TableRow> rowFactory =
> tv.getRowFactory();
> 650: TableRow tableRow = rowFactory != null ? rowFactory.call(tv)
> : new TableRow<>();
When there is no row factory, we probably should just return like in line
632-633?
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableColumnHeader.java
line 653:
> 651: tableSkin.getChildren().add(tableRow);
> 652: tableRow.applyCss();
> 653: ((SkinBase) tableRow.getSkin()).getChildren().add(cell);
I don't think this is a safe cast. Instead, you probably should do an
`instanceof SkinBase` check before
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableColumnHeader.java
line 667:
> 665: if ((cell.getText() != null && !cell.getText().isEmpty()) ||
> cell.getGraphic() != null) {
> 666: tableRow.applyCss();
> 667: cell.applyCss();
Just wondering: Is `cell.applyCss();` still needed here?
modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java
line 259:
> 257: /**
> 258: * @test
> 259: * @bug 8251480 Row style must affect the required column width
This annotations are not needed, although they do not hurt (just a note)
-
Changes requested by mhanl (Author).
PR: https://git.openjdk.java.net/jfx/pull/757
Re: RFR: 8283346: Optimize observable ArrayList creation in FXCollections [v3]
> This simple PR optimizes the observable `ArrayList` creation by using the
> ArrayList constructor/array size so that the underlying array will be
> initialized at the correct size which will speed up the creation as the array
> does not need to grow as a result of the `addAll` call.
>
> I also added tests which will succeed before and after to verify that nothing
> got broken by this change.
> Also I made a benchmark test. Results:
>
> | Benchmark | Mode| Cnt | Score | Error | Units
> | - | - | - | - |
> - | - |
> | ListBenchmark OLD | thrpt | 25 | 722,842 | ± 26,93 | ops/s
> | ListBenchmark NEW | thrpt | 25 | 29262,274 | ± 2088,712 | ops/s
>
> Benchmark code
>
>
> import javafx.collections.FXCollections;
> import javafx.collections.ObservableList;
> import org.openjdk.jmh.annotations.Benchmark;
> import org.openjdk.jmh.annotations.Scope;
> import org.openjdk.jmh.annotations.Setup;
> import org.openjdk.jmh.annotations.State;
> import org.openjdk.jmh.annotations.TearDown;
>
> import java.util.ArrayList;
> import java.util.List;
>
> @State(Scope.Benchmark)
> public class ListBenchmark {
>
> List strings;
>
> @Setup
> public void setup() {
> strings = new ArrayList<>();
> for(int i = 0; i< 10;i++) {
> strings.add("abc: " + i);
> }
> }
>
> @TearDown
> public void tearDown() {
> strings = null;
> }
>
> @Benchmark
> public ObservableList init() {
> return FXCollections.observableArrayList(strings);
> }
> }
>
>
>
Marius Hanl has updated the pull request incrementally with one additional
commit since the last revision:
8283346: Improved test name
-
Changes:
- all: https://git.openjdk.java.net/jfx/pull/758/files
- new: https://git.openjdk.java.net/jfx/pull/758/files/241962a3..a92e9a01
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jfx=758=02
- incr: https://webrevs.openjdk.java.net/?repo=jfx=758=01-02
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.java.net/jfx/pull/758.diff
Fetch: git fetch https://git.openjdk.java.net/jfx pull/758/head:pull/758
PR: https://git.openjdk.java.net/jfx/pull/758
Re: RFR: 8283346: Optimize observable ArrayList creation in FXCollections [v2]
> This simple PR optimizes the observable `ArrayList` creation by using the
> ArrayList constructor/array size so that the underlying array will be
> initialized at the correct size which will speed up the creation as the array
> does not need to grow as a result of the `addAll` call.
>
> I also added tests which will succeed before and after to verify that nothing
> got broken by this change.
> Also I made a benchmark test. Results:
>
> | Benchmark | Mode| Cnt | Score | Error | Units
> | - | - | - | - |
> - | - |
> | ListBenchmark OLD | thrpt | 25 | 722,842 | ± 26,93 | ops/s
> | ListBenchmark NEW | thrpt | 25 | 29262,274 | ± 2088,712 | ops/s
>
> Benchmark code
>
>
> import javafx.collections.FXCollections;
> import javafx.collections.ObservableList;
> import org.openjdk.jmh.annotations.Benchmark;
> import org.openjdk.jmh.annotations.Scope;
> import org.openjdk.jmh.annotations.Setup;
> import org.openjdk.jmh.annotations.State;
> import org.openjdk.jmh.annotations.TearDown;
>
> import java.util.ArrayList;
> import java.util.List;
>
> @State(Scope.Benchmark)
> public class ListBenchmark {
>
> List strings;
>
> @Setup
> public void setup() {
> strings = new ArrayList<>();
> for(int i = 0; i< 10;i++) {
> strings.add("abc: " + i);
> }
> }
>
> @TearDown
> public void tearDown() {
> strings = null;
> }
>
> @Benchmark
> public ObservableList init() {
> return FXCollections.observableArrayList(strings);
> }
> }
>
>
>
Marius Hanl has updated the pull request incrementally with one additional
commit since the last revision:
8283346: Better test name
-
Changes:
- all: https://git.openjdk.java.net/jfx/pull/758/files
- new: https://git.openjdk.java.net/jfx/pull/758/files/30262a55..241962a3
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jfx=758=01
- incr: https://webrevs.openjdk.java.net/?repo=jfx=758=00-01
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.java.net/jfx/pull/758.diff
Fetch: git fetch https://git.openjdk.java.net/jfx pull/758/head:pull/758
PR: https://git.openjdk.java.net/jfx/pull/758
Re: RFR: 8283346: Optimize observable ArrayList creation in FXCollections
On Fri, 18 Mar 2022 21:28:33 GMT, yosbits wrote: > * I don't know what the modCount property is used for, but is it okay if the > modified code has different values? This is fine as the mod count is only there to detect concurrent modifications. I recommend to read the `modCount` javadoc, its is very well written. And as @mstr2 also wrote, it is also an internal field (for the above mentioned purpose). - PR: https://git.openjdk.java.net/jfx/pull/758
Aw: Re: ArrayIndexOutOfBoundsException when disconnecting screen
Sorry for the delay. I tested a bit around, unfortunately this bug doesn't happen all the time. It looks like it can also happen when disconnecting just one screen. I have filed a ticket: https://bugs.openjdk.java.net/browse/JDK-8283401 Also one time the JVM crashed, leaving a hs_err_pid file, which I attached as well. -- Marius Gesendet: Mittwoch, 23. Februar 2022 um 23:54 Uhr Von: "Kevin Rushforth" An: "Marius Hanl" , "openjfx-dev" Betreff: Re: ArrayIndexOutOfBoundsException when disconnecting screen I spent a fair bit of time simulating and testing the D3DERR_DEVICEREMOVED cases for RDP disconnect to fix JDK-8239589 [1], but detaching a screen, which will change the number of active adapters, is a somewhat different case (I'm not sure how easy it would be to simulate it). It's very likely that it isn't handled in a robust manner. Do you know whether you need to disconnect 2 screens to reproduce this, or would disconnecting a single external monitor cause it? Based on the stack trace, it looks like the D3DResourceFactory array has been recreated with the reduced number of adapters (1), but the old screen is still being used by the instance of PPSRenderer. Can you file a bug with the stack trace, and as much information about how to reproduce it as you can. -- Kevin [1] [1]https://bugs.openjdk.java.net/browse/JDK-8239589 On 2/23/2022 3:35 AM, Marius Hanl wrote: > I get an ArrayIndexOutOfBoundsException sometimes when I disconnect my > screen(s). > Setup: I have a laptop with a docking station attached. The docking > station is connected to two screen, so when I disconnect it 2 screen > will be 'lost'. > > The following stacktrace is visible and the application is completely > black and unresponsive: > java.lang.ArrayIndexOutOfBoundsException: Index 1 out of bounds for > length 1 > at > com.sun.prism.d3d.D3DPipeline.getD3DResourceFactory(D3DPipeline.java:21 > 7) > at > com.sun.prism.d3d.D3DPipeline.getResourceFactory(D3DPipeline.java:284) > at > com.sun.scenario.effect.impl.prism.ps.PPSRenderer.validate(PPSRenderer. > java:101) > at > com.sun.scenario.effect.impl.prism.ps.PPSRenderer.getCompatibleImage(PP > SRenderer.java:221) > at > com.sun.scenario.effect.impl.prism.ps.PPSRenderer.getCompatibleImage(PP > SRenderer.java:67) > at > com.sun.scenario.effect.Effect.getCompatibleImage(Effect.java:479) > at > com.sun.javafx.sg.prism.NodeEffectInput.getImageDataForBoundedNode(Node > EffectInput.java:228) > at > com.sun.javafx.sg.prism.NodeEffectInput.filter(NodeEffectInput.java:131 > ) > at > com.sun.scenario.effect.FilterEffect.filter(FilterEffect.java:185) > at com.sun.scenario.effect.Offset.filter(Offset.java:160) > at com.sun.scenario.effect.Merge.filter(Merge.java:148) > at > com.sun.scenario.effect.DelegateEffect.filter(DelegateEffect.java:70) > at > com.sun.scenario.effect.impl.prism.PrEffectHelper.render(PrEffectHelper > .java:166) > at > com.sun.javafx.sg.prism.EffectFilter.render(EffectFilter.java:61) > at com.sun.javafx.sg.prism.NGNode.renderEffect(NGNode.java:2384) > at com.sun.javafx.sg.prism.NGNode.doRender(NGNode.java:2069) > at com.sun.javafx.sg.prism.NGNode.render(NGNode.java:1964) > at com.sun.javafx.sg.prism.NGGroup.renderContent(NGGroup.java:270) > at > com.sun.javafx.sg.prism.NGRegion.renderContent(NGRegion.java:579) > at com.sun.javafx.sg.prism.NGNode.doRender(NGNode.java:2072) > ... ( a lot of doRender(..), renderContent(..) calls... ) > at > com.sun.javafx.tk.quantum.ViewPainter.doPaint(ViewPainter.java:480) > at > com.sun.javafx.tk.quantum.ViewPainter.paintImpl(ViewPainter.java:329) > at > com.sun.javafx.tk.quantum.PresentingPainter.run(PresentingPainter.java: > 92) > at > java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors > .java:515) > at > java.base/java.util.concurrent.FutureTask.runAndReset$$$capture(FutureT > ask.java:305) > at > java.base/java.util.concurrent.FutureTask.runAndReset(FutureTask.java) > at com.sun.javafx.tk.RenderJob.run(RenderJob.java:58) > at > java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolE > xecutor.java:1128) > at > java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPool > Executor.java:628) > at > com.sun.javafx.tk.quantum.QuantumRenderer$PipelineRunnable.run(QuantumR > enderer.java:126) > at java.base/java.lang.Thread.run(Thread.java:829) > >
RFR: 8283346: Optimize observable ArrayList creation in FXCollections
This simple PR optimizes the observable `ArrayList` creation by using the
ArrayList constructor/array size so that the underlying array will be
initialized at the correct size which will speed up the creation as the array
does not need to grow as a result of the `addAll` call.
I also added tests which will succeed before and after to verify that nothing
got broken by this change.
Also I made a benchmark test. Results:
Old: `ListBenchmark.init thrpt 25 722,842 ± 26,930 ops/s`
New: `ListBenchmark.init thrpt 25 29262,274 ± 2088,712 ops/s`
Benchmark code
import javafx.collections.FXCollections;
import javafx.collections.ObservableList;
import org.openjdk.jmh.annotations.Benchmark;
import org.openjdk.jmh.annotations.Scope;
import org.openjdk.jmh.annotations.Setup;
import org.openjdk.jmh.annotations.State;
import org.openjdk.jmh.annotations.TearDown;
import java.util.ArrayList;
import java.util.List;
@State(Scope.Benchmark)
public class ListBenchmark {
List strings;
@Setup
public void setup() {
strings = new ArrayList<>();
for(int i = 0; i< 10;i++) {
strings.add("abc: " + i);
}
}
@TearDown
public void tearDown() {
strings = null;
}
@Benchmark
public ObservableList init() {
return FXCollections.observableArrayList(strings);
}
}
-
Commit messages:
- 8283346: Optimized observable array list creation
Changes: https://git.openjdk.java.net/jfx/pull/758/files
Webrev: https://webrevs.openjdk.java.net/?repo=jfx=758=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8283346
Stats: 47 lines in 2 files changed: 41 ins; 2 del; 4 mod
Patch: https://git.openjdk.java.net/jfx/pull/758.diff
Fetch: git fetch https://git.openjdk.java.net/jfx pull/758/head:pull/758
PR: https://git.openjdk.java.net/jfx/pull/758
Re: RFR: 8251480: TableColumnHeader: calc of cell width must respect row styling
On Wed, 16 Mar 2022 08:20:59 GMT, Robert Lichtenberger wrote: > This fix respects a row factory, if present. > It will put the cell that is used to measure the column width as child below > the row. > In that way the row's style will be used. Might make sense to also adjust the TreeTableView sizing implementation? - PR: https://git.openjdk.java.net/jfx/pull/757
Re: RFR: 8251483: TableCell: NPE on modifying item's list [v2]
> This PR fixes an issue where the item of the table row is null, although the > cell itself is not empty (non null value). > > The fix is to call `indexChanged(..)` immediately after the index was > changed, but before all `indexProperty()` listener are notified. > The then notified listener in `TableRowSkinBase` will update the underlying > cells, which will eventually result in an call to `updateItem(..)`, where > the NPE happened (and now not anymore, since the table row is now correctly > setup before). > > There is one special case: When the index didn't changed at all, we manually > call `indexChanged(..)` (basically just like before) since when a property is > not changed, `invalidated()` is not called, but we need to notify subclasses > that `updateIndex(..)` was called. Marius Hanl has updated the pull request incrementally with one additional commit since the last revision: 8251483: Updated copyright year - Changes: - all: https://git.openjdk.java.net/jfx/pull/741/files - new: https://git.openjdk.java.net/jfx/pull/741/files/5f65b82c..23921bf2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=741=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=741=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/741.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/741/head:pull/741 PR: https://git.openjdk.java.net/jfx/pull/741
RFR: 8251483: TableCell: NPE on modifying item's list
This PR fixes an issue where the item of the table row is null, although the cell itself is not empty (non null value). The fix is to call `indexChanged(..)` immediately after the index was changed, but before all `indexProperty()` listener are notified. The then notified listener in `TableRowSkinBase` will update the underlying cells, which will eventually result in an call to `updateItem(..)`, where the NPE happened (and now not anymore, since the table row is now correctly setup before). There is one special case: When the index didn't changed at all, we manually call `indexChanged(..)` (basically just like before) since when a property is not changed, `invalidated()` is not called, but we need to notify subclasses that `updateIndex(..)` was called. - Commit messages: - 8251483: indexChanged(..) is now called earlier to update the underlying item first before the listener which reacts to the index change are notified. Changes: https://git.openjdk.java.net/jfx/pull/741/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=741=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8251483 Stats: 75 lines in 3 files changed: 67 ins; 0 del; 8 mod Patch: https://git.openjdk.java.net/jfx/pull/741.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/741/head:pull/741 PR: https://git.openjdk.java.net/jfx/pull/741
ArrayIndexOutOfBoundsException when disconnecting screen
I get an ArrayIndexOutOfBoundsException sometimes when I disconnect my screen(s). Setup: I have a laptop with a docking station attached. The docking station is connected to two screen, so when I disconnect it 2 screen will be 'lost'. The following stacktrace is visible and the application is completely black and unresponsive: java.lang.ArrayIndexOutOfBoundsException: Index 1 out of bounds for length 1 at com.sun.prism.d3d.D3DPipeline.getD3DResourceFactory(D3DPipeline.java:21 7) at com.sun.prism.d3d.D3DPipeline.getResourceFactory(D3DPipeline.java:284) at com.sun.scenario.effect.impl.prism.ps.PPSRenderer.validate(PPSRenderer. java:101) at com.sun.scenario.effect.impl.prism.ps.PPSRenderer.getCompatibleImage(PP SRenderer.java:221) at com.sun.scenario.effect.impl.prism.ps.PPSRenderer.getCompatibleImage(PP SRenderer.java:67) at com.sun.scenario.effect.Effect.getCompatibleImage(Effect.java:479) at com.sun.javafx.sg.prism.NodeEffectInput.getImageDataForBoundedNode(Node EffectInput.java:228) at com.sun.javafx.sg.prism.NodeEffectInput.filter(NodeEffectInput.java:131 ) at com.sun.scenario.effect.FilterEffect.filter(FilterEffect.java:185) at com.sun.scenario.effect.Offset.filter(Offset.java:160) at com.sun.scenario.effect.Merge.filter(Merge.java:148) at com.sun.scenario.effect.DelegateEffect.filter(DelegateEffect.java:70) at com.sun.scenario.effect.impl.prism.PrEffectHelper.render(PrEffectHelper .java:166) at com.sun.javafx.sg.prism.EffectFilter.render(EffectFilter.java:61) at com.sun.javafx.sg.prism.NGNode.renderEffect(NGNode.java:2384) at com.sun.javafx.sg.prism.NGNode.doRender(NGNode.java:2069) at com.sun.javafx.sg.prism.NGNode.render(NGNode.java:1964) at com.sun.javafx.sg.prism.NGGroup.renderContent(NGGroup.java:270) at com.sun.javafx.sg.prism.NGRegion.renderContent(NGRegion.java:579) at com.sun.javafx.sg.prism.NGNode.doRender(NGNode.java:2072) ... ( a lot of doRender(..), renderContent(..) calls... ) at com.sun.javafx.tk.quantum.ViewPainter.doPaint(ViewPainter.java:480) at com.sun.javafx.tk.quantum.ViewPainter.paintImpl(ViewPainter.java:329) at com.sun.javafx.tk.quantum.PresentingPainter.run(PresentingPainter.java: 92) at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors .java:515) at java.base/java.util.concurrent.FutureTask.runAndReset$$$capture(FutureT ask.java:305) at java.base/java.util.concurrent.FutureTask.runAndReset(FutureTask.java) at com.sun.javafx.tk.RenderJob.run(RenderJob.java:58) at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolE xecutor.java:1128) at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPool Executor.java:628) at com.sun.javafx.tk.quantum.QuantumRenderer$PipelineRunnable.run(QuantumR enderer.java:126) at java.base/java.lang.Thread.run(Thread.java:829) --- --- Now my question: A naive fix would be an array check here but does someone has more information about the code here? And more interesting: Is there a way to write tests for this? Maybe with simulating/faking screens and then 'faking' a screen disconnect? -- Marius
Aw: Re: Sneak preview: cell edit api to support commit-on-focusLost
Sounds good. :) I think you just can push the branch, but not sure. I think only people with the 'contributor' title can push to it, which I'm not yet. It might not be simpler, but more visible and on a more common location. But yeah, the look and observing part can also be done in your fork. -- Marius Gesendet: Montag, 21. Februar 2022 um 14:21 Uhr Von: "Jeanette Winzenburg" An: "Marius Hanl" Cc: "openjfx-dev" Betreff: Re: Aw: Sneak preview: cell edit api to support commit-on-focusLost hmm .. yeah, will do .. once it's less sneak and less pre ;) Still fighting .. Though not entirely certain how to work with the sandbox: technically, that would be fork the sandbox, create a branch, merge my changes into that branch and push? And not sure how that would be simpler for others (except when actually cooperating) than now looking/monitoring the changes in my jfx fork? -- Jeanette Zitat von Marius Hanl : > Can you may create a branch at the jfx sandbox repo? > [1]https://github.com/openjdk/jfx-sandbox > > Then I can more easily compare and check it out. (and monitor it :)) > > -- Marius > GESENDET: Freitag, 11. Februar 2022 um 18:03 Uhr > VON: "Jeanette Winzenburg" > AN: "openjfx-dev" > BETREFF: Sneak preview: cell edit api to support commit-on-focusLost > > Hi folks, > > after fixing a bunch of edit-related issues, time feels right for > nudging elephant out of the room :) Which is the bigger goal of > supporting commit-on-focusLost. > > Working on a PoC (ListView/-Cell only) in a branch of my fork > [2]https://github.com/kleopatra/jfx/tree/dokeep-edit-api-editstate with > an overview of suggested changes > [3]https://github.com/kleopatra/swingempire-fx/wiki/CellEditAPI and a > example driver as gist > [4]https://gist.github.com/kleopatra/447344183e017537c21f7905a062396d > > Still rough, but working: all current unit tests still passing (a > bunch of my own not, and the new api is barely tested) and many of the > requested requirements can be implemented (see code in the example and > the table of use-cases in the overview) > > Any feedback - for the good or for the bad - highly welcome :) > > -- Jeanette > > References 1. https://github.com/openjdk/jfx-sandbox 2. https://github.com/kleopatra/jfx/tree/dokeep-edit-api-editstate 3. https://github.com/kleopatra/swingempire-fx/wiki/CellEditAPI 4. https://gist.github.com/kleopatra/447344183e017537c21f7905a062396d
Aw: Sneak preview: cell edit api to support commit-on-focusLost
Can you may create a branch at the jfx sandbox repo? https://github.com/openjdk/jfx-sandbox Then I can more easily compare and check it out. (and monitor it :)) -- Marius Gesendet: Freitag, 11. Februar 2022 um 18:03 Uhr Von: "Jeanette Winzenburg" An: "openjfx-dev" Betreff: Sneak preview: cell edit api to support commit-on-focusLost Hi folks, after fixing a bunch of edit-related issues, time feels right for nudging elephant out of the room :) Which is the bigger goal of supporting commit-on-focusLost. Working on a PoC (ListView/-Cell only) in a branch of my fork [1]https://github.com/kleopatra/jfx/tree/dokeep-edit-api-editstate with an overview of suggested changes [2]https://github.com/kleopatra/swingempire-fx/wiki/CellEditAPI and a example driver as gist [3]https://gist.github.com/kleopatra/447344183e017537c21f7905a062396d Still rough, but working: all current unit tests still passing (a bunch of my own not, and the new api is barely tested) and many of the requested requirements can be implemented (see code in the example and the table of use-cases in the overview) Any feedback - for the good or for the bad - highly welcome :) -- Jeanette References 1. https://github.com/kleopatra/jfx/tree/dokeep-edit-api-editstate 2. https://github.com/kleopatra/swingempire-fx/wiki/CellEditAPI 3. https://gist.github.com/kleopatra/447344183e017537c21f7905a062396d
Re: RFR: 8187309: TreeCell must not change tree's data
On Wed, 2 Feb 2022 14:18:18 GMT, Jeanette Winzenburg wrote: > Issue was TreeView commit editing implementation violated the spec'ed > mechanism: > > - no default commit handler on TreeView > - TreeCell modifying the data directly > > Fix is to move the saving of the edited value from cell into a default commit > handler in tree and set that handler in the constructor. > > Added tests that failed/passed before/after the fix (along with a sanity test > for default commit that passed also before). Also fixed a test bug (incorrect > registration of custom commit handler, see > [JDK-8280951)](https://bugs.openjdk.java.net/browse/JDK-8280951) in > TreeViewTest.test_rt_29650 to keep it passing. Looks good to me too. The commitEdit(..) method of TreeCell is now in sync with the other cells. Just left two minor comments. modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeCellTest.java line 944: > 942: } > 943: > 944: Very minor: Empty line modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeCellTest.java line 957: > 955: > 956: /** > 957: * Test test setup. Minor: I would rephrase that a bit. Something like: `Tests the cell editing setup`. - Marked as reviewed by mhanl (Author). PR: https://git.openjdk.java.net/jfx/pull/724
Integrated: 8251481: TableCell accessing row: NPE on auto-sizing
On Fri, 14 Jan 2022 00:04:49 GMT, Marius Hanl wrote: > This PR will fix a simple NPE which may happens when using the `TableRow` > inside a `TableCell` during the initial auto sizing. > In the auto sizing code, no `TableRow` is set, therefore `getTableRow()` will > return null and it is not possible to e.g. access the row item. > > This is fixed by adding the `TableRow` in the `resizeColumnToFitContent` > method, similar as it is already done for the `TreeTableView` > (`TreeTableRow`). This pull request has now been integrated. Changeset: 59cd8ec2 Author:Marius Hanl Committer: Jeanette Winzenburg URL: https://git.openjdk.java.net/jfx/commit/59cd8ec2d6a221a19ac4eb3a26f65535766410cc Stats: 52 lines in 3 files changed: 49 ins; 0 del; 3 mod 8251481: TableCell accessing row: NPE on auto-sizing Reviewed-by: fastegal - PR: https://git.openjdk.java.net/jfx/pull/716
Re: RFR: 8251481: TableCell accessing row: NPE on auto-sizing [v3]
On Sat, 29 Jan 2022 13:57:01 GMT, Jeanette Winzenburg
wrote:
>> Marius Hanl has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8251481: Using global stageLoader now
>
> modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableCellTest.java
> line 690:
>
>> 688: */
>> 689: @Test
>> 690: public void testRowIsNotNullWhenAutoSizing() {
>
> same as autosizing test for TableCell: would like the issue id :)
done
-
PR: https://git.openjdk.java.net/jfx/pull/716
Re: RFR: 8251481: TableCell accessing row: NPE on auto-sizing [v4]
> This PR will fix a simple NPE which may happens when using the `TableRow` > inside a `TableCell` during the initial auto sizing. > In the auto sizing code, no `TableRow` is set, therefore `getTableRow()` will > return null and it is not possible to e.g. access the row item. > > This is fixed by adding the `TableRow` in the `resizeColumnToFitContent` > method, similar as it is already done for the `TreeTableView` > (`TreeTableRow`). Marius Hanl has updated the pull request incrementally with one additional commit since the last revision: 8251481: Added jdk ticket reference in TreeTableCellTest + Grammar - Changes: - all: https://git.openjdk.java.net/jfx/pull/716/files - new: https://git.openjdk.java.net/jfx/pull/716/files/a8d99555..a706068e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=716=03 - incr: https://webrevs.openjdk.java.net/?repo=jfx=716=02-03 Stats: 3 lines in 2 files changed: 1 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jfx/pull/716.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/716/head:pull/716 PR: https://git.openjdk.java.net/jfx/pull/716
Integrated: 8277122: SplitPane divider drag can hang the layout
On Mon, 15 Nov 2021 14:34:04 GMT, Marius Hanl wrote: > When a divider is moved via drag or code it will call **requestLayout()** for > the **SplitPane**. > While this is fine, it is also called when the > **SplitPaneSkin#layoutChildren(..)** method is repositioning the divider. > > This makes no sense since we are currently layouting everything, so we don't > need to request it again. (The divider positioning is the first part of > **layoutChildren(..)**. In the second part the SplitPane content is layouted > based off those positions) > > -> With this behaviour the layout may hang under some conditions (check > attached source). The problem is that the **requestLayout()** will mark the > **SplitPane** dirty but won't propagate to the parent since the **SplitPane** > is currently doing a layout. > > This PR fixes this by not requesting layout when we are currently doing it > (and thus repositioning the dividers as part of it). > > Note: I also fixed a simple typo of a private method in SplitPaneSkin: > initializeDivderEventHandlers -> initializeDiv**i**derEventHandlers This pull request has now been integrated. Changeset: ee52d146 Author:Marius Hanl Committer: Jeanette Winzenburg URL: https://git.openjdk.java.net/jfx/commit/ee52d14653996921a9bd30e9b568151d3d4d06de Stats: 84 lines in 2 files changed: 76 ins; 1 del; 7 mod 8277122: SplitPane divider drag can hang the layout Reviewed-by: fastegal, aghaisas - PR: https://git.openjdk.java.net/jfx/pull/669
Re: RFR: 8251481: TableCell accessing row: NPE on auto-sizing [v3]
On Fri, 28 Jan 2022 12:40:15 GMT, Jeanette Winzenburg wrote: >> I can align it. And yeah makes sense to add a test for the >> TreeTableView/TreeTableCell. > > just curious: why didn't you move the tests into TableColumnHeaderTest? I had no particular reason, I think the test fits both classes, although `TableColumnHeaderTest` just tests the normal `TableView`. So I kept it in the `TableCellTest` class - PR: https://git.openjdk.java.net/jfx/pull/716
Re: RFR: 8277122: SplitPane divider drag can hang the layout [v2]
On Thu, 27 Jan 2022 15:21:33 GMT, Marius Hanl wrote: >> just for reference - found the source of my [faintly >> remember](https://github.com/openjdk/jfx/pull/337/files/8a6fc1cf6cad2c453de17b71777ddd63fadb539e#r510975640) > > I see. Unfortunately though that was not done consistently during the past > PRs. Done. Also adjusted the copyright year ;) - PR: https://git.openjdk.java.net/jfx/pull/669
Re: RFR: 8277122: SplitPane divider drag can hang the layout [v4]
> When a divider is moved via drag or code it will call **requestLayout()** for > the **SplitPane**. > While this is fine, it is also called when the > **SplitPaneSkin#layoutChildren(..)** method is repositioning the divider. > > This makes no sense since we are currently layouting everything, so we don't > need to request it again. (The divider positioning is the first part of > **layoutChildren(..)**. In the second part the SplitPane content is layouted > based off those positions) > > -> With this behaviour the layout may hang under some conditions (check > attached source). The problem is that the **requestLayout()** will mark the > **SplitPane** dirty but won't propagate to the parent since the **SplitPane** > is currently doing a layout. > > This PR fixes this by not requesting layout when we are currently doing it > (and thus repositioning the dividers as part of it). > > Note: I also fixed a simple typo of a private method in SplitPaneSkin: > initializeDivderEventHandlers -> initializeDiv**i**derEventHandlers Marius Hanl has updated the pull request incrementally with one additional commit since the last revision: 8277122: Added and used global stageLoader + changed copyright year to 2022 - Changes: - all: https://git.openjdk.java.net/jfx/pull/669/files - new: https://git.openjdk.java.net/jfx/pull/669/files/345552d4..a5f9f500 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=669=03 - incr: https://webrevs.openjdk.java.net/?repo=jfx=669=02-03 Stats: 11 lines in 2 files changed: 7 ins; 1 del; 3 mod Patch: https://git.openjdk.java.net/jfx/pull/669.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/669/head:pull/669 PR: https://git.openjdk.java.net/jfx/pull/669
Re: RFR: 8251481: TableCell accessing row: NPE on auto-sizing [v3]
On Tue, 25 Jan 2022 12:09:31 GMT, Jeanette Winzenburg wrote: >> Marius Hanl has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8251481: Using global stageLoader now > > modules/javafx.controls/src/test/java/test/javafx/scene/control/TableCellTest.java > line 385: > >> 383: StageLoader loader = new StageLoader(table); >> 384: >> 385: loader.dispose(); > > note that the loader isn't disposed if the test fails - that's why there's an > instance field (which will be disposed in cleanup), which should be used here done - PR: https://git.openjdk.java.net/jfx/pull/716
Re: RFR: 8251481: TableCell accessing row: NPE on auto-sizing [v3]
> This PR will fix a simple NPE which may happens when using the `TableRow` > inside a `TableCell` during the initial auto sizing. > In the auto sizing code, no `TableRow` is set, therefore `getTableRow()` will > return null and it is not possible to e.g. access the row item. > > This is fixed by adding the `TableRow` in the `resizeColumnToFitContent` > method, similar as it is already done for the `TreeTableView` > (`TreeTableRow`). Marius Hanl has updated the pull request incrementally with one additional commit since the last revision: 8251481: Using global stageLoader now - Changes: - all: https://git.openjdk.java.net/jfx/pull/716/files - new: https://git.openjdk.java.net/jfx/pull/716/files/30dd77e9..a8d99555 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=716=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx=716=01-02 Stats: 6 lines in 2 files changed: 0 ins; 4 del; 2 mod Patch: https://git.openjdk.java.net/jfx/pull/716.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/716/head:pull/716 PR: https://git.openjdk.java.net/jfx/pull/716
Re: RFR: 8277122: SplitPane divider drag can hang the layout [v2]
On Thu, 27 Jan 2022 12:46:19 GMT, Jeanette Winzenburg wrote: >> ah okay. Was just confusing for me since I never read that and I think some >> recent PRs still had this pattern, e.g. also the touch table scrolling PR I >> had a look at yesterday. >> >> Maybe for future it makes sense to have an abstract test class with the >> stage loader setup already in place. > > just for reference - found the source of my [faintly > remember](https://github.com/openjdk/jfx/pull/337/files/8a6fc1cf6cad2c453de17b71777ddd63fadb539e#r510975640) I see. Unfortunately though that was not done consistently during the past PRs. - PR: https://git.openjdk.java.net/jfx/pull/669
Re: RFR: 8277122: SplitPane divider drag can hang the layout [v2]
On Thu, 27 Jan 2022 10:18:31 GMT, Jeanette Winzenburg wrote: >> Hm, is this really needed? Not sure, are there any side effects by the >> `StageLoader` like this when a test failed? >> Just asking since the `StageLoader` is used a lot like this. >> And since the tests normally run green unless you may change something >> locally (on the JavaFX Pipeline it should never be red), it would probably >> never affect anything (or maybe it does?). > >> Hm, is this really needed? > > yes, IMO, we want the exact same cleanup for passing/failing tests. So either > dispose is required always (then need to make sure it's called on failure) or > not required always (then all its calls would be noise). > >> Not sure, are there any side effects by the `StageLoader` like this when a >> test failed? Just asking since the `StageLoader` is used a lot like this. > > don't now (and doesn't matter, what matters is the guaranteed cleanup) - and > aware of those slightly fishy patterns, we all learn :) Faintly remember > having discussed the point in a PR (can't find it right now, though), and > just as faintly remember the outcome was to guarantee the cleanup in new > tests. ah okay. Was just confusing for me since I never read that and I think some recent PRs still had this pattern, e.g. also the touch table scrolling PR I had a look at yesterday. Maybe for future it makes sense to have an abstract test class with the stage loader setup already in place. - PR: https://git.openjdk.java.net/jfx/pull/669
Re: RFR: 8251481: TableCell accessing row: NPE on auto-sizing [v2]
> This PR will fix a simple NPE which may happens when using the `TableRow` > inside a `TableCell` during the initial auto sizing. > In the auto sizing code, no `TableRow` is set, therefore `getTableRow()` will > return null and it is not possible to e.g. access the row item. > > This is fixed by adding the `TableRow` in the `resizeColumnToFitContent` > method, similar as it is already done for the `TreeTableView` > (`TreeTableRow`). Marius Hanl has updated the pull request incrementally with one additional commit since the last revision: 8251481: Added TreeTableCellTest / reordered TableCellTest and added more details to it (javadoc) - Changes: - all: https://git.openjdk.java.net/jfx/pull/716/files - new: https://git.openjdk.java.net/jfx/pull/716/files/0ba6b283..30dd77e9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=716=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=716=00-01 Stats: 65 lines in 2 files changed: 45 ins; 19 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/716.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/716/head:pull/716 PR: https://git.openjdk.java.net/jfx/pull/716
Re: RFR: 8251481: TableCell accessing row: NPE on auto-sizing
On Tue, 25 Jan 2022 12:00:11 GMT, Jeanette Winzenburg
wrote:
>> This PR will fix a simple NPE which may happens when using the `TableRow`
>> inside a `TableCell` during the initial auto sizing.
>> In the auto sizing code, no `TableRow` is set, therefore `getTableRow()`
>> will return null and it is not possible to e.g. access the row item.
>>
>> This is fixed by adding the `TableRow` in the `resizeColumnToFitContent`
>> method, similar as it is already done for the `TreeTableView`
>> (`TreeTableRow`).
>
> modules/javafx.controls/src/test/java/test/javafx/scene/control/TableCellTest.java
> line 371:
>
>> 369: @Test
>> 370: public void testRowIsNotNullWhenAutoSizing() {
>> 371: TableColumn tableColumn = new TableColumn<>();
>
> - the bug that's fixed in this PR is in TableColumnHeader, shouldn't the test
> be in TableColumnHeaderTest?
> - if you decide to keep it here: it's in the middle of some edit-related
> tests, you might consider moving it up/down before/after those
> - the fix aligns the resizeToFit method for TableView with that for
> TreeTableView: for symmetry, I would also expect a test method for the latter
> (which will pass both before and after the fix)
I can align it. And yeah makes sense to add a test for the
TreeTableView/TreeTableCell.
> modules/javafx.controls/src/test/java/test/javafx/scene/control/TableCellTest.java
> line 379:
>
>> 377:
>> 378: assertNotNull(getTableRow());
>> 379: }
>
> feeling slightly uncomfortable with the generality of this: looks a bit like
> it's guaranteed that tableRow != null always (bullet 2 of our previous
> conversation) - would suggest to make it more specific to auto-sizing (f.i.
> start without auto-size, then trigger autosizing by code). Or at least doc it
> (add a message to the assertion, add the bug id) so that future readers will
> know what exactly is tested here :)
Pretty sure table row is never null. Or is it on some corner case? Because even
on an empty table, table rows with empty cells will have the row (since rows
will be added in the virtualflow)
-
PR: https://git.openjdk.java.net/jfx/pull/716
Re: RFR: 8277122: SplitPane divider drag can hang the layout [v2]
On Tue, 25 Jan 2022 13:45:54 GMT, Jeanette Winzenburg wrote: >> Marius Hanl has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8277122: Added test for setting a negative divider position + improved >> readability > > modules/javafx.controls/src/test/java/test/javafx/scene/control/SplitPaneTest.java > line 1387: > >> 1385: assertTrue(layoutCounter.get() > 0); >> 1386: stageLoader.dispose(); >> 1387: } > > the stageLoader is not disposed if the test fails - a (recently introduced :) > general pattern is to use an instance field that's disposed in the test > cleanup Hm, is this really needed? Not sure, are there any side effects by the `StageLoader` like this when a test failed? Just asking since the `StageLoader` is used a lot like this. And since the tests normally run green unless you may change something locally (on the JavaFX Pipeline it should never be red), it would probably never affect anything (or maybe it does?). - PR: https://git.openjdk.java.net/jfx/pull/669
Re: RFR: 8277122: SplitPane divider drag can hang the layout [v2]
On Tue, 25 Jan 2022 13:38:53 GMT, Jeanette Winzenburg
wrote:
>> Marius Hanl has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8277122: Added test for setting a negative divider position + improved
>> readability
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/SplitPaneSkin.java
> line 72:
>
>> 70: * {@link #layoutChildren(double, double, double, double)} since we
>> are currently doing the layout.
>> 71: */
>> 72: private boolean duringLayout;
>
> would like a reference to the bug this fixes
Added it
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/SplitPaneSkin.java
> line 226:
>
>> 224: // If the window is less than the min size we want to resize
>> proportionally
>> 225: duringLayout = true;
>> 226: double minSize = totalMinSize();
>
> - setting the flag belongs above the code comment to not disrupt explanation
> and its target (== minsize)
> - I think we don't do formatting (here: change the code comment to a single
> line)
Changed.
I kept the comment since it is basically just a single line but yes you are
right :)
> modules/javafx.controls/src/test/java/test/javafx/scene/control/SplitPaneTest.java
> line 1344:
>
>> 1342: * which can hang the layout as it resulted in multiple layout
>> requests (through SplitPaneSkin.layoutChildren).
>> 1343: */
>> 1344: @Test
>
> My preference would be to add the bug id to the tests as well ..
Added it
-
PR: https://git.openjdk.java.net/jfx/pull/669
Re: RFR: 8277122: SplitPane divider drag can hang the layout [v3]
> When a divider is moved via drag or code it will call **requestLayout()** for > the **SplitPane**. > While this is fine, it is also called when the > **SplitPaneSkin#layoutChildren(..)** method is repositioning the divider. > > This makes no sense since we are currently layouting everything, so we don't > need to request it again. (The divider positioning is the first part of > **layoutChildren(..)**. In the second part the SplitPane content is layouted > based off those positions) > > -> With this behaviour the layout may hang under some conditions (check > attached source). The problem is that the **requestLayout()** will mark the > **SplitPane** dirty but won't propagate to the parent since the **SplitPane** > is currently doing a layout. > > This PR fixes this by not requesting layout when we are currently doing it > (and thus repositioning the dividers as part of it). > > Note: I also fixed a simple typo of a private method in SplitPaneSkin: > initializeDivderEventHandlers -> initializeDiv**i**derEventHandlers Marius Hanl has updated the pull request incrementally with one additional commit since the last revision: 8277122: Added bug id to the fix and test for future reference - Changes: - all: https://git.openjdk.java.net/jfx/pull/669/files - new: https://git.openjdk.java.net/jfx/pull/669/files/9db28ff0..345552d4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=669=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx=669=01-02 Stats: 5 lines in 2 files changed: 4 ins; 1 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/669.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/669/head:pull/669 PR: https://git.openjdk.java.net/jfx/pull/669
Re: RFR: 8277853: With Touch enabled devices scrollbar disappears and the table is scrolled to the beginning [v4]
On Wed, 26 Jan 2022 05:36:07 GMT, meghanEmbrace wrote: >> With a touch-enabled device, the scrollbar disappears a short while after >> it's used. During the layout, updateHbar() checks the hbar visibility and >> resets the clip, causing the user to be scrolled fully to the left when >> trying to access columns on the right. Using hbar.isVisible() is not >> feasible as there are times when the scrollbar is necessary but not visible >> (such as on touch-enabled devices where the scrollbar disappears when not in >> use, or when hidden by CSS). Hence, it is more reliable to use the variable >> that determines whether the hbar is necessary. > > meghanEmbrace has updated the pull request incrementally with one additional > commit since the last revision: > > Updated test name to be more descriptive. Looks good to me too. Tested with `-Dcom.sun.javafx.touch=true` and also without to make sure there is no regression. - Marked as reviewed by mhanl (Author). PR: https://git.openjdk.java.net/jfx/pull/688
Re: RFR: 8277853: With Touch enabled devices scrollbar disappears and the table is scrolled to the beginning [v3]
On Tue, 25 Jan 2022 14:34:09 GMT, meghanEmbrace wrote:
>> With a touch-enabled device, the scrollbar disappears a short while after
>> it's used. During the layout, updateHbar() checks the hbar visibility and
>> resets the clip, causing the user to be scrolled fully to the left when
>> trying to access columns on the right. Using hbar.isVisible() is not
>> feasible as there are times when the scrollbar is necessary but not visible
>> (such as on touch-enabled devices where the scrollbar disappears when not in
>> use, or when hidden by CSS). Hence, it is more reliable to use the variable
>> that determines whether the hbar is necessary.
>
> meghanEmbrace has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Replaced the null test with an assert.
modules/javafx.controls/src/test/java/test/javafx/scene/control/TableViewTest.java
line 801:
> 799: * Tests for specific bugs
> *
> 800:
> /
> 801: @Test public void test_jdk_8277853() {
Not sure if there is some preference around here but I always like if the test
method is not just named after the ticket but instead what it actually tests,
e.g. something like `testInvisibleScrollbarDoesNotScrollTableToBeginning`.
Optionally the ticket number can be referenced in the javadoc then. But that's
just me (just a side note here).
-
PR: https://git.openjdk.java.net/jfx/pull/688
Aw: Strange test failures: validation of PGGroup children failed
I think I had the same issues some days ago. What helped was to delete all the 'build' or 'target' or 'out' folders - so basically all the folders with the compiled files. Gesendet: Dienstag, 18. Januar 2022 um 15:46 Uhr Von: "Jeanette Winzenburg" An: "openjfx-dev" Betreff: Strange test failures: validation of PGGroup children failed Just stumbled across it while reviewing PR 719 ([1]https://github.com/openjdk/jfx/pull/716) - added the new test method to see it fail (which it does) - run all control tests throws the error below in _unrelated_ tests (the one below is from TableCellTest, that is the same test the new failing method resides in, but seeing similar in other tests that use Toolkit.firePulse) Still happens after a clean build from gradle (clean, sdk, :controls:test). Any idea what might be the reason? java.lang.AssertionError: validation of PGGroup children failed at javafx.graphics@19-internal/javafx.scene.Parent.validatePG(Parent.java: 243) at javafx.graphics@19-internal/javafx.scene.Parent.doUpdatePeer(Parent.jav a:201) at javafx.graphics@19-internal/javafx.scene.Parent$1.doUpdatePeer(Parent.j ava:109) at javafx.graphics@19-internal/com.sun.javafx.scene.ParentHelper.updatePee rImpl(ParentHelper.java:78) at javafx.graphics@19-internal/com.sun.javafx.scene.layout.RegionHelper.up datePeerImpl(RegionHelper.java:72) at javafx.graphics@19-internal/com.sun.javafx.scene.NodeHelper.updatePeer( NodeHelper.java:103) at javafx.graphics@19-internal/javafx.scene.Node.syncPeer(Node.java:715) at javafx.graphics@19-internal/javafx.scene.Scene$ScenePulseListener.syncA ll(Scene.java:2397) at javafx.graphics@19-internal/javafx.scene.Scene$ScenePulseListener.syncA ll(Scene.java:2406) at javafx.graphics@19-internal/javafx.scene.Scene$ScenePulseListener.syncA ll(Scene.java:2406) at javafx.graphics@19-internal/javafx.scene.Scene$ScenePulseListener.syncA ll(Scene.java:2406) at javafx.graphics@19-internal/javafx.scene.Scene$ScenePulseListener.synch ronizeSceneNodes(Scene.java:2373) at javafx.graphics@19-internal/javafx.scene.Scene$ScenePulseListener.pulse (Scene.java:2529) at javafx.graphics@19-internal/com.sun.javafx.tk.Toolkit.lambda$runPulse$2 (Toolkit.java:405) at java.base/java.security.AccessController.doPrivileged(AccessController. java:389) at javafx.graphics@19-internal/com.sun.javafx.tk.Toolkit.runPulse(Toolkit. java:404) at javafx.graphics@19-internal/com.sun.javafx.tk.Toolkit.firePulse(Toolkit .java:434) at test.javafx.scene.control.TableCellTest.testEditCancelEventAfterModifyI tems(TableCellTest.java:557) References 1. https://github.com/openjdk/jfx/pull/716
Re: RFR: 8279640: ListView with null SelectionModel/FocusModel throws NPE
On Tue, 11 Jan 2022 17:21:13 GMT, Michael Strauß wrote: > The question is whether or not `null` should be a valid value for the > `selectionModel` and `focusModel` properties. I think there are good reasons > to think that it shouldn't. The primitive property classes > (`IntegerProperty`, `BooleanProperty`, etc.) have the same issue, and they > deal with this situation by coercing the invalid `null` value to their > respective default value. I think the property classes for primitives are another use case, since those values really can't be null. I get your point but I think a null `selectionModel` or `focusModel` should be allowed. At least an use case would be to have a read-only `ListView` which should not be selectable nor focusable. If you just set a ListView to uneditable (via `setEditable(..)`), you can still select entries. If you disable the `ListView` it is grey styled which is not desired in this use case. Of course I can change the style, but a null selection and-focusModel may make sense as well. But I agree to a certain point, maybe it make's sense to fail for some properties when null is set/or use default values (but without changing the property?). Right now some properties in e.g. `Node` can be set to null and it will throw a NPE somewhere (e.g. `setCacheHint(null)`, `setRotationAxis(null)`, `Labeled.setFont(null)` ...). But on the other hand a lot of code handle properties with null with some kind of default behaviour (the property itself is never changed). Examples for this: - `Labeled.getTextOverrun()` -> null will be handled as ELLIPSIS in LabeledSkinBase - `Labeled.getEllipsisString()` -> null will be handled as "" in LabeledSkinBase - ... So there is no consistent behaviour for this, but a lot of code already handles null by behaving in some kind of default way without changing the property directly, and I think it might be the best to adapt to this. - PR: https://git.openjdk.java.net/jfx/pull/711
Re: RFR: 8187307: ListView, TableView, TreeView: receives editCancel event when edit is committed
On Tue, 30 Nov 2021 12:32:37 GMT, Jeanette Winzenburg wrote: > The misbehaviour was that an edit handler received both a commit and cancel > event when cell commitEdit is called. That happened whenever a collaborator > reset the controls editing state (either directly or indirectly) while > processing the editCommit event. The reason was that the cell had not yet > updated its own editing state when receiving the change of editing from the > control. > > Fix is to update cell's editing state before firing the event, that is change > the sequence or method calls from fire/super.commit to super.commit/fire. > > Added tests that fail/pass before/after the fix. Looks good to me. I also did some manual tests and everything looks fine here too. - Marked as reviewed by mhanl (Author). PR: https://git.openjdk.java.net/jfx/pull/684
Re: RFR: 8251481: TableCell accessing row: NPE on auto-sizing
On Fri, 14 Jan 2022 11:52:40 GMT, Jeanette Winzenburg wrote: > hmm .. I think here are two issues: > > * the auto-sizing code definitely needs to fully configure the cell (with > index, row, column, tableView plus applying css) to measure the correct size > * the implementation of cell.updateItem (even though it's my own ;) is a bit > fishy: does its spec really guarantee that getTableRow() != null if !empty()? > Meanwhile, I think that's not the case - but then, it's for demonstrating the > first bullet :) > > The first is fixed by this PR (looks good on first sight). The second might > need a clarification in the method doc .. or not, undecided. Yes the auto sizing has some more issues. While looking on it, I found two problems which should be done as follow-up: 1. Both `resizeColumnToFitContent` methods do not use the row factory of the corresponding table (minor, but still something which should be improved here) 2. Currently both implementation add the `TableCell` directly into the `TableSkin`, which is not how it is usually done. Normally the `TableRow` is added (by the `VirtualFlow`), therefore the example of the related ticket [JDK-8251480](https://bugs.openjdk.java.net/browse/JDK-8251480) won't work after this fix, but may do when the `TableRow` is added instead (didn't tested it though) May both can be addressed in future via [JDK-8251480](https://bugs.openjdk.java.net/browse/JDK-8251480) - PR: https://git.openjdk.java.net/jfx/pull/716
RFR: 8251481: TableCell accessing row: NPE on auto-sizing
This PR will fix a simple NPE which may happens when using the `TableRow` inside a `TableCell` during the initial auto sizing. In the auto sizing code, no `TableRow` is set, therefore `getTableRow()` will return null and it is not possible to e.g. access the row item. This is fixed by adding the `TableRow` in the `resizeColumnToFitContent` method, similar as it is already done for the `TreeTableView` (`TreeTableRow`). - Commit messages: - 8251481: TableCell accessing row: NPE on auto-sizing Changes: https://git.openjdk.java.net/jfx/pull/716/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=716=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8251481 Stats: 28 lines in 2 files changed: 26 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jfx/pull/716.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/716/head:pull/716 PR: https://git.openjdk.java.net/jfx/pull/716
Re: RFR: 8279640: ListView with null SelectionModel/FocusModel throws NPE
On Tue, 11 Jan 2022 09:01:51 GMT, John Hendrikx wrote:
> ```
> public final MultipleSelectionModel getSelectionModel() {
> return selectionModel == null ? null :
> selectionModel.get() == NONE_SELECTION_MODEL ? null :
> selectionModel.get();
> }
> ```
That would work altough I think it's a bit hacky as when I listen on the
`selectionModelProperty` I still would get the noop selection model.
I'm in general not a fan of 'self-changing' properties. When a null value is
possible, guards against are needed.
Unfortunately there is no built-in way to forbid null in Java as whole.
> A check can also be done to see if something matches the `NONE` model, just
> like you can check for `null`, so you can still fast return in special cases.
But then I don't see any advantage as it makes no difference if we check for
NONE model or null. Checks are needed in any case.
-
PR: https://git.openjdk.java.net/jfx/pull/711
Re: RFR: 8279640: ListView with null SelectionModel/FocusModel throws NPE
On Sat, 8 Jan 2022 14:32:30 GMT, John Hendrikx wrote: > Same goes for the selection model; if set to `null` it should not allow any > kind of selection, which again seems best achieved by installing a model that > ignores all such actions and always returns empty values. We had a similar discussion on https://github.com/openjdk/jfx/pull/557. We came to the conclusion that we allow null as a selection or focusmodel and don't set a noop selection model or something of this kind. The biggest problem with this is the following example: I as a developer set the selection model to null via `setSelectionModel(null)`. Now if the code silently set the selection model to an empty noop selection model, we won't get null when calling `getSelectionModel()`, which a developer would expect. Also from a look of the code, even if we use a noop focus model, we would still the focus because of the `anchor` stuff, which is set in the `ListViewBehaviour`. If we do a null check and fast return like now, they won't be set -> there are not visible. So it might not even fix our problem but makes even more. - PR: https://git.openjdk.java.net/jfx/pull/711
RFR: 8279640: ListView with null SelectionModel/FocusModel throws NPE
This PR fixes a bunch of NPEs when a null `SelectionModel` or `FocusModel` is
set on a `ListView`.
The following NPEs are fixed (all are also covered by exactly one test case):
NPEs with null selection model:
- Mouse click on a `ListCell`
- SPACE key press
- KP_UP (arrow up) key press
- HOME key press
- END key press
- BACK_SLASH + CTRL key press
NPEs with null focus model:
- SPACE key press
- Select an items: getSelectionModel().select(1)
- Clear-Select an item and add one after:
listView.getSelectionModel().clearAndSelect(1); listView.getItems().add("3");
-
Commit messages:
- 8279640: ListView with null SelectionModel/FocusModel throws NPE
Changes: https://git.openjdk.java.net/jfx/pull/711/files
Webrev: https://webrevs.openjdk.java.net/?repo=jfx=711=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8279640
Stats: 145 lines in 4 files changed: 130 ins; 2 del; 13 mod
Patch: https://git.openjdk.java.net/jfx/pull/711.diff
Fetch: git fetch https://git.openjdk.java.net/jfx pull/711/head:pull/711
PR: https://git.openjdk.java.net/jfx/pull/711
Integrated: 8191995: Regression: DatePicker must commit on focusLost
On Wed, 24 Nov 2021 09:09:53 GMT, Marius Hanl wrote: > This PR fixes an issue where the `DatePicker` is not committing his text as > value when the focus is lost. > As the ticket also mentions, this is a regression which last worked on JavaFX > 8 and got broken by this refactoring: > [JDK-8150946](https://bugs.openjdk.java.net/browse/JDK-8150946) > > The fix is to provide the same api to the `DatePicker` which was introduced > by [JDK-8150946](https://bugs.openjdk.java.net/browse/JDK-8150946) for > `ComboBox` and `Spinner`. > > Note: While fixing this I found a possible bug which I tracked here: > [JDK-8277756](https://bugs.openjdk.java.net/browse/JDK-8277756) > -> When creating a `DatePicker` with the second constructor (with `LocalDate` > as parameter) two listener won't be added since they are only added at the > first constructor (That's also why I added the focusProperty listener in the > second constructor). This pull request has now been integrated. Changeset: 6fd4ab61 Author:Marius Hanl Committer: Kevin Rushforth URL: https://git.openjdk.java.net/jfx/commit/6fd4ab6104ebaa97530ea9182466e6c1e346a4f4 Stats: 133 lines in 2 files changed: 133 ins; 0 del; 0 mod 8191995: Regression: DatePicker must commit on focusLost Reviewed-by: fastegal, kcr - PR: https://git.openjdk.java.net/jfx/pull/679
Integrated: 8278425: TreeTableCellStartEditTest uses deprecated TreeTableCell methods
On Fri, 10 Dec 2021 00:12:20 GMT, Marius Hanl wrote: > This PR replaces 2 deprecations which were added in > https://bugs.openjdk.java.net/browse/JDK-8188026 This pull request has now been integrated. Changeset: 79151937 Author:Marius Hanl Committer: Kevin Rushforth URL: https://git.openjdk.java.net/jfx/commit/79151937b68ff4684f09b44979da67cf6c9ade0b Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod 8278425: TreeTableCellStartEditTest uses deprecated TreeTableCell methods Reviewed-by: kcr - PR: https://git.openjdk.java.net/jfx/pull/692
RFR: 8278425: TreeTableCellStartEditTest uses deprecated TreeTableCell methods
This PR replaces 2 deprecations which were added in https://bugs.openjdk.java.net/browse/JDK-8188026 - Commit messages: - 8278425: TreeTableCellStartEditTest uses deprecated TreeTableCell methods Changes: https://git.openjdk.java.net/jfx/pull/692/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=692=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8278425 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jfx/pull/692.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/692/head:pull/692 PR: https://git.openjdk.java.net/jfx/pull/692
Re: RFR: 8191995: Regression: DatePicker must commit on focusLost
On Wed, 24 Nov 2021 09:09:53 GMT, Marius Hanl wrote: > This PR fixes an issue where the `DatePicker` is not committing his value > when the focus is lost. > As the ticket also mentions, this is a regression which last worked on JavaFX > 8 and got broken by the refactoring: > [JDK-8150946](https://bugs.openjdk.java.net/browse/JDK-8150946) > > The fix is to provide the same api to the `DatePicker` which was introduced > by [JDK-8150946](https://bugs.openjdk.java.net/browse/JDK-8150946) for > `ComboBox` and `Spinner`. > > Note: While fixing this I found a possible bug which I tracked here: > [JDK-8277756](https://bugs.openjdk.java.net/browse/JDK-8277756) > -> When creating a `DatePicker` with the second constructor (with `LocalDate` > as parameter) two listener won't be added since they are only added at the > first constructor (That's also why I added the focusProperty listener in the > second constructor). I just finished the CSR. :) - PR: https://git.openjdk.java.net/jfx/pull/679
RFR: 8191995: Regression: DatePicker must commit on focusLost
This PR fixes an issue where the `DatePicker` is not committing his value when the focus is lost. As the ticket also mentions, this is a regression which last worked on JavaFX 8 and got broken by the refactoring: [JDK-8150946](https://bugs.openjdk.java.net/browse/JDK-8150946) The fix is to provide the same api to the `DatePicker` which was introduced by [JDK-8150946](https://bugs.openjdk.java.net/browse/JDK-8150946) for `ComboBox` and `Spinner`. Note: While fixing this I found a possible bug which I tracked here: [JDK-8277756](https://bugs.openjdk.java.net/browse/JDK-8277756) -> When creating a `DatePicker` with the second constructor (with `LocalDate` as parameter) two listener won't be added since they are only added at the first constructor (That's also why I added the focusProperty listener in the second constructor). - Commit messages: - 8191995: Regression: DatePicker must commit on focusLost Changes: https://git.openjdk.java.net/jfx/pull/679/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=679=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8191995 Stats: 133 lines in 2 files changed: 133 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/679.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/679/head:pull/679 PR: https://git.openjdk.java.net/jfx/pull/679
Re: RFR: 8277122: SplitPane divider drag can hang the layout [v2]
> When a divider is moved via drag or code it will call **requestLayout()** for > the **SplitPane**. > While this is fine, it is also called when the > **SplitPaneSkin#layoutChildren(..)** method is repositioning the divider. > > This makes no sense since we are currently layouting everything, so we don't > need to request it again. (The divider positioning is the first part of > **layoutChildren(..)**. In the second part the SplitPane content is layouted > based off those positions) > > -> With this behaviour the layout may hang under some conditions (check > attached source). The problem is that the **requestLayout()** will mark the > **SplitPane** dirty but won't propagate to the parent since the **SplitPane** > is currently doing a layout. > > This PR fixes this by not requesting layout when we are currently doing it > (and thus repositioning the dividers as part of it). > > Note: I also fixed a simple typo of a private method in SplitPaneSkin: > initializeDivderEventHandlers -> initializeDiv**i**derEventHandlers Marius Hanl has updated the pull request incrementally with one additional commit since the last revision: 8277122: Added test for setting a negative divider position + improved readability - Changes: - all: https://git.openjdk.java.net/jfx/pull/669/files - new: https://git.openjdk.java.net/jfx/pull/669/files/919f5db8..9db28ff0 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=669=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=669=00-01 Stats: 27 lines in 2 files changed: 14 ins; 1 del; 12 mod Patch: https://git.openjdk.java.net/jfx/pull/669.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/669/head:pull/669 PR: https://git.openjdk.java.net/jfx/pull/669
Re: RFR: 8277122: SplitPane divider drag can hang the layout
On Fri, 19 Nov 2021 12:39:23 GMT, Jeanette Winzenburg wrote: > interesting issue - and fix :) Verified the mis-behaviour before the fix and > its working after. > > Wondering, though (read: I don't understand ) > > * why the layout details in the splitpane hinders the visual update of a > completely unrelated component (like the combo)? > * why does it only happen on increasing the divider pos, not on decreasing? > > As to the test: would prefer to also see a test of the fixed effect (vs. the > fix implementation of not re-entering layout) - might be a bit tricky, though. - the combobox is a children of the splitpane, so therefore it is affected by the invalid layout state of the splitpane. - it can also happen when decreasing, it might be that you need to set the min width to 0 of the left content of the splitpane. -> The bug is only happening if you drag the divider while being either as far on the left as possible or as far as possible on the right, so that a drag won't affect the divider since it is already at the min/max position. The layout code whoever will basically adjust the divider a second time since it would now be out of bounds Do you mean something like checking the combobox display text? - PR: https://git.openjdk.java.net/jfx/pull/669
Re: RFR: 8197991: Selecting many items in a TableView is very slow
On Wed, 17 Nov 2021 05:34:46 GMT, Abhinay Agarwal wrote: > This work improves the performance of `MultipleSelectionModel` over large > data sets by caching some values and avoiding unnecessary calls to > `SelectedIndicesList#size`. It further improves the performance by reducing > the number of iterations required to find the index of an element in the > BitSet. > > The work is based on [an abandoned > patch](https://github.com/openjdk/jfx/pull/127) submitted by @yososs > > There are currently 2 manual tests for this fix. Just wondering, isn't it also possible to write some unit tests for the MultipleSelectionModel(Base) ? - PR: https://git.openjdk.java.net/jfx/pull/673
RFR: 8277122: SplitPane divider drag can hang the layout
When a divider is moved via drag or code it will call **requestLayout()** for the **SplitPane**. While this is fine, it is also called when the **layoutChildren(..)** method is repositioning the divider. This makes no sense since we are currently layouting everything, so we don't need to request it again (-> We are doing it right now). After positioning the dividers the **SplitPane** content is layouted (based of the may new positioned dividers). This PR fixes this by not requesting layout when we are currently doing it (and thus repositioning the dividers as part of it). Note: I also fixed a simple typo of a private method in SplitPaneSkin: initializeDivderEventHandlers -> initializeDiv**i**derEventHandlers - Commit messages: - 8277122: SplitPane divider drag can hang the layout Changes: https://git.openjdk.java.net/jfx/pull/669/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=669=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8277122 Stats: 61 lines in 2 files changed: 54 ins; 1 del; 6 mod Patch: https://git.openjdk.java.net/jfx/pull/669.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/669/head:pull/669 PR: https://git.openjdk.java.net/jfx/pull/669
Re: RFR: 8274061: Tree-/TableRowSkin: misbehavior on switching skin [v2]
On Mon, 1 Nov 2021 12:54:12 GMT, Jeanette Winzenburg wrote: >> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableRowSkin.java >> line 365: >> >>> 363: // Fix for RT-27782: Need to set isDirty to true, >>> rather than the >>> 364: // cheaper updateCells, as otherwise the text >>> indentation will not >>> 365: // be recalculated in >>> TreeTableCellSkin.leftLabelPadding() >> >> Actually this comment is not correct anymore since my PR got merged >> (https://github.com/openjdk/jfx/pull/568). >> Instead, it should be `TreeTableCellSkin.calculateIndentation()`. > > well .. that would be a merge conflict, had you updated the code comment in > your PR As noted in my comments to Ajit's review, the listener registration > is simply moved (including the code comment .. belatedly :) > > Not sure how to handle it from here - following the rules, we might need a > follow-up issue to the issue fixed in your PR? My PR is already merged, so this is not a problem. :) I dont know, but since this is only fixing a (also before) wrong comment it might be okay as it is very minor? :) - PR: https://git.openjdk.java.net/jfx/pull/632
Re: RFR: 8274061: Tree-/TableRowSkin: misbehavior on switching skin [v2]
On Wed, 27 Oct 2021 09:56:46 GMT, Jeanette Winzenburg wrote: >> Cleanup of Tree-/TableRowSkin to support switching skins >> >> The misbehavior/s >> - memory leaks due to manually registered listeners that were not removed >> - side-effects due to listeners still active on old skin (like NPEs) >> >> Fix >> - use skin api for all listener registration (for automatic removal in >> dispose) >> - do not install listeners that are not needed (fixedCellSize, same as in >> fix of ListCellSkin >> [JDK-8246745](https://bugs.openjdk.java.net/browse/JDK-8246745)) >> >> Added tests for each listener involved in the fix to guarantee it's still >> working and does not have unwanted side-effects after switching skins. >> >> Note: there are pecularities in row skins (like not updating themselves on >> property changes of its control, throwing NPEs when not added to a >> VirtualFlow) which are not part of this issue but covered in >> [JDK-8274065](https://bugs.openjdk.java.net/browse/JDK-8274065) > > Jeanette Winzenburg has updated the pull request incrementally with one > additional commit since the last revision: > > re-added forgotten code comments modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableRowSkin.java line 365: > 363: // Fix for RT-27782: Need to set isDirty to true, rather > than the > 364: // cheaper updateCells, as otherwise the text > indentation will not > 365: // be recalculated in > TreeTableCellSkin.leftLabelPadding() Actually this comment is not correct anymore since my PR got merged (https://github.com/openjdk/jfx/pull/568). Instead, it should be `TreeTableCellSkin.calculateIndentation()`. - PR: https://git.openjdk.java.net/jfx/pull/632
Integrated: 8222455: JavaFX error loading glass.dll from cache
On Tue, 26 Oct 2021 12:09:19 GMT, Marius Hanl wrote: > This problem can happen when using multiple instances with different jfx > early access (ea) versions. > > Example: > Instance 1 uses 18-ea+4 and Instance 2 uses 18-ea+1. > Instance 1 is started (and will cache and use libraries), then instance 2. > Now instance 2 detects via a hash comparison that the cached libraries are > not the same as the supplied ones. > With this information instance 2 tries to delete the old libraries but fails > while doing so (as instance 1 uses them currently) and will terminate right > after. > > The problem here is that instance 1 and 2 are using the same cache folder: > **18-ea**. This is because the `NativeLibLoader` uses the `javafx.version` > property for determining the folder name, which in case of an ea version will > always be **18-ea** (for all ea versions starting with 18 obviously). > > Fix as also mentioned in the ticket is to use the `javafx.runtime.version` > property instead. > With this the folders will be named 18-ea+1 and 18-ea+4. This pull request has now been integrated. Changeset: 6c881063 Author:Marius Hanl Committer: Kevin Rushforth URL: https://git.openjdk.java.net/jfx/commit/6c8810634ec63af8116dd978656805b985eec800 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8222455: JavaFX error loading glass.dll from cache Reviewed-by: jvos, kcr - PR: https://git.openjdk.java.net/jfx/pull/654
Integrated: 8274699: Certain blend modes cannot be set from CSS
On Wed, 6 Oct 2021 07:43:15 GMT, Marius Hanl wrote: > This PR fixes a bug where the blend mode will be falsely recognized as a > color and therefore won't be set. > Also a ClassCastException is thrown (The converter expects a String, but gets > a Color). > > Note: There is another similar bug but I can't reproduce it (Tried with > 18-ea+3). > https://bugs.openjdk.java.net/browse/JDK-8268657 > It also looks different so I don't think this PR fixes this, if it is still > there. This pull request has now been integrated. Changeset: c6f4ff01 Author:Marius Hanl Committer: Kevin Rushforth URL: https://git.openjdk.java.net/jfx/commit/c6f4ff01f2af7d2616b0b49a51ded3ec69f7b0a5 Stats: 13 lines in 2 files changed: 9 ins; 1 del; 3 mod 8274699: Certain blend modes cannot be set from CSS Reviewed-by: kcr, mstrauss - PR: https://git.openjdk.java.net/jfx/pull/640
Re: RFR: 8274699: Certain blend modes cannot be set from CSS [v2]
On Fri, 29 Oct 2021 14:34:39 GMT, Michael Strauß wrote: > Looks good, although the solution in itself remains fishy. Maybe create a > ticket to implement a better solution in the future? yes, the css code is sometimes very ... interesting. I didn't had a deep look but it's weird that there are converters - which converts the value while its also done here. A cleanup generally is not a bad idea here. That's also visible by the amount of TODOs in the code. - PR: https://git.openjdk.java.net/jfx/pull/640
Re: RFR: 8222455: JavaFX error loading glass.dll from cache
On Wed, 27 Oct 2021 08:12:05 GMT, Johan Vos wrote: > This looks like a good solution, and I agree it fixes the issue. I have 2 > minor concerns: > > 1. Are we sure there are no platform-specific restrictions when using a `+` > in a filename? > 2. Testing this is probably difficult. We can only test it if a build is > created (and the versionInfo is set). > > I think the second concern can not be handled by our current test battery, > but we do some smoke tests before releasing the maven repository, which > should be able to detect this. If you are confident about the first concern, > I think we're all good on this. I tested the change by manipulating the folder value in `NativeLibLoader` on multiple jfx versions. That worked well. Today I tested the '+' sign for folders on windows (nfts) and different formatted usb sticks (Tried FAT, FAT32, exFat), linux and mac. They all work. If we want to be really safe we can replace a + by - or something else, but not sure if really needed. I agree it's hard to test. I also thought of writing a test but it's not really possible. The libraries are also loaded different (no cache directory is created). - PR: https://git.openjdk.java.net/jfx/pull/654
RFR: 8222455: JavaFX error loading glass.dll from cache
This problem can happen when using multiple instances with different jfx early access (ea) versions. Example: Instance 1 uses 18-ea+4 and Instance 2 uses 18-ea+1. Instance 1 is started (and will cache and use libraries), then instance 2. Now instance 2 detects via a hash comparison that the cached libraries are not the same as the supplied ones. With this information instance 2 tries to delete the old libraries but fails while doing so (as instance 1 uses them currently) and will terminate right after. The problem here is that instance 1 and 2 are using the same cache folder: **18-ea**. This is because the `NativeLibLoader` uses the `javafx.version` property for determining the folder name, which in case of an ea version will always be **18-ea** (for all ea versions starting with 18 obviously). Fix as also mentioned in the ticket is to use the `javafx.runtime.version` property instead. With this the folders will be named 18-ea+1 and 18-ea+4. - Commit messages: - 8222455: Using javafx.runtime.version as cache directory name so that different ea versions use the same folder Changes: https://git.openjdk.java.net/jfx/pull/654/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=654=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8222455 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/654.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/654/head:pull/654 PR: https://git.openjdk.java.net/jfx/pull/654
Integrated: 8274669: Dialog sometimes ignores max height
On Sat, 2 Oct 2021 23:53:02 GMT, Marius Hanl wrote: > This PR fixes a visual glitch which may happen when showing a dialog. > When a max height is set and the pref height of the dialog content is bigger > the dialog starts to flicker between the max height and the pref height. > > This happens because in one case the height is not bound between the min and > the max height (-> max height is ignored). > - First layout pass: The dialog will incorrectly resize to a the pref height, > which is bigger than the max height > - Second layout pass: The dialog will correctly resize to the max height > - repeat > > The fix is to bound the height there as well (Fix in **layoutChildren()**). > Example where a red stackpane has a bigger pref height then the max height of > the dialog (see also example in ticket): > > https://user-images.githubusercontent.com/66004280/135734463-03b422f4-710d-4436-9715-c91bdb768d76.mp4 > > * only the max height test fails before and succeeds after. This pull request has now been integrated. Changeset: 717cfdc8 Author:Marius Hanl Committer: Ajit Ghaisas URL: https://git.openjdk.java.net/jfx/commit/717cfdc85817aee57d5326e592340c849382d7a4 Stats: 108 lines in 2 files changed: 107 ins; 0 del; 1 mod 8274669: Dialog sometimes ignores max height Reviewed-by: aghaisas - PR: https://git.openjdk.java.net/jfx/pull/637
Aw: Easier handling of ObservableList/Set/Map change events
I like this idea.
Pretty sure some listener I wrote won't handle a permutation correctly.
I may have one question: Is there something that needs to be done in
case of an update (wasUpdated)?
-- Marius
Gesendet: Mittwoch, 20. Oktober 2021 um 21:07 Uhr
Von: "Michael Strau�"
An: "openjfx-dev@openjdk.java.net List"
Betreff: Easier handling of ObservableList/Set/Map change events
I'd like to gauge interest on a small feature addition for JavaFX
collections.
ObservableList (and similarly, ObservableSet/ObservableMap) allows
developers to register ListChangeListeners to observe changes to the
list. In some cases, these changes are applied to another list or
projected into a different form.
Implementing ListChangeListener correctly is quite tricky, especially
if the ObservableList implementation also fires "replace" and
"permutate" events. As a result, it is very easy to implement this
interface wrongly, which often goes undetected when the implementation
is only tested against basic operations like "add" and "remove".
Maybe we could make it easier for developers to get it right more of
the time, by offering pre-built adapters for ListChangeListener,
SetChangeListener and MapChangeListener.
Here's what that could look like:
public abstract class ListChangeListenerAdapter implements
ListChangeListener {
// Basic methods
public abstract void onAdded(int index, E element);
public abstract void onRemoved(int index);
// Optional methods
public void onAdded(int index, List elements);
public void onRemoved(int from, int to);
public void onReplaced(int index, E element);
public void onUpdated(int index, E element);
}
An implementation of this adapter must at the very least implement the
basic `onAdded` and `onRemoved` methods. The adapter will then
correctly map all other change events ("replace" and "permutate") to
these methods. All adapter methods will always be called in exactly
the right order, such that they always refer to the current state of
the list.
An adapter implementation can improve performance characteristics by
overriding the optional `onAdded`, `onRemoved` and `onReplaced`
methods (which map to `addAll`, `remove` and `set` of the List
interface). Optional methods are implemented by default to throw a
private exception in ListChangeListenerAdapter, which is used to
discover whether the methods are overridden and should be called
instead of the basic methods.
Re: RFR: 8187474: Tree-/TableCell, TreeCell: editingCell/Item not updated in cell.startEdit [v3]
On Fri, 22 Oct 2021 10:36:24 GMT, Jeanette Winzenburg wrote: >> cell startEdit is supposed to update the editing location on its associated >> control - was done in ListCell, not in Tree-/TableCell nor TreeCell. >> >> Fix was to add control.edit(..). Note that ListCell was also touched to use >> the exact same method call pattern as the fixed cell types. >> >> Added/unignored cell tests that are failing/passing before/after the fix. > > Jeanette Winzenburg has updated the pull request incrementally with one > additional commit since the last revision: > > next try with fixing the formatting Looks good. - Marked as reviewed by mhanl (Author). PR: https://git.openjdk.java.net/jfx/pull/638
Re: RFR: 8187474: Tree-/TableCell, TreeCell: editingCell/Item not updated in cell.startEdit [v2]
On Fri, 22 Oct 2021 10:15:59 GMT, Jeanette Winzenburg wrote: >> darn .. ;) Thanks > > hmm .. I'm all for consistency, so don't mind trying again but ... what _is_ > the formatting rule? Searching in controls: > > - wildcard search: `` = 1000+ vs. `` = 379 > - verbatim: `` = 173 vs. `` = 98 > > Looks .. inconclusive .. ? > > But then, [generics > tutorial](https://docs.oracle.com/javase/tutorial/java/generics/types.html) > has the space - so the space it will be. Weird. Also standard classes like HashMap, AbstractMap or just Map doesn't have the space. I'm also for consisteny, but don't know which would be 'the best' - PR: https://git.openjdk.java.net/jfx/pull/638
Re: RFR: 8187474: Tree-/TableCell, TreeCell: editingCell/Item not updated in cell.startEdit [v2]
On Fri, 15 Oct 2021 10:21:12 GMT, Jeanette Winzenburg wrote: >> cell startEdit is supposed to update the editing location on its associated >> control - was done in ListCell, not in Tree-/TableCell nor TreeCell. >> >> Fix was to add control.edit(..). Note that ListCell was also touched to use >> the exact same method call pattern as the fixed cell types. >> >> Added/unignored cell tests that are failing/passing before/after the fix. > > Jeanette Winzenburg has updated the pull request incrementally with one > additional commit since the last revision: > > fixed formatting as suggested in review > > and removed unused (by this fix) import in same file modules/javafx.controls/src/test/java/test/javafx/scene/control/TableCellTest.java line 548: > 546: int editingRow = 1; > 547: cell.updateIndex(editingRow); > 548: TablePosition editingCell = new TablePosition<>(table, > editingRow, editingColumn); Just saw you added the space for the `` in line 559 (which I didn't saw ) but not here :) - PR: https://git.openjdk.java.net/jfx/pull/638
Re: RFR: 8218745: TableView: visual glitch at borders on horizontal scrolling
On Fri, 24 Sep 2021 12:42:31 GMT, Kevin Rushforth wrote: >> This PR fixes an issue which is probably in JavaFX since VirtualFlow exists. >> While horizontal scrolling any VirtualFlow control the left blue border >> sometimes disappear/gets smaller. (see also image below) >> >> This can be fixed by **snapping** the scroll bar value (similar like e.g. a >> **ScrollPane** does). >> The same needs to be done on the table header as otherwise the column lines >> might be 1 px off to the cell lines. >> As a side effect this also fixes that the column lines sometimes get's >> blurry when horizontal scrolling (see second image). >> >> While testing with **-Dglass.win.uiScale** I found out that the problem is >> not fixed for a scale like 1.25 or 1.5, while it is fixed for 1 or 2. The >> border sometimes disappears only when the snapped value is a decimal number >> (which obviously does not happen on a scale of 1 or 2), e.g. something like >> 12.6 but it will never disappear when it's a normal number, so e.g. just 12. >> >> That's why something like **Math.round(..)** or just a **cast** to an >> **int** instead of snapping fixes this problem for all scales. I also didn't >> notice any side effect. But not sure if this the right fix then. >> How does JavaFX render a **node** when e.g. the x is a decimal number? And >> does a decimal number make sense (Why we e.g. don't round the value)? >> >> Another explanation could also be that there is an issue somewhere deep >> inside the node layout/snapping/Clip/Group/pixel rendering and to simply >> round/cast the value just fixes the symptom here. >> >> In any case I'm open for any feedback, help or explaination. >> I'm also glad for anything which might help identify the root cause here, if >> any. >> >> --- >> 1. >>  >> --- >> 2. >>  > > modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java > line 3082: > >> 3080: double snappedClipX = snapPositionX(clipX); >> 3081: setLayoutX(-snappedClipX); >> 3082: clipRect.setLayoutX(snappedClipX); > > This is likely not the right place to snap the coordinates to a pixel > boundary. This is usually done as part of layout. I'm also skeptical of the > fact that you added it to `setClipX` but not `setClipY`. I didn't set **clipY** because it's not part of the story and **clipY** is always set to 0 when the `VirtualFlow` is vertical (which is the default for all `VirtualFlowContainer`. Only `ListView` **can** be non-vertical - This should be another bug and test case). That's why there is no issue with it normally. I'm not sure if this is the wrong place. E.g. `ScrollPaneSkin` also listens on the scrollbar **valueProperty** and sets the **snapped** scrollbar value with **setLayoutX** - which is very similar then here. We set the **layoutX** here as well (for clip and the container). Finally: While I understand it does not fix every scale, isn't it at least a good first step? (When we agree this is the correct location here) - PR: https://git.openjdk.java.net/jfx/pull/630
Re: RFR: 8274699: Certain blend modes cannot be set from CSS [v2]
> This PR fixes a bug where the blend mode will be falsely recognized as a > color and therefore won't be set. > Also a ClassCastException is thrown (The converter expects a String, but gets > a Color). > > Note: There is another similar bug but I can't reproduce it (Tried with > 18-ea+3). > https://bugs.openjdk.java.net/browse/JDK-8268657 > It also looks different so I don't think this PR fixes this, if it is still > there. Marius Hanl has updated the pull request incrementally with two additional commits since the last revision: - 8274699: Revert whitespace - 8274699: Revert minor code change - Changes: - all: https://git.openjdk.java.net/jfx/pull/640/files - new: https://git.openjdk.java.net/jfx/pull/640/files/2a18bf24..302b6425 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=640=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=640=00-01 Stats: 9 lines in 1 file changed: 5 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jfx/pull/640.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/640/head:pull/640 PR: https://git.openjdk.java.net/jfx/pull/640
Re: RFR: 8274699: Certain blend modes cannot be set from CSS
On Thu, 14 Oct 2021 17:45:40 GMT, Kevin Rushforth wrote:
>> This PR fixes a bug where the blend mode will be falsely recognized as a
>> color and therefore won't be set.
>> Also a ClassCastException is thrown (The converter expects a String, but
>> gets a Color).
>>
>> Note: There is another similar bug but I can't reproduce it (Tried with
>> 18-ea+3).
>> https://bugs.openjdk.java.net/browse/JDK-8268657
>> It also looks different so I don't think this PR fixes this, if it is still
>> there.
>
> modules/javafx.graphics/src/main/java/javafx/css/CssParser.java line 822:
>
>> 820: int ttype = root.token.getType();
>> 821:
>> 822: if (ttype != CssLexer.STRING && ttype != CssLexer.IDENT ||
>> str == null || str.isEmpty()) {
>
> Why did you modify this code? It looks conceptually identical to the
> previous, but without a check for `root.token == null`. As it is, you will
> need to prove that `root.token` can never be `null` (I suspect it cannot be,
> but since `root` is an instance variable you will need to prove that no call
> earlier in this method can change it as a side effect). I recommend either
> restoring the previous logic to minimize the diffs of your fix, or else
> adding a check for `token == null`.
Right, I removed it because it is checked on top of this method (Line 705), and
this allowed to tidy the code a bit as this big if condition is quite
confusing. But since I will readd the null check, it's probably the best to
restore the behaviour.
-
PR: https://git.openjdk.java.net/jfx/pull/640
Re: RFR: 8187474: Tree-/TableCell, TreeCell: editingCell/Item not updated in cell.startEdit
On Tue, 5 Oct 2021 13:18:07 GMT, Jeanette Winzenburg wrote: > cell startEdit is supposed to update the editing location on its associated > control - was done in ListCell, not in Tree-/TableCell nor TreeCell. > > Fix was to add control.edit(..). Note that ListCell was also touched to use > the exact same method call pattern as the fixed cell types. > > Added/unignored cell tests that are failing/passing before/after the fix. Looks good, just left one minor comment. :) modules/javafx.controls/src/test/java/test/javafx/scene/control/TableCellTest.java line 549: > 547: int editingRow = 1; > 548: cell.updateIndex(editingRow); > 549: TablePosition editingCell = new TablePosition<>(table, > editingRow, editingColumn); Minor: There is a space missing in `` - Marked as reviewed by mhanl (Author). PR: https://git.openjdk.java.net/jfx/pull/638
Re: RFR: 8187474: Tree-/TableCell, TreeCell: editingCell/Item not updated in cell.startEdit
On Wed, 6 Oct 2021 15:34:14 GMT, Jeanette Winzenburg wrote: > > Interesting, I just saw that it worked before because of the > > TableCellBehavior (edit method). Does this mean this can be removed from > > the behaviour in future? > > hmm .. the behavior talks directly to the control (not the cell) by invoking > control.edit(...) - which might be a problem (or not, didn't look closely yet > - left to later when dealing with with big big edit issue ;) > > The issue here is cell.startEdit which must call the control.edit(...) to > switch the control into editing, independent on other parties. Yeah right, just want to share my findings though. :) The whole behaviour stuff is weird anyway, also given that there is still no way do change the behaviour without accessing the api. - PR: https://git.openjdk.java.net/jfx/pull/638
Re: RFR: 8187474: Tree-/TableCell, TreeCell: editingCell/Item not updated in cell.startEdit
On Tue, 5 Oct 2021 13:18:07 GMT, Jeanette Winzenburg wrote: > cell startEdit is supposed to update the editing location on its associated > control - was done in ListCell, not in Tree-/TableCell nor TreeCell. > > Fix was to add control.edit(..). Note that ListCell was also touched to use > the exact same method call pattern as the fixed cell types. > > Added/unignored cell tests that are failing/passing before/after the fix. And by the way, you know why there is a requestFocus() in List/TreeCell, but not in the other two cells? :) - PR: https://git.openjdk.java.net/jfx/pull/638
Re: RFR: 8187474: Tree-/TableCell, TreeCell: editingCell/Item not updated in cell.startEdit
On Tue, 5 Oct 2021 13:18:07 GMT, Jeanette Winzenburg wrote: > cell startEdit is supposed to update the editing location on its associated > control - was done in ListCell, not in Tree-/TableCell nor TreeCell. > > Fix was to add control.edit(..). Note that ListCell was also touched to use > the exact same method call pattern as the fixed cell types. > > Added/unignored cell tests that are failing/passing before/after the fix. Interesting, I just saw that it worked before because of the TableCellBehavior (edit method). Does this mean this can be removed from the behaviour in future? - PR: https://git.openjdk.java.net/jfx/pull/638
RFR: 8274699: Certain blend modes cannot be set from CSS
This PR fixes a bug where the blend mode will be falsely recognized as a color and therefore won't be set. Also a ClassCastException is thrown (The converter expects a String, but gets a Color). Note: There is another similar bug but I can't reproduce this (Tried on 18-ea+3). https://bugs.openjdk.java.net/browse/JDK-8268657 - Commit messages: - 8274699: Blend mode won't be detected and converted as color anymore Changes: https://git.openjdk.java.net/jfx/pull/640/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=640=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8274699 Stats: 21 lines in 2 files changed: 9 ins; 6 del; 6 mod Patch: https://git.openjdk.java.net/jfx/pull/640.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/640/head:pull/640 PR: https://git.openjdk.java.net/jfx/pull/640
RFR: 8274669: Dialog sometimes ignores max height
This PR fixes a visual glitch which may happen when showing a dialog. When a max height is set and the pref height of the dialog content is bigger the dialog starts to flicker between the max height and the pref height. This happens because in one case the height is not bound between the min and the max height (-> max height is ignored). - First layout pass: The dialog will incorrectly resize to a the pref height, which is bigger than the max height - Second layout pass: The dialog will correctly resize to the max height - repeat The fix is to bound the height there as well (Fix in **layoutChildren()**). Example where a red stackpane has a bigger pref height then the max height of the dialog (see also example in ticket): https://user-images.githubusercontent.com/66004280/135734463-03b422f4-710d-4436-9715-c91bdb768d76.mp4 * only the max height test fails before and succeeds after. - Commit messages: - 8274669: Dialog sometimes ignores max height Changes: https://git.openjdk.java.net/jfx/pull/637/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=637=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8274669 Stats: 108 lines in 2 files changed: 107 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/637.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/637/head:pull/637 PR: https://git.openjdk.java.net/jfx/pull/637
Re: RFR: 8274433: All Cells: misbehavior of startEdit [v2]
On Fri, 1 Oct 2021 14:59:02 GMT, Jeanette Winzenburg wrote: >> The misbehavior happens if (super) startEdit didn't succeed (== >> !cell.isEditing): >> >> - must not fire editStart event >> - must not update control's editing location >> >> fix is to back out of startEdit if super.startEdit doesn't switch the cell >> into editing mode >> >> Added tests that failed/passed before/after the fix. Note that for Tree-, >> Table-, TreeTableCell one of the added tests would be a false green due to >> those cells not updating the editing location on its control >> [JDK-8187474](https://bugs.openjdk.java.net/browse/JDK-8187474), so it's >> ignore until that's fixed. > > Jeanette Winzenburg has updated the pull request incrementally with one > additional commit since the last revision: > > test cleanup, following review comments Looks good! - Marked as reviewed by mhanl (Author). PR: https://git.openjdk.java.net/jfx/pull/636
Aw: Re: JavaFX snapping and scale
Thanks for your answer. Then one more question: How is a non-integer value rendered then? Say we have snapped x value of 10.4 (scale 1.25). As we can just render on a whole pixel, what will happen? - Marius Gesendet: Dienstag, 28. September 2021 um 01:57 Uhr Von: "Kevin Rushforth" An: openjfx-dev@openjdk.java.net Betreff: Re: JavaFX snapping and scale The basic idea behind snapping to pixel boundaries (which is optional, but on by default) is that 2D primitives will look more crisp if they are rendered on whole pixel boundaries. When there is no HiDPI involved, the operation of snapping to a pixel boundary can be done with a simple floor, round, or ceil operation (depending on what you are snapping). When there is a HIDPI scale involved, the value in user space needs to be chosen such that the transformed value ends up on a pixel boundary. That's why you will see snapped values that aren't on integer boundaries when the HiDPI scale is, say 1.25. In the case you ran into, the problem might be that the snapping isn't happening in the right place in the computation. Also, it seems quite possible that the clipping isn't being calculated correctly with respect to pixel snapping. You can take a look at JDK-8211294 [1] which was fixed by PR #308 [2] for a recent example of a HiDPI bug affecting ScrollPane that was fixed. There are a couple of follow-on issues that came out of that issue, although I suspect they aren't relevant to the bug you are looking at. -- Kevin [1] [1]https://bugs.openjdk.java.net/browse/JDK-8211294 [2] [2]https://github.com/openjdk/jfx/pull/308 On 9/27/2021 2:41 AM, Marius Hanl wrote: > I recently tried to fix "TableView: visual glitch at borders on > horizontal scrolling". > Ticket: [1][3]https://bugs.openjdk.java.net/browse/JDK-8218745 > PR: [2][4]https://github.com/openjdk/jfx/pull/630 > > As also written in the PR I have problems understanding the > snapping/scaling of JavaFX. > Here in short what I found out: > - Snapping fixed the issue for a scale of 1 or 2, but not for a scale > like 1.25 or 1.5 > --- Also VirtualFlow is the only occurence where we set the layoutX of > a clip (might be the problem?) > - This visual glitch only happens sometimes when the x value is a > decimal number, e.g. 12.66 (never when it's a round number like 13) > - Math.round(..) or a cast to int fixed this (for all scales), but is > probably not the correct solution or maybe only fixing a symptom here > > Which leads to my question where may some of you can help me: > - How does JavaFX renders a node when e.g. x is a decimal number? How > many pixel are used then? > - And does a decimal number make sense (Why we e.g. don't round the > value), which looks like it works fine and doesn't result in visual > glitches > > Also information/insights about fixes made in the past which relates > with this are welcome. > I saw that there were quite some issues with a scale other then 1 in > the past. > > Note: If the result is that everything works as expected chances are > there might a generic problem with snapping/layout/rendering somewhere > then. > Any information are welcome and feel free to also have a look at the > PR. > > -- Marius > > References > > 1. [5]https://bugs.openjdk.java.net/browse/JDK-8218745 > 2. [6]https://github.com/openjdk/jfx/pull/630 References 1. https://bugs.openjdk.java.net/browse/JDK-8211294 2. https://github.com/openjdk/jfx/pull/308 3. https://bugs.openjdk.java.net/browse/JDK-8218745 4. https://github.com/openjdk/jfx/pull/630 5. https://bugs.openjdk.java.net/browse/JDK-8218745 6. https://github.com/openjdk/jfx/pull/630
Re: RFR: 8274433: All Cells: misbehavior of startEdit
On Thu, 30 Sep 2021 12:00:16 GMT, Jeanette Winzenburg
wrote:
> The misbehavior happens if (super) startEdit didn't succeed (==
> !cell.isEditing):
>
> - must not fire editStart event
> - must not update control's editing location
>
> fix is to back out of startEdit if super.startEdit doesn't switch the cell
> into editing mode
>
> Added tests that failed/passed before/after the fix. Note that for Tree-,
> Table-, TreeTableCell one of the added tests would be a false green due to
> those cells not updating the editing location on its control
> [JDK-8187474](https://bugs.openjdk.java.net/browse/JDK-8187474), so it's
> ignore until that's fixed.
Change looks good, I just left some comments on the tests. :)
modules/javafx.controls/src/test/java/test/javafx/scene/control/ListCellTest.java
line 860:
> 858: cell.updateListView(list);
> 859: cell.updateIndex(list.getItems().size());
> 860: List events = new ArrayList<>();
Very minor: You can use EditEvent here so you don't use the raw generic
(and also on the other locations). But I guess that's just syntax sugar.
modules/javafx.controls/src/test/java/test/javafx/scene/control/ListCellTest.java
line 874:
> 872: cell.startEdit();
> 873: assertFalse("sanity: off-range cell must not be editing",
> cell.isEditing());
> 874: assertEquals("list editing location must not be updated", - 1,
> list.getEditingIndex());
`-1` would look better here (without the space inbetween) given the fact this
is not a mathematical context
modules/javafx.controls/src/test/java/test/javafx/scene/control/TableCellTest.java
line 726:
> 724: int editingRow = table.getItems().size();
> 725: cell.updateIndex(editingRow);
> 726: List events = new ArrayList<>();
`events` is effectively unused here, maybe you forgot the assert here?
modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeCellTest.java
line 870:
> 868: cell.startEdit();
> 869: assertFalse("sanity: off-range cell must not be editing",
> cell.isEditing());
> 870: assertEquals("editing location", null, tree.getEditingItem());
Minor: This can be replaced with `assertNull`, which would also be a bit more
readable :)
modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableCellTest.java
line 1051:
> 1049: cell.startEdit();
> 1050: assertFalse("sanity: off-range cell must not be editing",
> cell.isEditing());
> 1051: assertEquals("editing location", null, tree.getEditingCell());
Minor: This can be replaced with `assertNull`. :)
-
PR: https://git.openjdk.java.net/jfx/pull/636
Integrated: 8188026: TextFieldXXCell: NPE on calling startEdit
On Wed, 7 Jul 2021 22:33:07 GMT, Marius Hanl wrote:
> This PR sets an unified logic to every **startEdit()** method of all Cell
> implementations.
> So startEdit() is always doing the same now:
>
> `super.startEdit();`
> `if (!isEditing()) {
> return;
> }`
>
> This will prevent a NPE while also being cleaner (no more double checks)
> The commits are splitted into 4 base commits:
> - First commit changes the startEdit() for TableCell implementations (8 files)
> - Second commit changes the startEdit() for TreeTableCell implementations (7
> files)
> - Third commit changes the startEdit() for ListCell implementations (7 files)
> - Fourth commit changes the startEdit() for TreeCell implementations (7 files)
>
> While writing tests, I found out that the CheckBoxListCell and the
> CheckBoxTreeCell don't disable their CheckBox, which is wrong.
> In CheckBoxTableCell and CheckBoxTreeTableCell the CheckBox is disabled, but
> they don't check their dependencies e.g. TableColumn for null, leading to a
> NPE if not set.
>
> I wrote a follow-up and assigned it to myself:
> https://bugs.openjdk.java.net/browse/JDK-8270042
> The aim of this should be, that all 4 CheckBox...Cells have a proper CheckBox
> disablement while also being nullsafe.
>
> ~Note 1: I removed the tests in TableCellTest added in
> https://github.com/openjdk/jfx/pull/529 in favor of the new parameterized
> TableCellStartEditTest~
> Note 2: This also fixes https://bugs.openjdk.java.net/browse/JDK-8268295
This pull request has now been integrated.
Changeset: 64aa9263
Author:Marius Hanl
Committer: Jeanette Winzenburg
URL:
https://git.openjdk.java.net/jfx/commit/64aa92631fa5aad9293553e8dd174eab647de2f3
Stats: 934 lines in 28 files changed: 625 ins; 257 del; 52 mod
8188026: TextFieldXXCell: NPE on calling startEdit
8268295: Tree- and TableCell sub implementations should respect the row
editability
Reviewed-by: fastegal, aghaisas
-
PR: https://git.openjdk.java.net/jfx/pull/569
Integrated: 8231644: TreeTableView Regression: Indentation wrong using Label as column content type
On Wed, 7 Jul 2021 00:25:15 GMT, Marius Hanl wrote: > This PR fixes a long standing issue with the TreeTableView indentation. > >  > > In short: > **TreeTableCellSkin** overrides **leftLabelPadding()** to calculate the > indentation (the result of this method is added to **x**). > While this is fine, this method is not always called (by > **LabeledSkinBase#layoutLabelInArea**), e.g. when no text is set. > So when a TreeTableCell only sets a graphic (e.g. via **setGraphic()** in > **updateItem()**), the indentation will be messed up. > > Fixed this by adding the calculated indentation to **x** before we call > **layoutChildren()**. > > We also need/should adjust every other location where `leftLabelPadding()` > was used: > - **computePrefHeight** -> prefWidth() is always called with -1, so nothing > got broken by not overriding this, but we should do it of course to be > accurate in case we do one day. > - **computePrefWidth** -> this is needed for auto sizing. I saw that it was > slightly off without, so this 100% needed. > - **computeMinWidth** -> the min width of a cell is not used, so nothing got > broken by not overriding this but same as in computePrefHeight(), we should > comply with the specs. > - **layoutChildren** -> I saw a slight off sizing if the indentation is not > subtracted to the width. > > As a result of this, all method do effectively the same as they did with an > overridden `leftLabelPadding()` (just earlier/later and always now of course). > Note: I also added some tests which pass before and pass after. This pull request has now been integrated. Changeset: 30f56069 Author:Marius Hanl Committer: Kevin Rushforth URL: https://git.openjdk.java.net/jfx/commit/30f56069dacf32e43e2c98d05ba252ffc4bd9fe9 Stats: 210 lines in 2 files changed: 197 ins; 0 del; 13 mod 8231644: TreeTableView Regression: Indentation wrong using Label as column content type Reviewed-by: aghaisas, kcr - PR: https://git.openjdk.java.net/jfx/pull/568
Aw: RFR: 8267606: Style classes not always working
It would be very helpful if you could provide a minimum reproducable example. So really just the code which is necessary for all of us to reproduce it. Am 29.09.21, 00:06 schrieb Alessandro Parisi : Can we have a look at this report please? I'm having this issue more and more often when making custom controls JBS <[1]https://bugs.openjdk.java.net/browse/JDK-8267606> References 1. https://bugs.openjdk.java.net/browse/JDK-8267606
JavaFX snapping and scale
I recently tried to fix "TableView: visual glitch at borders on horizontal scrolling". Ticket: [1]https://bugs.openjdk.java.net/browse/JDK-8218745 PR: [2]https://github.com/openjdk/jfx/pull/630 As also written in the PR I have problems understanding the snapping/scaling of JavaFX. Here in short what I found out: - Snapping fixed the issue for a scale of 1 or 2, but not for a scale like 1.25 or 1.5 --- Also VirtualFlow is the only occurence where we set the layoutX of a clip (might be the problem?) - This visual glitch only happens sometimes when the x value is a decimal number, e.g. 12.66 (never when it's a round number like 13) - Math.round(..) or a cast to int fixed this (for all scales), but is probably not the correct solution or maybe only fixing a symptom here Which leads to my question where may some of you can help me: - How does JavaFX renders a node when e.g. x is a decimal number? How many pixel are used then? - And does a decimal number make sense (Why we e.g. don't round the value), which looks like it works fine and doesn't result in visual glitches Also information/insights about fixes made in the past which relates with this are welcome. I saw that there were quite some issues with a scale other then 1 in the past. Note: If the result is that everything works as expected chances are there might a generic problem with snapping/layout/rendering somewhere then. Any information are welcome and feel free to also have a look at the PR. -- Marius References 1. https://bugs.openjdk.java.net/browse/JDK-8218745 2. https://github.com/openjdk/jfx/pull/630
Re: Re: Gauging interest in updating the JavaFX test framework with JUnit 5
I also really like this move, thanks for that.
I have also experience with JUnit 5.
And if there is something where I may can help I'm glad to do so.
For people who may don't know, this are the features JUnit5 gives
us and I'm also excited about:
- assertThrows(..)
- assertDoesNotThrows(..)
-> No more tests without any assert... :)
- Parameterized tests are much easier to write and can be mixed with
normal tests
- We can provide a display name
- We can provide better display names for parameterized test
- We can decide where the value the parameterized test should use
come from,
this gives us the ability to have multiple parameterized tests in
one class
More: [1]https://junit.org/junit5/docs/current/user-guide/#writing-test
s-parameterized-tests
Example: Hopefully it's ok readable on the mailing list:
- START --
@DisplayName("A parameterized test with named arguments")
@ParameterizedTest(name = "{0} should match with {1} (lower case)")
@MethodSource("initData")
void testToLowerCase(String string1, String string2) {
assertEquals(string1, string2.toLowerCase());
}
static Stream initData() {
return Stream.of(of("aaa", "AAA"), of("bbb", "BBB"), of("ccc", "cCc"));
}
static Arguments of(String string1, String string2) {
return Arguments.of(string1, string2);
}
- END--
-- Marius
Gesendet: Samstag, 25. September 2021 um 15:23 Uhr
Von: "Kevin Rushforth"
An: "Nir Lisker" , "John Hendrikx"
Cc: "openjfx-dev@openjdk.java.net Mailing"
Betreff: Re: Gauging interest in updating the JavaFX test framework
with JUnit 5
I also see advantages in moving to JUnit 5, given that we can still
support both JUint 4 and JUnit 5 in the same project using
jupiter-vintage (thus avoiding the need to rewrite existing tests).
Do any other contributors have experiences with JUnit 5 that they could
share?
-- Kevin
On 9/25/2021 12:16 AM, Nir Lisker wrote:
> I much prefer JUnit 5 to 4, so I'm in favor.
>
> On Sat, Sep 25, 2021 at 12:40 AM John Hendrikx
wrote:
>
>> Posting this to gauge the interest in adding JUnit 5 as a test
>> dependency to JavaFX, enabling writing tests with this new version
of
>> JUnit while still supporting all JUnit 4 tests.
>>
>> A draft PR has been submitted here:
>> [2]https://github.com/openjdk/jfx/pull/633
>>
>> And an issue has been filed here:
>> [3]https://bugs.openjdk.java.net/browse/JDK-8274274
>>
>> Although very personally motivated to be able to write nested unit
tests
>> in future JavaFX pull requests, I think this would be a great
addition
>> to have in our testing arsenal.
>>
>> The main benefits of using JUnit 5:
>>
>> Better integration with Java 8, specifically use of Lambda functions
>> where this would make sense. For example `assertThrows` is really
useful
>> and can replace the `expected` parameter in the Test annotation
>> (although this has also been backported to recent JUnit 4 versions).
>>
>> Support for better organization of tests (using Nested) and better
>> naming. See this image to see Nested and naming in action:
>>
>>
[4]https://user-images.githubusercontent.com/995917/111858133-d6ce0f80-
8936-11eb-9724-be2c15150590.png
>>
>> Better extension system allowing to combine extensions whereas JUnit
4
>> only allowed one extension at a time.
>>
>> Native support for parameterized tests, repeated tests, nested
tests,
>> conditional test execution (by platform or environment for example),
>> assumptions, test generation and timeouts.
>>
>> Please let us know if you'd like to see a newer version of JUnit
>> included in JavaFX to further ease testing :)
>>
>> --John
>>
>>
>>
>>
References
1.
https://junit.org/junit5/docs/current/user-guide/#writing-tests-parameterized-tests
2. https://github.com/openjdk/jfx/pull/633
3. https://bugs.openjdk.java.net/browse/JDK-8274274
4.
https://user-images.githubusercontent.com/995917/111858133-d6ce0f80-8936-11eb-9724-be2c15150590.png
Re: RFR: 8231644: TreeTableView Regression: Indentation wrong using Label as column content type [v3]
On Fri, 24 Sep 2021 10:48:53 GMT, Ajit Ghaisas wrote: > Overall the fix looks ok. The new test fails without the fix and passes with > it. > > Can you please confirm the test programs provided in 2 duplicated bugs of > JDK-8231644 also work as expected with this fix? Just tried them. They both work fine. :) - PR: https://git.openjdk.java.net/jfx/pull/568
Re: RFR: 8231644: TreeTableView Regression: Indentation wrong using Label as column content type [v3]
On Fri, 24 Sep 2021 10:43:31 GMT, Ajit Ghaisas wrote:
>> Marius Hanl has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains three commits:
>>
>> - Merge branch 'master' of https://github.com/openjdk/jfx into
>> 8231644-indentation
>>
>> Conflicts:
>>
>> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableCellSkin.java
>> - calculated indentation in every method now which was previously done via
>> leftLabelPadding
>> - 8231644: fixed wrong indentation for tree cells with graphic only
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableCellSkin.java
> line 127:
>
>> 125: double leftInset) {
>> 126: if (isDeferToParentForPrefWidth) {
>> 127: return super.computePrefWidth(height, topInset, rightInset,
>> bottomInset, leftInset) + calculateIndentation();
>
> If we are deferring to Parent for the preferred width, then we need not add
> indentation.
> I think, we should interchange line 127 and line 130.
No that's actually correct as it is.
That is because when a cell is deferred from the parent we want to know the
width we need without being truncated. For now that is only set when the auto
size of the table is triggered (see also:
**TableColumnHeader#resizeColumnToFitContent**).
And if not deferred, the width of the cell is in sync with the column (you can
also see it in the **super.computePrefWidth**)
-
PR: https://git.openjdk.java.net/jfx/pull/568
RFR: 8218745: TableView: visual glitch at borders on horizontal scrolling
This PR fixes an issue which is probably in JavaFX since VirtualFlow exists. While horizontal scrolling any VirtualFlow control the left blue border sometimes disappear/gets smaller. (see also image below) This can be fixed by **snapping** the scroll bar value (similar like e.g. a **ScrollPane** does). The same needs to be done on the table header as otherwise the column lines might be 1 px off to the cell lines. As a side effect this also fixes that the column lines sometimes get's blurry when horizontal scrolling (see second image). While testing with **-Dglass.win.uiScale** I found out that the problem is not fixed for a scale like 1.25 or 1.5, while it is fixed for 1 or 2. The border sometimes disappears only when the snapped value is a decimal number (which obviously does not happen on a scale of 1 or 2), e.g. something like 12.6 but it will never disappear when it's a normal number, so e.g. just 12. That's why something like **Math.round(..)** or just a **cast** to an **int** instead of snapping fixes this problem for all scales. I also didn't notice any side effect. But not sure if this the right fix then. How does JavaFX render a **node** when e.g. the x is a decimal number? And does a decimal number make sense (Why we e.g. don't round the value)? Another explanation could also be that there is an issue somewhere deep inside the node layout/snapping/Clip/Group/pixel rendering and to simply round/cast the value just fixes the symptom here. In any case I'm open for any feedback, help or explanation. I'm also glad for anything which might help identify the root cause here, if any. --- 1.  --- 2.  - Commit messages: - 8218745: Snapping X/Y when scrolling Changes: https://git.openjdk.java.net/jfx/pull/630/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=630=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8218745 Stats: 59 lines in 6 files changed: 55 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jfx/pull/630.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/630/head:pull/630 PR: https://git.openjdk.java.net/jfx/pull/630
Re: RFR: 8272808: Update constant collections to use the new immutable collections - leftovers
On Tue, 21 Sep 2021 11:13:06 GMT, Nir Lisker wrote: > Replacements of more immutable collections. > > One thing I found was that the field `idMap` in > `com.sun.java.scene.web.WebViewHelper.WebView` seems unused. I can remove it > if that's indeed the case. modules/javafx.graphics/src/main/java/com/sun/javafx/font/WindowsFontMap.java line 44: > 42: } > 43: > 44: private static Map PLATFORM_FONT_MAP; Shouldn't this variable name stay with the normal camel case because it is not final (only static)? - PR: https://git.openjdk.java.net/jfx/pull/627
Re: RFR: 8264591: HBox/VBox child widths pixel-snap to wrong value
On Mon, 29 Mar 2021 19:57:17 GMT, Michael Strauß wrote: > The children of HBox/VBox don't always pixel-snap to the same value as the > container itself when a render scale other than 1 is used. This can lead to a > visual glitch where the content bounds don't line up with the container > bounds. In this case, the container will extend beyond its content, or vice > versa. > > The following program shows the problem for HBox: > > Region r1 = new Region(); > Region r2 = new Region(); > Region r3 = new Region(); > Region r4 = new Region(); > Region r5 = new Region(); > Region r6 = new Region(); > r1.setBackground(new Background(new BackgroundFill(Color.GREY, null, null))); > r2.setBackground(new Background(new BackgroundFill(Color.DARKGRAY, null, > null))); > r3.setBackground(new Background(new BackgroundFill(Color.BLACK, null, null))); > r4.setBackground(new Background(new BackgroundFill(Color.GREY, null, null))); > r5.setBackground(new Background(new BackgroundFill(Color.DARKGRAY, null, > null))); > r6.setBackground(new Background(new BackgroundFill(Color.BLACK, null, null))); > r1.setPrefWidth(25.3); > r2.setPrefWidth(25.3); > r3.setPrefWidth(25.4); > r4.setPrefWidth(25.3); > r5.setPrefWidth(25.3); > r6.setPrefWidth(25.4); > r1.setMaxHeight(30); > r2.setMaxHeight(30); > r3.setMaxHeight(30); > r4.setMaxHeight(30); > r5.setMaxHeight(30); > r6.setMaxHeight(30); > HBox hbox1 = new HBox(r1, r2, r3, r4, r5, r6); > hbox1.setBackground(new Background(new BackgroundFill(Color.RED, null, > null))); > hbox1.setPrefHeight(40); > > r1 = new Region(); > r2 = new Region(); > r3 = new Region(); > r4 = new Region(); > r5 = new Region(); > r6 = new Region(); > r1.setBackground(new Background(new BackgroundFill(Color.GREY, null, null))); > r2.setBackground(new Background(new BackgroundFill(Color.DARKGRAY, null, > null))); > r3.setBackground(new Background(new BackgroundFill(Color.BLACK, null, null))); > r4.setBackground(new Background(new BackgroundFill(Color.GREY, null, null))); > r5.setBackground(new Background(new BackgroundFill(Color.DARKGRAY, null, > null))); > r6.setBackground(new Background(new BackgroundFill(Color.BLACK, null, null))); > r1.setPrefWidth(25.3); > r2.setPrefWidth(25.3); > r3.setPrefWidth(25.4); > r4.setPrefWidth(25.3); > r5.setPrefWidth(25.3); > r6.setPrefWidth(25.4); > r1.setMaxHeight(30); > r2.setMaxHeight(30); > r3.setMaxHeight(30); > r4.setMaxHeight(30); > r5.setMaxHeight(30); > r6.setMaxHeight(30); > HBox hbox2 = new HBox(r1, r2, r3, r4, r5, r6); > hbox2.setBackground(new Background(new BackgroundFill(Color.RED, null, > null))); > hbox2.setPrefHeight(40); > hbox2.setPrefWidth(152); > > VBox root = new VBox(new HBox(hbox1), new HBox(hbox2)); > root.setSpacing(20); > Scene scene = new Scene(root, 500, 250); > > primaryStage.setScene(scene); > primaryStage.show(); > > > Here's a before-and-after comparison of the program above after applying the > fix. Note that 'before', the backgrounds of the container (red) and its > content (black) don't align perfectly. The render scale is 175% in this > example. >  > > I've filed a bug report and will change the title of the GitHub issue as soon > as it's posted. I had a look at the PR. But it took quite a while to fully understand the changes (also the current implementation ). I think it make sense to improve it a bit e.g. by adding javadoc to the new methods, maybe also the existing? Other ideas are also welcome. With that said maybe more people will review it then. I will have a full look soon as well. :) - PR: https://git.openjdk.java.net/jfx/pull/445
Re: RFR: 8271474: Tree-/TableCell: inconsistent edit event firing pattern
On Tue, 7 Sep 2021 14:53:50 GMT, Jeanette Winzenburg wrote: > this PR fixes the inconsistent event firing pattern in cell's xxEdit methods > (please see the issue for more details): > > - fires event if column != null > - accesses table state if table != null > > The first requires a change in CellEditEvent which now allows null table in > its constructor. > > Added tests that failed/passed before/after the fix (along with some that > also passed before for complete coverage of state permutations). Changed two > old test methods which passed/failed before/after the fix - but did test the > wrong thingy anyway ;) Looks good to me. - Marked as reviewed by mhanl (Author). PR: https://git.openjdk.java.net/jfx/pull/620
Re: Re: [External] : Re: Convenience factories for Border and Background
As also written in a comment
here: https://github.com/openjdk/jfx/pull/610
I would like to propose one more convenience method which should be
added to JavaFX:
public static Border stroke(Paint stroke, double width) {
return new Border(new BorderStroke(stroke, BorderStrokeStyle.SOLID, null, ne
w BorderWidths(width)));
}
I think it's quite common that you want to create a solid border with
another width then the default of 1 (for every side).
Note: This is also the last use case I think makes sense to add as a
convenience method.
Any other use case is likely to be so complex that it makes sense to
use the normal existing constructors.
Feel free to share you opinion.
- Marius
Gesendet: Dienstag, 08. Juni 2021 um 03:19 Uhr
Von: "Nir Lisker"
An: "Kevin Rushforth"
Cc: "openjfx-dev@openjdk.java.net Mailing"
Betreff: Re: [External] : Re: Convenience factories for Border and
Background
The new API:
1. `Border.of(Paint stroke)` or `Border.stroke(Paint stroke)` that does
`new Border(new BorderStroke(Paint stroke , BorderStrokeStyle.SOLID,
null,
null));`
2. `Background.of((Paint fill)` or `Background.fill(Paint fill)` that
does
`new Background(new BackgroundFill(Paint fill, null, null));`
I don't mind either name choice.
On Tue, Jun 8, 2021 at 2:50 AM Kevin Rushforth
wrote:
> If I recall, there were a few developers that chimed in. It's a
simple
> enough addition -- at least your original proposal (not the
suggestion of
> mirroring the Color API, which I don't like) -- that it seems OK to
me.
>
> Can you repost your currently proposed API and see if those who might
like
> to use it are satisfied with it?
>
> -- Kevin
>
>
> On 6/7/2021 4:41 PM, Nir Lisker wrote:
>
> Does this constitute sufficient interest in the enhancement?
>
> On Thu, May 13, 2021 at 6:41 PM Michael Strau�
> wrote:
>
>> Another option could be to mirror the `Color` API in both `Border`
and
>> `Background`, like in the following examples:
>>
>> Color.rgb(125, 100, 75)
>> Border.rgb(125, 100, 75)
>> Background.rgb(125, 100, 75)
>>
>> Color.gray(127)
>> Border.gray(127)
>> Background.gray(127)
>>
>> Color.web("orange", 0.5)
>> Border.web("orange", 0.5)
>> Background.web("orange", 0.5)
>>
>> We could also mirror the named color constants, which would enable a
>> very compact syntax:
>>
>> StackPane pane = new StackPane();
>> pane.setBorder(Border.RED);
>> pane.setBackground(Background.BLUE);
>>
>> This is very similar to how "red" or "blue" are valid values for
>> "-fx-border" or "-fx-background" in CSS.
>>
>
>
