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? 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 https://github.com/kleopatra/jfx/tree/dokeep-edit-api-editstate with an overview of suggested changes https://github.com/kleopatra/swingempire-fx/wiki/CellEditAPI and a example driver as gist 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
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 https://github.com/kleopatra/jfx/tree/dokeep-edit-api-editstate with an overview of suggested changes https://github.com/kleopatra/swingempire-fx/wiki/CellEditAPI and a example driver as gist 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
Integrated: 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. This pull request has now been integrated. Changeset: 4cf66d9f Author:Jeanette Winzenburg URL: https://git.openjdk.java.net/jfx/commit/4cf66d9f6e96aab29c4c5fc72fa3e81a38a8d783 Stats: 95 lines in 4 files changed: 89 ins; 4 del; 2 mod 8187309: TreeCell must not change tree's data Reviewed-by: mstrauss, mhanl, aghaisas - PR: https://git.openjdk.java.net/jfx/pull/724
Re: RFR: 8187309: TreeCell must not change tree's data
On Tue, 8 Feb 2022 12:49:59 GMT, Marius Hanl 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. > > 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`. oops .. forgot to change this (and the other comment) before integration - sry - PR: https://git.openjdk.java.net/jfx/pull/724
Re: RFR: 8187309: TreeCell must not change tree's data
On Wed, 9 Feb 2022 10:51:54 GMT, Ajit Ghaisas 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. > > modules/javafx.controls/src/main/java/javafx/scene/control/TreeView.java line > 341: > >> 339: setFocusModel(new TreeViewFocusModel(this)); >> 340: >> 341: setOnEditCommit(DEFAULT_EDIT_COMMIT_HANDLER); > > You are adding the default edit commit handler to TreeView. Although it seems > to be correct, this default handler is likely to be overwritten if the user > code already has a setOnEditCommit() call. This is shown by the > test_rt_29650() failure in TreeviewTest.java which you have corrected. > > The changes done in this PR makes TreeView similar to ListView and TableView > in terms of having the default edit commit handler. > > Unfortunately, I do not see how can we avoid user code accidentally > overwriting the default edit commit handler. Documenting this possibility > seems to be the only way ahead. > Any other idea? Yes, the change might break application code, though that code would be buggy. Actually, the behavior as implemented now, _already is_ documented :) > It is very important to note that if you call > setOnEditCommit(javafx.event.EventHandler) with your own EventHandler, then > you will be removing the default handler. Unless you then handle the > writeback to the property (or the relevant data source), nothing will happen. so: if they have a custom handler that doesn't save the data, they were violating the specification (though were getting away with it due to the bug). Personally, I think that we cannot keep backward compatibility to bugs. - PR: https://git.openjdk.java.net/jfx/pull/724
Integrated: 8280951: Testbug: fix commented asserts in XXViewTest.test_rt_29650
On Thu, 3 Feb 2022 11:38:55 GMT, Jeanette Winzenburg wrote: > the issue was commented asserts in a test methods - they had been failing due > to incorrect usage of registering an edit commit handler > > fix was to correct the registration and uncomment the assert (ListViewTest). > For Tree/TableViewTest, had to adjust value access also (was: c&p from > ListViewTest). Verified that the uncommented (corrected) asserts are > failing/passing before/after fix of test bug This pull request has now been integrated. Changeset: 929e7c92 Author:Jeanette Winzenburg URL: https://git.openjdk.java.net/jfx/commit/929e7c9217ff7ea04f9bb31bd5c1bf7d74086752 Stats: 10 lines in 3 files changed: 0 ins; 3 del; 7 mod 8280951: Testbug: fix commented asserts in XXViewTest.test_rt_29650 Reviewed-by: kcr - PR: https://git.openjdk.java.net/jfx/pull/725
RFR: 8280951: Testbug: fix commented asserts in XXViewTest.test_rt_29650
the issue was commented asserts in a test methods - they had been failing due to incorrect usage of registering an edit commit handler fix was to correct the registration and uncomment the assert (ListViewTest). For Tree/TableViewTest, had to adjust value access also (was: c&p from ListViewTest). Verified that the uncommented (corrected) asserts are failing/passing before/after fix of test bug - Commit messages: - 8280951: Testbug: fix commented asserts in XXViewTest.test_rt_29650 Changes: https://git.openjdk.java.net/jfx/pull/725/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx&pr=725&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8280951 Stats: 10 lines in 3 files changed: 0 ins; 3 del; 7 mod Patch: https://git.openjdk.java.net/jfx/pull/725.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/725/head:pull/725 PR: https://git.openjdk.java.net/jfx/pull/725
RFR: 8187309: TreeCell must not change tree's data
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. - Commit messages: - 8187309: TreeCell must not change tree's data Changes: https://git.openjdk.java.net/jfx/pull/724/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx&pr=724&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8187309 Stats: 95 lines in 4 files changed: 89 ins; 4 del; 2 mod Patch: https://git.openjdk.java.net/jfx/pull/724.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/724/head:pull/724 PR: https://git.openjdk.java.net/jfx/pull/724
Re: RFR: 8251481: TableCell accessing row: NPE on auto-sizing [v4]
On Tue, 1 Feb 2022 17:41:47 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`). > > Marius Hanl has updated the pull request incrementally with one additional > commit since the last revision: > > 8251481: Added jdk ticket reference in TreeTableCellTest + Grammar okay - Marked as reviewed by fastegal (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/716
Re: JBS: possibilty to merge issues?
will do, thanks Kevin :) Zitat von Kevin Rushforth : Based on your description, it seems fine to combine them. To do this, just closed one of the (probably 8187473) as a duplicate of the other, and add any information from the closed bug into the other one, adjusting the title of the bug if needed. -- Kevin On 1/31/2022 6:57 AM, Jeanette Winzenburg wrote: Currently working on two related issues: https://bugs.openjdk.java.net/browse/JDK-8187309: TreeCell must not change data of treeView https://bugs.openjdk.java.net/browse/JDK-8187473: TreeView must have default commit handler which are so intertwined that there's no way to fix one without the other (we must have a default commit mechanism in place at any time). I could merge them in the PR (also-fixes), but would feel cleaner to merge in JBS, if possible. -- Jeanette
JBS: possibilty to merge issues?
Currently working on two related issues: https://bugs.openjdk.java.net/browse/JDK-8187309: TreeCell must not change data of treeView https://bugs.openjdk.java.net/browse/JDK-8187473: TreeView must have default commit handler which are so intertwined that there's no way to fix one without the other (we must have a default commit mechanism in place at any time). I could merge them in the PR (also-fixes), but would feel cleaner to merge in JBS, if possible. -- Jeanette
Re: RFR: 8251481: TableCell accessing row: NPE on auto-sizing [v3]
On Thu, 27 Jan 2022 20:38:56 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`).
>
> Marius Hanl has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8251481: Using global stageLoader now
looks good (there's one minor thingy in completely aligning the test for
TreeTable)
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 :)
-
Marked as reviewed by fastegal (Reviewer).
PR: https://git.openjdk.java.net/jfx/pull/716
Re: RFR: 8251481: TableCell accessing row: NPE on auto-sizing [v3]
On Fri, 28 Jan 2022 13:14:37 GMT, Marius Hanl wrote: >> 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 just thought I had overlooked something, .. okay, fair enough :) Still think that all tests for autosizing (both for Tree/TableView) have their best home in TableColumnHeaderTest, but then that's my personal preference. - PR: https://git.openjdk.java.net/jfx/pull/716
Re: RFR: 8251481: TableCell accessing row: NPE on auto-sizing [v3]
On Thu, 27 Jan 2022 10:21:18 GMT, Marius Hanl wrote:
>> 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.
just curious: why didn't you move the tests into TableColumnHeaderTest?
> Pretty sure table row is never null. Or is it on some corner case?
updateItem has no precondition :) So a clean implementation must cope with
whatever state the cell is in (getting away with not thinking of potential
corner cases most of the time).
-
PR: https://git.openjdk.java.net/jfx/pull/716
Re: RFR: 8277122: SplitPane divider drag can hang the layout [v4]
On Thu, 27 Jan 2022 20:48:40 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 > > 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 looks good now :) - Marked as reviewed by fastegal (Reviewer). 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 11:42:28 GMT, Marius Hanl wrote: >>> 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. just for reference - found the source of my [faintly remember](https://github.com/openjdk/jfx/pull/337/files/8a6fc1cf6cad2c453de17b71777ddd63fadb539e#r510975640) - PR: https://git.openjdk.java.net/jfx/pull/669
Re: RFR: 8277122: SplitPane divider drag can hang the layout [v2]
On Wed, 26 Jan 2022 20:14:37 GMT, Marius Hanl wrote: > 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. - PR: https://git.openjdk.java.net/jfx/pull/669
Re: RFR: 8277122: SplitPane divider drag can hang the layout [v2]
On Mon, 22 Nov 2021 14:03:56 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
>
> 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
fix looks good (though it _feels_ a bit upside down that it is needed ;) -
verified example/tests failing/passing before/after fixing
left minor inline comments (just to make it easier to understand :)
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
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)
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 ..
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
-
Changes requested by fastegal (Reviewer).
PR: https://git.openjdk.java.net/jfx/pull/669
Re: RFR: 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`).
fix looks good: verified that the bug example doesn't throw after the fix, also
that the test failed/passed before/after the fix
left a couple of inline comments for the test
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)
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 :)
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
-
Changes requested by fastegal (Reviewer).
PR: https://git.openjdk.java.net/jfx/pull/716
Re: Commit bots confused?
Thanks for the cleanup, Kevin - and sry for having created the mess ;) -- Jeanette Zitat von Kevin Rushforth : Yes, this happened because the bug was still targeted to openjfx18. I had meant to go through all of the issues still targeted to openjfx18 earlier this week, but then got busy with (too many) other things. I'll fix up the JBS records. Thanks. -- Kevin On 1/20/2022 4:13 AM, Jeanette Winzenburg wrote: wondering what happened in https://github.com/openjdk/jfx/pull/684 (fix for https://bugs.openjdk.java.net/browse/JDK-8187307) - requested integration (into master) after review approval - the bots did the usual thingies (commit, comment/close the issue in jbs) - the bots did some unusual thingies: creating a backport https://bugs.openjdk.java.net/browse/JDK-8280380 into jfx19 and commented/closed it as fixed immediately) verified that the commit (as of now) really is into master, not fx18 - so the actual commits seem to be clean, it's just that weird auto-backport What might have confused them is that I didn't update the fix version of the issue (it's still fx18) after rampdown (being still in holiday mode ;) Anything I should do? -- Jeanette
Commit bots confused?
wondering what happened in https://github.com/openjdk/jfx/pull/684 (fix for https://bugs.openjdk.java.net/browse/JDK-8187307) - requested integration (into master) after review approval - the bots did the usual thingies (commit, comment/close the issue in jbs) - the bots did some unusual thingies: creating a backport https://bugs.openjdk.java.net/browse/JDK-8280380 into jfx19 and commented/closed it as fixed immediately) verified that the commit (as of now) really is into master, not fx18 - so the actual commits seem to be clean, it's just that weird auto-backport What might have confused them is that I didn't update the fix version of the issue (it's still fx18) after rampdown (being still in holiday mode ;) Anything I should do? -- Jeanette
Integrated: 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. This pull request has now been integrated. Changeset: 4c79c54c Author:Jeanette Winzenburg URL: https://git.openjdk.java.net/jfx/commit/4c79c54c490031a9491790702c6bfb611d88f3b9 Stats: 158 lines in 8 files changed: 117 ins; 29 del; 12 mod 8187307: ListView, TableView, TreeView: receives editCancel event when edit is committed Reviewed-by: mhanl, aghaisas - PR: https://git.openjdk.java.net/jfx/pull/684
Re: Aw: Strange test failures: validation of PGGroup children failed
Zitat von Marius Hanl : 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. thanks for the suggestion - didn't help, unfortunately wondering what is "javafx.graphics@19-internal" - doesn't it sound like some mis-configuration somewhere? (in Eclipse it's the same error message, but without the @19-internal). Will simply ignore it for now (working fine if the test is passing and that's what we are after in the end :), nevertheless itching .. 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 (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.java:201) at javafx.graphics@19-internal/javafx.scene.Parent$1.doUpdatePeer(Parent.java:109) at javafx.graphics@19-internal/com.sun.javafx.scene.ParentHelper.updatePeerImpl(ParentHelper.java:78) at javafx.graphics@19-internal/com.sun.javafx.scene.layout.RegionHelper.updatePeerImpl(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.syncAll(Scene.java:2397) at javafx.graphics@19-internal/javafx.scene.Scene$ScenePulseListener.syncAll(Scene.java:2406) at javafx.graphics@19-internal/javafx.scene.Scene$ScenePulseListener.syncAll(Scene.java:2406) at javafx.graphics@19-internal/javafx.scene.Scene$ScenePulseListener.syncAll(Scene.java:2406) at javafx.graphics@19-internal/javafx.scene.Scene$ScenePulseListener.synchronizeSceneNodes(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.testEditCancelEventAfterModifyItems(TableCellTest.java:557)
Strange test failures: validation of PGGroup children failed
Just stumbled across it while reviewing PR 719 (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.java:201) at javafx.graphics@19-internal/javafx.scene.Parent$1.doUpdatePeer(Parent.java:109) at javafx.graphics@19-internal/com.sun.javafx.scene.ParentHelper.updatePeerImpl(ParentHelper.java:78) at javafx.graphics@19-internal/com.sun.javafx.scene.layout.RegionHelper.updatePeerImpl(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.syncAll(Scene.java:2397) at javafx.graphics@19-internal/javafx.scene.Scene$ScenePulseListener.syncAll(Scene.java:2406) at javafx.graphics@19-internal/javafx.scene.Scene$ScenePulseListener.syncAll(Scene.java:2406) at javafx.graphics@19-internal/javafx.scene.Scene$ScenePulseListener.syncAll(Scene.java:2406) at javafx.graphics@19-internal/javafx.scene.Scene$ScenePulseListener.synchronizeSceneNodes(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.testEditCancelEventAfterModifyItems(TableCellTest.java:557)
Re: RFR: 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`). 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. - PR: https://git.openjdk.java.net/jfx/pull/716
Integrated: 8278134: Move static utility methods to infrastructure (EditAndScrollTest)
On Thu, 9 Dec 2021 12:43:54 GMT, Jeanette Winzenburg wrote: > Extracted static test utility methods from EditAndScrollTest into new > VirtualizedControlTestUtils, added rudimentary tests for the methods. This pull request has now been integrated. Changeset: 002d4f57 Author:Jeanette Winzenburg URL: https://git.openjdk.java.net/jfx/commit/002d4f5734b00c598f97a529c924e26c0451de9c Stats: 348 lines in 3 files changed: 290 ins; 57 del; 1 mod 8278134: Move static utility methods to infrastructure (EditAndScrollTest) Reviewed-by: aghaisas - PR: https://git.openjdk.java.net/jfx/pull/690
Re: RFR: 8278134: Move static utility methods to infrastructure (EditAndScrollTest) [v2]
> Extracted static test utility methods from EditAndScrollTest into new > VirtualizedControlTestUtils, added rudimentary tests for the methods. Jeanette Winzenburg has updated the pull request incrementally with one additional commit since the last revision: fixed c&p error in doc - Changes: - all: https://git.openjdk.java.net/jfx/pull/690/files - new: https://git.openjdk.java.net/jfx/pull/690/files/c2642bbe..1f2a9815 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=690&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=690&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/690.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/690/head:pull/690 PR: https://git.openjdk.java.net/jfx/pull/690
Re: RFR: 8278134: Move static utility methods to infrastructure (EditAndScrollTest) [v2]
On Thu, 16 Dec 2021 11:00:39 GMT, Ajit Ghaisas wrote: >> Jeanette Winzenburg has updated the pull request incrementally with one >> additional commit since the last revision: >> >> fixed c&p error in doc > > modules/javafx.controls/src/test/java/test/com/sun/javafx/scene/control/infrastructure/VirtualizedControlTestUtils.java > line 86: > >> 84: >> 85: /** >> 86: * Returns a vertical ScrollBar of the control. > > I found this typo. I know, you just moved it from one file to another.. but, > it would be good to fix in this PR :) > Typo : This should be - "Returns a horizontal..." actually, it was me who introduced it - in the previous PR 😁 Thanks for spotting! - PR: https://git.openjdk.java.net/jfx/pull/690
Re: RFR: 8268225: Support :focus-visible and :focus-within CSS pseudoclasses [v7]
On Tue, 7 Dec 2021 15:44:26 GMT, Michael Strauß wrote: >However, I'd still argue that a node that is not part of the scene graph >(non-atomically) should not be the focus owner of the scene graph. I agree :) But that's what seems to happen: in my example, add a handler to remove the focused "moving" button. When removed, the scene's focus owner is either the next focusable (the "move" button) or null if there is none. On re-adding the button, it's either unfocused (if there had been a next focusable) or focused (if there is none). Not sure what happens in your test snippet .. - PR: https://git.openjdk.java.net/jfx/pull/475
RFR: 8278134: Move static utility methods to infrastructure (EditAndScrollTest)
Extracted static test utility methods from EditAndScrollTest into new VirtualizedControlTestUtils, added rudimentary tests for the methods. - Commit messages: - 8278134: Move static utility methods to infrastructure Changes: https://git.openjdk.java.net/jfx/pull/690/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx&pr=690&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8278134 Stats: 348 lines in 3 files changed: 290 ins; 57 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/690.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/690/head:pull/690 PR: https://git.openjdk.java.net/jfx/pull/690
Re: RFR: 8268225: Support :focus-visible and :focus-within CSS pseudoclasses [v7]
On Fri, 20 Aug 2021 05:15:49 GMT, Michael Strauß wrote:
>> This PR adds the `Node.focusVisible` and `Node.focusWithin` properties, as
>> well as the corresponding `:focus-visible` and `:focus-within` CSS
>> pseudo-classes.
>
> Michael Strauß has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fixed undeterministic test failures
thanks for taking a closer look :)
> > might also need a test that verifies the focusWithin of a parent added
> > somewhere above the focused node? hmm .. or maybe not, that would require
> > to re-arrange a complete subtree ..
>
> Inserting a parent into a scene graph such that the existing subtree at that
> position becomes a subtree of the newly inserted parent can't be done as an
> atomic operation. First, you'll need to remove the subtree at the insertion
> point from the existing scene graph (otherwise you'll get an exception saying
> that a node can only appear once in a scene graph). Then you can add the new
> parent with the removed subtree as its child.
yeah that's true, but might happen behind the scenes - we can move a focused
node from one parent to another by simply adding it the new.
So I'm not sure I would agree:
> I'm inclined to think that this behavior is a bug.
arguable, though :) Below is an example of moving a focused node (button
"Moving", the other is just for having something else that might get the focus)
between 2 parents by F1: the button keeps its focused state in its new parent
(and scene's focusOwner doesn't change) - I think that's what a user would
expect also when dragging a focused node around (s/he might not even be aware
of the hierarchy). I suspect that changing the behavior would break existing
applications - except if it could be done completely transparently.
The example:
private Parent createContent() {
first = new VBox();
first.setBackground(new Background(new BackgroundFill(Color.ALICEBLUE,
null, null)));
first.getChildren().add(new Label("label on first"));
second = new VBox();
second.setBackground(new Background(new BackgroundFill(Color.LAVENDER,
null, null)));
second.getChildren().add(new Label("label on second"));
moving = new Button("moving");
first.getChildren().add(moving);
Button move = new Button("move");
move.setOnAction(e -> {
move();
});
move.setDefaultButton(true);
HBox content = new HBox(10, first, second, move);
content.addEventFilter(KeyEvent.KEY_PRESSED, e -> {
if (e.getCode() == KeyCode.F1) {
move();
}
});
return content;
}
private void move() {
Parent parent = moving.getParent();
Pane target = parent == first ? second : first;
target.getChildren().add(moving);
}
@Override
public void start(Stage stage) throws Exception {
Scene scene = new Scene(createContent(), 300, 300);
scene.focusOwnerProperty().addListener((scr, ov, nv) -> {
System.out.println("focusOwner: " + nv);
});
stage.setScene(scene);
stage.show();
}
private VBox first;
private VBox second;
private Button moving;
-
PR: https://git.openjdk.java.net/jfx/pull/475
Re: RFR: 8268225: Support :focus-visible and :focus-within CSS pseudoclasses [v7]
On Fri, 20 Aug 2021 05:15:49 GMT, Michael Strauß wrote: >> This PR adds the `Node.focusVisible` and `Node.focusWithin` properties, as >> well as the corresponding `:focus-visible` and `:focus-within` CSS >> pseudo-classes. > > Michael Strauß has updated the pull request incrementally with one additional > commit since the last revision: > > fixed undeterministic test failures Just some musings (slightly overwhelmed by the magnitude of the suggested change ;) On first look it sounds like this has 2 separated (though related) parts: - focus-visible: that's visual sugar (*ducking) - focus-within: that might be a game changer (or enhancer) for both internal and external usage >From my current focus - *haha - on cell editing, the latter feels very >promising for allowing fine-grained control of defining, implementing and >configuring the required, much debated and (until now) unsuccessful tries on >"commit-on-focusLost" semantics. - PR: https://git.openjdk.java.net/jfx/pull/475
Re: RFR: 8268225: Support :focus-visible and :focus-within CSS pseudoclasses [v2]
On Mon, 2 Aug 2021 13:20:29 GMT, Michael Strauß wrote: > > 3. I think the way you are propagating `focusWithin` might not work if > > nodes are added or removed from the scene graph. > > I've added a test for this case: > `FocusTest.testFocusStatesAreClearedFromFormerParentsOfFocusedNode`. might also need a test that verifies the focusWithin of a parent added somewhere above the focused node? hmm .. or maybe not, that would require to re-arrange a complete subtree .. - PR: https://git.openjdk.java.net/jfx/pull/475
Re: RFR: 8276167: VirtualFlow.scrollToTop doesn't scroll to the top of the last element [v2]
On Mon, 6 Dec 2021 09:23:53 GMT, eduardsdv wrote: > I tested it again. Without the changes in this PR the bug is still there. > **If the item is larger than the viewport**, the VirtualFlow.scrollToTop(int) > scrolls to the end instead of to top of the item. ahhh .. finally I understand what you mean - bolding of the quote ;) Might be obvious for everybody except me .. but the bug report is missing that crucial information ... - PR: https://git.openjdk.java.net/jfx/pull/656
Integrated: 8272118: ListViewSkin et al: must not cancel edit on scrolling
On Thu, 25 Nov 2021 15:46:01 GMT, Jeanette Winzenburg wrote: > Issue was that mouse pressed on the scrollbars of all virtualized controls > cancelled the edit. That's inconsistent with other scroll triggers > (mouseWheel, programmatic). Fixed by removing the cancel. > > Added tests that failed/passed before/after the fix. Also added tests that > passed both before/after to guarantee that required functionality of the > mouse pressed (= requesting focus on the control if needed) is still working. This pull request has now been integrated. Changeset: aa045c5e Author:Jeanette Winzenburg URL: https://git.openjdk.java.net/jfx/commit/aa045c5e60f9566748b55d745d162d0c94ba5e59 Stats: 594 lines in 6 files changed: 566 ins; 28 del; 0 mod 8272118: ListViewSkin et al: must not cancel edit on scrolling Reviewed-by: aghaisas, kcr - PR: https://git.openjdk.java.net/jfx/pull/682
Re: RFR: 8272118: ListViewSkin et al: must not cancel edit on scrolling
On Thu, 2 Dec 2021 00:18:37 GMT, Kevin Rushforth wrote: >> Issue was that mouse pressed on the scrollbars of all virtualized controls >> cancelled the edit. That's inconsistent with other scroll triggers >> (mouseWheel, programmatic). Fixed by removing the cancel. >> >> Added tests that failed/passed before/after the fix. Also added tests that >> passed both before/after to guarantee that required functionality of the >> mouse pressed (= requesting focus on the control if needed) is still working. > > modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/EditAndScrollTest.java > line 408: > >> 406: >> 407: >> 408: //- Utility methods (TODO: move into infrastructure) > > Can you file a follow-up issue for this? yeah, sure - was undecided whether to do it here or later (locally, there are more that will be added for fixing related issues) - [JDK-8278134](https://bugs.openjdk.java.net/browse/JDK-8278134) - PR: https://git.openjdk.java.net/jfx/pull/682
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). fix looks good - verified that the new api and its implementation is the exact same as in ComboBox. Tests look good as well - verified the focused-related test fails/passes before/after the fix, all other tests pass after the fix. - Marked as reviewed by fastegal (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/679
RFR: 8187307: ListView, TableView, TreeView: receives editCancel event when edit is committed
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. - Commit messages: - 8187307: LisingtView, TableView, TreeView: receives editCancel event Changes: https://git.openjdk.java.net/jfx/pull/684/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx&pr=684&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8187307 Stats: 158 lines in 8 files changed: 117 ins; 29 del; 12 mod Patch: https://git.openjdk.java.net/jfx/pull/684.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/684/head:pull/684 PR: https://git.openjdk.java.net/jfx/pull/684
Re: RFR: 8276553: ListView scrollTo() is broken after fix for JDK-8089589
On Mon, 29 Nov 2021 14:38:10 GMT, Michael Strauß wrote: >> After (re)setting the number of elements, make sure to do at least some >> estimation of the total size. >> Added a testcase for this scenario. > > modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java > line 903: > >> 901: recalculateAndImproveEstimatedSize(2); >> 902: >> 903: cellCount.set(value); > > Can this be implemented in a way that doesn't violate the property contract? > Since this property is public API, `setCellCount(int)` should just call > `cellCountProperty().set(int)`. Maybe you can extract the composite operation > here into a private method and use that instead of `setCellCount(int)`. > > `position` has the same problem. good catch! the "additional" stuff typically is done in the properties' invalidated .. - PR: https://git.openjdk.java.net/jfx/pull/683
Re: RFR: 8276206: Rename TextBinding class to better express its purpose
On Wed, 24 Nov 2021 04:23:59 GMT, Michael Strauß wrote: > This PR renames `TextBinding` to `MnemonicInfo` and adds the following text > to its javadoc: > > /** > 33 +* Provides information about mnemonics contained within a string. looks good (modulo our disagreement on whether or not to do it at all :) - Marked as reviewed by fastegal (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/678
RFR: 8272118: ListViewSkin et al: must not cancel edit on scrolling
Issue was that mouse pressed on the scrollbars of all virtualized controls cancelled the edit. That's inconsistent with other scroll triggers (mouseWheel, programmatic). Fixed by removing the cancel. Added tests that failed/passed before/after the fix. Also added tests that passed both before/after to guarantee that required functionality of the mouse pressed (= requesting focus on the control if needed) is still working. - Commit messages: - 8272118: ListViewSkin et al: must not cancel edit on scrolling Changes: https://git.openjdk.java.net/jfx/pull/682/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx&pr=682&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8272118 Stats: 594 lines in 6 files changed: 566 ins; 28 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/682.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/682/head:pull/682 PR: https://git.openjdk.java.net/jfx/pull/682
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). will review thoroughly soon - for now just a comment to get rid of that annoying yellow bar at the top ;) On first skim, it looks straightforward - adding the api same as for combo/spinner. I think you might get going and flesh out the csr. - PR: https://git.openjdk.java.net/jfx/pull/679
Integrated: 8274061: Tree-/TableRowSkin: misbehavior on switching skin
On Fri, 24 Sep 2021 16:01:38 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))~~ not > handled here, see > [JDK-8277000](https://bugs.openjdk.java.net/browse/JDK-8277000) > > 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) This pull request has now been integrated. Changeset: d14be6a8 Author:Jeanette Winzenburg URL: https://git.openjdk.java.net/jfx/commit/d14be6a811591df70ef99fd6ec5448423be6fb7d Stats: 1086 lines in 8 files changed: 1025 ins; 44 del; 17 mod 8274061: Tree-/TableRowSkin: misbehavior on switching skin Reviewed-by: aghaisas, kcr - PR: https://git.openjdk.java.net/jfx/pull/632
Re: RFR: 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 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. - PR: https://git.openjdk.java.net/jfx/pull/669
Re: RFR: 8274061: Tree-/TableRowSkin: misbehavior on switching skin [v5]
> 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))~~ not > handled here, see > [JDK-8277000](https://bugs.openjdk.java.net/browse/JDK-8277000) > > 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: changes as requested in review - Changes: - all: https://git.openjdk.java.net/jfx/pull/632/files - new: https://git.openjdk.java.net/jfx/pull/632/files/603b08af..801182ff Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=632&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=632&range=03-04 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jfx/pull/632.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/632/head:pull/632 PR: https://git.openjdk.java.net/jfx/pull/632
Re: RFR: 8274061: Tree-/TableRowSkin: misbehavior on switching skin [v4]
On Fri, 19 Nov 2021 09:24:42 GMT, Ajit Ghaisas wrote: >> Jeanette Winzenburg has updated the pull request incrementally with one >> additional commit since the last revision: >> >> reverted fixedCellSize handling > > modules/javafx.controls/src/shims/java/javafx/scene/control/skin/TableSkinShim.java > line 2: > >> 1: /* >> 2: * Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved. > > 2021 :) lost a year to the pandemic ;) - PR: https://git.openjdk.java.net/jfx/pull/632
Re: RFR: 8274061: Tree-/TableRowSkin: misbehavior on switching skin [v2]
On Fri, 19 Nov 2021 09:45:06 GMT, Ajit Ghaisas wrote: >> FYI: now the listener registration - including the incorrect code comment >> (which is the same as in current master) - is back at the old location in >> the re-inserted setupTreeTableViewListeners. So still need input whether >> it's okay to correct the code comment here. > > I think, it is okay to fix the comment in this review itself. It is very > minor to warrant a separate PR. Also, this anomaly was discovered in this > review makes it a candidate to be fixed here. done - thanks to both of you :) - PR: https://git.openjdk.java.net/jfx/pull/632
Re: RFR: 8274061: Tree-/TableRowSkin: misbehavior on switching skin [v2]
On Mon, 1 Nov 2021 12:59:42 GMT, Marius Hanl wrote: >> 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? :) FYI: now the listener registration - including the incorrect code comment (which is the same as in current master) - is back at the old location in the re-inserted setupTreeTableViewListeners. So still need input whether it's okay to correct the code comment here. - PR: https://git.openjdk.java.net/jfx/pull/632
Re: RFR: 8274061: Tree-/TableRowSkin: misbehavior on switching skin [v4]
> 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: reverted fixedCellSize handling - Changes: - all: https://git.openjdk.java.net/jfx/pull/632/files - new: https://git.openjdk.java.net/jfx/pull/632/files/fc71c931..603b08af Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=632&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=632&range=02-03 Stats: 312 lines in 6 files changed: 243 ins; 50 del; 19 mod Patch: https://git.openjdk.java.net/jfx/pull/632.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/632/head:pull/632 PR: https://git.openjdk.java.net/jfx/pull/632
Re: RFR: 8268225: Support :focus-visible and :focus-within CSS pseudoclasses [v7]
On Wed, 10 Nov 2021 17:45:32 GMT, Michael Strauß wrote: > > > @kleopatra Can you be a reviewer on this PR? css is not my turf, sry - PR: https://git.openjdk.java.net/jfx/pull/475
Re: RFR: Draft - 8274061: Tree-/TableRowSkin: misbehavior on switching skin [v3]
On Mon, 1 Nov 2021 12:53:43 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: > > removed unused code, fixed doc of test methods Changed to draft because just noted an issue with initial horizontal scrollBar for many columns and fixedCellSize which I don't quite understand yet. - PR: https://git.openjdk.java.net/jfx/pull/632
Re: RFR: 8274061: Tree-/TableRowSkin: misbehavior on switching skin [v2]
On Sun, 31 Oct 2021 17:05:52 GMT, Michael Strauß wrote:
>> 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 299:
>
>> 297: @Override protected ObjectProperty graphicProperty() {
>> 298: TreeTableRow treeTableRow = getSkinnable();
>> 299: // FIXME: illegal access if skinnable is null
>
> What is the purpose of removing the null check, but leaving getSkinnable() in
> there?
> Given the signature of the method, it seems that it shouldn't ever return
> `null` in any case.
none except me having been sloppy ;) - removed both the access and the
left-over code comment from my dev version). And yeah, true: guarding against
null skinnable is always an indication of something being wrong (skinnable can
be null only after dispose - after that, its methods must not be used)
For fun, here's the evolution of `graphicProperty()` (which was `getGraphic()`
early on):
Initially the treeItem is accessed directly from the skinnable: the null guard
already is wrong (and might hide problems elsewhere), but doesn't look so
@Override protected Node getGraphic() {
TreeTableRow treeTableRow = getSkinnable();
if (treeTableRow == null) return null;
TreeItem treeItem = treeTableRow.getTreeItem();
if (treeItem == null) return null;
return treeItem.getGraphic();
}
At some time ([JDK-8124119](https://bugs.openjdk.java.net/browse/JDK-8124119)
was RT-28098) the direct access was replaced by keeping an alias to the
treeItem. Now the skinnable is not needed, and the null guard still is wrong :)
@Override protected Node getGraphic() {
TreeTableRow treeTableRow = getSkinnable();
if (treeTableRow == null) return null;
if (treeItem == null) return null;
return treeItem.getGraphic();
}
-
PR: https://git.openjdk.java.net/jfx/pull/632
Re: RFR: 8274061: Tree-/TableRowSkin: misbehavior on switching skin [v2]
On Sun, 31 Oct 2021 18:07:27 GMT, Marius Hanl wrote: >> 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()`. 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? - PR: https://git.openjdk.java.net/jfx/pull/632
Re: RFR: 8274061: Tree-/TableRowSkin: misbehavior on switching skin [v3]
> 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: removed unused code, fixed doc of test methods - Changes: - all: https://git.openjdk.java.net/jfx/pull/632/files - new: https://git.openjdk.java.net/jfx/pull/632/files/5402e1fb..fc71c931 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=632&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=632&range=01-02 Stats: 6 lines in 2 files changed: 1 ins; 3 del; 2 mod Patch: https://git.openjdk.java.net/jfx/pull/632.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/632/head:pull/632 PR: https://git.openjdk.java.net/jfx/pull/632
Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v3]
On Fri, 29 Oct 2021 13:58:35 GMT, Michael Strauß wrote: >> This PR fixes an issue with mnemonic parsing by removing the restriction >> that a mnemonic symbol must be a letter. Now, it can be any character except >> whitespace. > > Michael Strauß has updated the pull request incrementally with one additional > commit since the last revision: > > changed javadoc there is no need for the class rename, is there? Even though it's formally internal api, I think we shouldn't do code breaking changes except if there's a very compelling reason - there are too many apps out in the wild that use internal api. - PR: https://git.openjdk.java.net/jfx/pull/647
Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v3]
On Fri, 29 Oct 2021 23:14:07 GMT, Michael Strauß wrote:
>> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/LabelSkinTest.java
>> line 2162:
>>
>>> 2160: label.autosize();
>>> 2161: skin.updateDisplayedText();
>>> 2162: assertEquals("foo _bar _qux",
>>> LabelSkinBaseShim.getText(label).getText());
>>
>> Can you verify here which letter / index is picked as mnemonic? (Also in the
>> other tests).
>
> I don't see an easy way to do that, and I'm not in favor of making private
> implementation details package-public just to test some internal state. Of
> course, mnemonic support should have been designed in a way that is more
> easily testable, but this PR is not the place to do that.
could be tested indirectly, though not in isolation:
- access the actual mnenomic via accessibleAttribute
- test whether labelFor/action is working as expected when firing an
alt-mnemonic onto the scene
-
PR: https://git.openjdk.java.net/jfx/pull/647
Integrated: 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. This pull request has now been integrated. Changeset: adcc40d5 Author: Jeanette Winzenburg URL: https://git.openjdk.java.net/jfx/commit/adcc40d57c0f2619cad0ca5ffc2ed9e9a5e032b0 Stats: 77 lines in 7 files changed: 46 ins; 22 del; 9 mod 8187474: Tree-/TableCell, TreeCell: editingCell/Item not updated in cell.startEdit Reviewed-by: mhanl, aghaisas - PR: https://git.openjdk.java.net/jfx/pull/638
Re: RFR: 8274061: Tree-/TableRowSkin: misbehavior on switching skin [v2]
On Tue, 26 Oct 2021 12:07:23 GMT, Ajit Ghaisas wrote: >> 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/TableRowSkin.java > line 134: > >> 132: // that when it changes, we can appropriately add / >> remove cells that may or may not >> 133: // be required (because we remove all cells that are >> not visible). >> 134: >> registerChangeListener(getVirtualFlow().widthProperty(), e -> >> tableView.requestLayout()); > > If this listener is removed, then is there a chance of reintroducing > [JDK-8144500](https://bugs.openjdk.java.net/browse/JDK-8144500)? > Unfortunately, there is no test added for that bug.. so it is difficult to > catch regression, if any. hmm .. the listener is not removed, its registration is moved to updateTableViewSkin. There are tests covering that it really is still registered :) Forgot to move the old code comment, though. Re-added. > modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableRowSkin.java > line 154: > >> 152: // that when it changes, we can appropriately add / >> remove cells that may or may not >> 153: // be required (because we remove all cells that are >> not visible). >> 154: >> registerChangeListener(getVirtualFlow().widthProperty(), e -> >> treeTableView.requestLayout()); > > Same comment as in TableRowSkin regarding this listener. same comment as to TableRowSkin :) - PR: https://git.openjdk.java.net/jfx/pull/632
Re: RFR: 8274061: Tree-/TableRowSkin: misbehavior on switching skin [v2]
> 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 - Changes: - all: https://git.openjdk.java.net/jfx/pull/632/files - new: https://git.openjdk.java.net/jfx/pull/632/files/52e18d22..5402e1fb Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=632&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=632&range=00-01 Stats: 8 lines in 2 files changed: 8 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/632.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/632/head:pull/632 PR: https://git.openjdk.java.net/jfx/pull/632
Re: RFR: 8187474: Tree-/TableCell, TreeCell: editingCell/Item not updated in cell.startEdit [v3]
> 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 - Changes: - all: https://git.openjdk.java.net/jfx/pull/638/files - new: https://git.openjdk.java.net/jfx/pull/638/files/4f76e1fe..4fd55ffa Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=638&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=638&range=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/638.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/638/head:pull/638 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 09:45:10 GMT, Jeanette Winzenburg wrote: >> 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 :) > > 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 .. 😁? - PR: https://git.openjdk.java.net/jfx/pull/638
Re: RFR: 8187474: Tree-/TableCell, TreeCell: editingCell/Item not updated in cell.startEdit [v2]
On Thu, 21 Oct 2021 18:50:45 GMT, Marius Hanl wrote: >> 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 :) darn .. ;) Thanks - PR: https://git.openjdk.java.net/jfx/pull/638
Re: RFR: 8187474: Tree-/TableCell, TreeCell: editingCell/Item not updated in cell.startEdit [v2]
On Wed, 13 Oct 2021 21:25:43 GMT, Marius Hanl wrote: >> 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 549: > >> 547: int editingRow = 1; >> 548: cell.updateIndex(editingRow); >> 549: TablePosition editingCell = new TablePosition<>(table, >> editingRow, editingColumn); > > Minor: There is a space missing in `` thanks :) - PR: https://git.openjdk.java.net/jfx/pull/638
Re: RFR: 8187474: Tree-/TableCell, TreeCell: editingCell/Item not updated in cell.startEdit [v2]
> 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 - Changes: - all: https://git.openjdk.java.net/jfx/pull/638/files - new: https://git.openjdk.java.net/jfx/pull/638/files/050a6caa..4f76e1fe Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=638&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=638&range=00-01 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/638.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/638/head:pull/638 PR: https://git.openjdk.java.net/jfx/pull/638
Re: RFR: 8187474: Tree-/TableCell, TreeCell: editingCell/Item not updated in cell.startEdit
On Thu, 7 Oct 2021 10:35:12 GMT, Marius Hanl wrote: > > > And by the way, you know why there is a requestFocus() in List/TreeCell, but > not in the other two cells? :) plain oversight? or the wrong thing to do, anyway? or having weird side-effects in tabular controls which were evaded by not requesting focus? We'll see the deeper we go in the cleanup quest :) - 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 ;) an aside I forgot to mention yesterday: this interference from behavior (triggered by mouseEvents) is one of the major stumbling-blocks in solving the ominous commit-on-focusLost issue - by _cancelling_ an edit. - 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:15:16 GMT, Marius Hanl 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. - PR: https://git.openjdk.java.net/jfx/pull/638
RFR: 8187474: Tree-/TableCell, TreeCell: editingCell/Item not updated in cell.startEdit
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. - Commit messages: - 8187474: Tree-/TableCell, TreeCell: editingCell/Item not updated in Changes: https://git.openjdk.java.net/jfx/pull/638/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx&pr=638&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8187474 Stats: 76 lines in 7 files changed: 46 ins; 21 del; 9 mod Patch: https://git.openjdk.java.net/jfx/pull/638.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/638/head:pull/638 PR: https://git.openjdk.java.net/jfx/pull/638
Integrated: 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. This pull request has now been integrated. Changeset: 2c86e0fc Author:Jeanette Winzenburg URL: https://git.openjdk.java.net/jfx/commit/2c86e0fc81074f6ad49b798c891a358a5fe15f94 Stats: 105 lines in 8 files changed: 105 ins; 0 del; 0 mod 8274433: All Cells: misbehavior of startEdit Reviewed-by: mhanl, aghaisas - PR: https://git.openjdk.java.net/jfx/pull/636
Re: RFR: 8274433: All Cells: misbehavior of startEdit [v2]
> 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 - Changes: - all: https://git.openjdk.java.net/jfx/pull/636/files - new: https://git.openjdk.java.net/jfx/pull/636/files/43015266..e51cb865 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=636&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=636&range=00-01 Stats: 9 lines in 4 files changed: 0 ins; 2 del; 7 mod Patch: https://git.openjdk.java.net/jfx/pull/636.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/636/head:pull/636 PR: https://git.openjdk.java.net/jfx/pull/636
Re: RFR: 8274433: All Cells: misbehavior of startEdit
On Fri, 1 Oct 2021 13:11:21 GMT, Marius Hanl 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.
>
> 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 :)
haha .. left-over from c&p (of ListCellTest ;)
-
PR: https://git.openjdk.java.net/jfx/pull/636
Re: RFR: 8274433: All Cells: misbehavior of startEdit
On Fri, 1 Oct 2021 13:10:29 GMT, Marius Hanl 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. > > 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? actually, no: forgot to removed the unused list ;) Good catch! - PR: https://git.openjdk.java.net/jfx/pull/636
RFR: 8274433: All Cells: misbehavior of startEdit
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. - Commit messages: - 8274433: All Cells: misbehavior of startEdit Changes: https://git.openjdk.java.net/jfx/pull/636/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx&pr=636&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8274433 Stats: 107 lines in 8 files changed: 107 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/636.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/636/head:pull/636 PR: https://git.openjdk.java.net/jfx/pull/636
Re: RFR: 8188026: TextFieldXXCell: NPE on calling startEdit [v4]
On Sun, 26 Sep 2021 12:05:11 GMT, Jeanette Winzenburg wrote: > the other way round: a cell that didn't switch into editing will nevertheless > fire a editStart on its target FYI: filed [JDK-8274433](https://bugs.openjdk.java.net/browse/JDK-8274433) - PR: https://git.openjdk.java.net/jfx/pull/569
Re: RFR: 8188026: TextFieldXXCell: NPE on calling startEdit [v4]
On Sun, 19 Sep 2021 11:24:43 GMT, Marius Hanl wrote:
>> modules/javafx.controls/src/main/java/javafx/scene/control/cell/ChoiceBoxTreeCell.java
>> line 301:
>>
>>> 299: return;
>>> 300: }
>>> 301:
>>
>> (darn, can't add the important lines - which is backing out if treeItem is
>> null)
>>
>> The re-ordering leads to change of behavior, here's a test that's
>> passing/failing before/after:
>>
>> /**
>> * change of behavior: cell must not be editing if treeItem == null.
>> * fails with fix, passes without
>> */
>> @Test
>> public void testChoiceBoxTreeCellEditing() {
>> TreeView treeView = new TreeView<>();
>> treeView.setEditable(true);
>> ChoiceBoxTreeCell cell = new ChoiceBoxTreeCell<>();
>> cell.updateTreeView(treeView);
>> cell.updateItem("TEST", false);
>>
>> cell.startEdit();
>> assertFalse(cell.isEditing());
>> assertNull(cell.getGraphic());
>> }
>>
>> same for ComboBoxTreeCell
>
> Hm.. weird that the super class is firing an edit event even with `treeItem =
> null`. Maybe this should be investigated in a follow-up. Good catch though. :)
yeah, there are whacky aspects:
- we can switch a cell that's not attached to its surroundings (like table,
column, treeItem, off-range) into editing state - the only constraint being
that it's not empty
- the other way round: a cell that didn't switch into editing will nevertheless
fire a editStart on its target
Definitely leeway for improvements ;)
-
PR: https://git.openjdk.java.net/jfx/pull/569
Re: RFR: 8188026: TextFieldXXCell: NPE on calling startEdit [v5]
On Sun, 19 Sep 2021 11:52:55 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
>
> Marius Hanl has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Addressed review comments
>
> - cell is supplied in setup() method
> - treeItem = null guard is done before calling startEdit()
> - Added checks for the cell graphic
fix looks okay now :)
Still a bit weary about the tests: personally, I don't like re-using and
re-configuring the same instance of an object inside a longish test method
(here: in the tight loop). But then, there are other tests in the code base
doing it so probably okay here as well.
-
Marked as reviewed by fastegal (Reviewer).
PR: https://git.openjdk.java.net/jfx/pull/569
Integrated: 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 ;) This pull request has now been integrated. Changeset: 55faac49 Author:Jeanette Winzenburg URL: https://git.openjdk.java.net/jfx/commit/55faac49082ee8c92893dac15d2777011ec7dca0 Stats: 404 lines in 12 files changed: 358 ins; 22 del; 24 mod 8271474: Tree-/TableCell: inconsistent edit event firing pattern Reviewed-by: mhanl, aghaisas - PR: https://git.openjdk.java.net/jfx/pull/620
RFR: 8274061: Tree-/TableRowSkin: misbehavior on switching skin
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) - Commit messages: - 8274061: Tree-/TableRowSkin: misbehavior on switching skin Changes: https://git.openjdk.java.net/jfx/pull/632/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx&pr=632&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8274061 Stats: 1005 lines in 8 files changed: 882 ins; 100 del; 23 mod Patch: https://git.openjdk.java.net/jfx/pull/632.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/632/head:pull/632 PR: https://git.openjdk.java.net/jfx/pull/632
Re: CFV: New OpenJFX Committer: Thiago Sayao
Vote: YES Zitat von Kevin Rushforth : I hereby nominate Thiago Sayao [1] to OpenJFX Committer. Thiago is an OpenJFX community member, who has contributed 15 commits [2] to OpenJFX. Votes are due by September 27, 2021 at 15:00 UTC. Only current OpenJFX Committers [3] are eligible to vote on this nomination. Votes must be cast in the open by replying to this mailing list. For Lazy Consensus voting instructions, see [4]. Nomination to a project Committer is described in [6]. Thanks. -- Kevin [1] https://openjdk.java.net/census#tsayao [2] https://github.com/openjdk/jfx/search?q=author-name%3A%22Thiago+Sayao%22&s=author-date&type=commits [3] https://openjdk.java.net/census#openjfx [4] https://openjdk.java.net/bylaws#lazy-consensus [6] https://openjdk.java.net/projects#project-committer
Re: RFR: 8188026: TextFieldXXCell: NPE on calling startEdit
On Thu, 22 Jul 2021 19:15:54 GMT, Marius Hanl wrote: >>> > just checked my notes (there's a cell-editing branch in my fork where I'm >>> > experimenting) - astonishingly the answer is no, could not see anything >>> > :) And actually, seems like we don't even need to return immediately: >>> > would have expected some unhealthy side-effects on doing the switching >>> > into visual editing state more than once, but couldn't detect anything. >>> > You might try, though :) >>> >>> Okay. Question is, should we guard against a double edit? There is already >>> one in `TreeTableCell#startEdit`, but probably forgotten in TableCell. I >>> think it makes sense and as there is already the check in TreeTableCell, >>> there was at least a thought of it somewhere in the past. >> >> good question, next question ;) >> >> - the oversight in startEdit of the base List/TableCell is not part of this >> (covered and soon fixed by >> [JDK-8188027](https://bugs.openjdk.java.net/browse/JDK-8188027), the >> concrete misbehavior is that they fire multiple edit events >> - as to the "real" editing cell types (that is those that actually have an >> editingComponent) - we (that is now you *grin) should try hard to find a >> scenario where multiple starts (== multiple configuration passes of the >> editingComponent) might hurt. Like when the user already typed something and >> for some reason startEdit is called again, the configuration would delete >> the input. >> >> > If there is nothing left, should I create a ticket for `startEdit` and >> for `cancelEdit` (this only affects the sub classes) ? :) >> >> hmm - not sure I understand what you are asking: startEdit is covered, and >> cancelEdit would be similar - either you find a scenario where multiple >> un-configure of the cell (after cancel) would hurt or not? > >> > > just checked my notes (there's a cell-editing branch in my fork where >> > > I'm experimenting) - astonishingly the answer is no, could not see >> > > anything :) And actually, seems like we don't even need to return >> > > immediately: would have expected some unhealthy side-effects on doing >> > > the switching into visual editing state more than once, but couldn't >> > > detect anything. You might try, though :) >> > >> > >> > Okay. Question is, should we guard against a double edit? There is already >> > one in `TreeTableCell#startEdit`, but probably forgotten in TableCell. I >> > think it makes sense and as there is already the check in TreeTableCell, >> > there was at least a thought of it somewhere in the past. >> >> good question, next question ;) >> >> * the oversight in startEdit of the base List/TableCell is not part of this >> (covered and soon fixed by >> [JDK-8188027](https://bugs.openjdk.java.net/browse/JDK-8188027), the >> concrete misbehavior is that they fire multiple edit events >> * as to the "real" editing cell types (that is those that actually have an >> editingComponent) - we (that is now you *grin) should try hard to find a >> scenario where multiple starts (== multiple configuration passes of the >> editingComponent) might hurt. Like when the user already typed something and >> for some reason startEdit is called again, the configuration would delete >> the input. >> >> > If there is nothing left, should I create a ticket for `startEdit` and for >> > `cancelEdit` (this only affects the sub classes) ? :) >> >> hmm - not sure I understand what you are asking: startEdit is covered, and >> cancelEdit would be similar - either you find a scenario where multiple >> un-configure of the cell (after cancel) would hurt or not? > > Finally coming back to this, when firing a **startEdit()** while already > editing e.g. a TextFieldCell, the input which was made by you gets lost and > you need to start over. So this can be a potential follow-up. I didn't found > anything for **cancelEdit()**. @Maran23 wondering why my review comments aren't addressed? Anything unclear? - PR: https://git.openjdk.java.net/jfx/pull/569
Re: RFR: 8268225: Support :focus-visible and :focus-within CSS pseudoclasses [v7]
On Thu, 9 Sep 2021 09:47:56 GMT, Michael Strauß wrote: > > > > Just curious: with this in place, would it be possible to use for > > supporting [JDK-8087926](https://bugs.openjdk.java.net/browse/JDK-8087926) > > (it's a bit vague, though, at least for me)? > > Yes, `:focus-within` can be used to select an ancestor of the > currently-focused node. cool - thanks :) - PR: https://git.openjdk.java.net/jfx/pull/475
Re: RFR: 8268225: Support :focus-visible and :focus-within CSS pseudoclasses [v7]
On Fri, 20 Aug 2021 05:15:49 GMT, Michael Strauß wrote: >> This PR adds the `Node.focusVisible` and `Node.focusWithin` properties, as >> well as the corresponding `:focus-visible` and `:focus-within` CSS >> pseudo-classes. > > Michael Strauß has updated the pull request incrementally with one additional > commit since the last revision: > > fixed undeterministic test failures Just curious: with this in place, would it be possible to use for supporting [JDK-8087926](https://bugs.openjdk.java.net/browse/JDK-8087926) (it's a bit vague, though, at least for me)? - PR: https://git.openjdk.java.net/jfx/pull/475
RFR: 8271474: Tree-/TableCell: inconsistent edit event firing pattern
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 ;) - Commit messages: - 8271474: Tree-/TableCell: inconsistent edit event firing pattern Changes: https://git.openjdk.java.net/jfx/pull/620/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx&pr=620&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8271474 Stats: 404 lines in 12 files changed: 358 ins; 22 del; 24 mod Patch: https://git.openjdk.java.net/jfx/pull/620.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/620/head:pull/620 PR: https://git.openjdk.java.net/jfx/pull/620
Integrated: 8269871: CellEditEvent: must not throw NPE in accessors
On Thu, 26 Aug 2021 14:09:58 GMT, Jeanette Winzenburg wrote: > The issue is unguarded access to tablePosition though it might be null (since > [JDK-8120610](https://bugs.openjdk.java.net/browse/JDK-8120610) > > The fix is to check against null in each accessor. Also required to fix the > default onEditCommit handlers in Tree/TableColumn to really cope with null > TablePostion on the event. > > Added tests that failed/pass before/after the fix. > > Note that there was an old test (in Tree/TableColumnTest each), that failed > after the fix because it expected the NPE. Fixed by removing the expected > parameter. This pull request has now been integrated. Changeset: 78ae4a81 Author:Jeanette Winzenburg URL: https://git.openjdk.java.net/jfx/commit/78ae4a815b728f5a0dca8fa6de8ca68a27a1d189 Stats: 411 lines in 6 files changed: 391 ins; 5 del; 15 mod 8269871: CellEditEvent: must not throw NPE in accessors Reviewed-by: aghaisas - PR: https://git.openjdk.java.net/jfx/pull/611
Re: RFR: 8269871: CellEditEvent: must not throw NPE in accessors [v2]
> The issue is unguarded access to tablePosition though it might be null (since > [JDK-8120610](https://bugs.openjdk.java.net/browse/JDK-8120610) > > The fix is to check against null in each accessor. Also required to fix the > default onEditCommit handlers in Tree/TableColumn to really cope with null > TablePostion on the event. > > Added tests that failed/pass before/after the fix. > > Note that there was an old test (in Tree/TableColumnTest each), that failed > after the fix because it expected the NPE. Fixed by removing the expected > parameter. Jeanette Winzenburg has updated the pull request incrementally with one additional commit since the last revision: removed forgotten code comments in tests as per review - Changes: - all: https://git.openjdk.java.net/jfx/pull/611/files - new: https://git.openjdk.java.net/jfx/pull/611/files/b6540a2d..7082541d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=611&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=611&range=00-01 Stats: 2 lines in 2 files changed: 0 ins; 2 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/611.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/611/head:pull/611 PR: https://git.openjdk.java.net/jfx/pull/611
Re: RFR: 8269871: CellEditEvent: must not throw NPE in accessors
On Fri, 3 Sep 2021 15:00:32 GMT, Ajit Ghaisas wrote:
>> The issue is unguarded access to tablePosition though it might be null
>> (since [JDK-8120610](https://bugs.openjdk.java.net/browse/JDK-8120610)
>>
>> The fix is to check against null in each accessor. Also required to fix the
>> default onEditCommit handlers in Tree/TableColumn to really cope with null
>> TablePostion on the event.
>>
>> Added tests that failed/pass before/after the fix.
>>
>> Note that there was an old test (in Tree/TableColumnTest each), that failed
>> after the fix because it expected the NPE. Fixed by removing the expected
>> parameter.
>
> modules/javafx.controls/src/test/java/test/javafx/scene/control/CellEditEventOfTreeTableColumnTest.java
> line 186:
>
>> 184: root.setExpanded(true);
>> 185: ObservableList model =
>> FXCollections.observableArrayList("Four", "Five", "Fear");
>> 186: // "Flop", "Food", "Fizz"
>
> Did you forget to remove this comment?
yeah, pitfalls of c&p test boiler-plate code ;)
-
PR: https://git.openjdk.java.net/jfx/pull/611
Integrated: 8273071: SeparatorSkin: must remove child on dispose
On Thu, 2 Sep 2021 12:45:02 GMT, Jeanette Winzenburg wrote: > minor skin cleanup issue: SeparatorSkin didn't remove the line it added to > the control's children > > fix is to override dispose and include the removal > for testing: removed the exclusion of SeparatorSkin from memoryLeakTest - > doing so lets it fail/pass before/after this fix This pull request has now been integrated. Changeset: 2267b115 Author:Jeanette Winzenburg URL: https://git.openjdk.java.net/jfx/commit/2267b115f907e7c9c447691569e48cf0b2b95029 Stats: 14 lines in 2 files changed: 7 ins; 7 del; 0 mod 8273071: SeparatorSkin: must remove child on dispose Reviewed-by: arapte - PR: https://git.openjdk.java.net/jfx/pull/616
RFR: 8273071: SeparatorSkin: must remove child on dispose
minor skin cleanup issue: SeparatorSkin didn't remove the line it added to the control's children fix is to override dispose and include the removal for testing: removed the exclusion of SeparatorSkin from memoryLeakTest - doing so lets it fail/pass before/after this fix - Commit messages: - 8273071: SeparatorSkin: must remove child on dispose Changes: https://git.openjdk.java.net/jfx/pull/616/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx&pr=616&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8273071 Stats: 14 lines in 2 files changed: 7 ins; 7 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/616.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/616/head:pull/616 PR: https://git.openjdk.java.net/jfx/pull/616
Integrated: 8269081: Tree/ListViewSkin: must remove flow on dispose
On Fri, 27 Aug 2021 11:15:58 GMT, Jeanette Winzenburg wrote: > left-over issue from cleanup of Tree/ListViewSkin: direct children that have > been added by the skin must be removed in dispose > > fixed by removing the flow (which allowed to revert the previous cleanup of > event handlers/cellfactory) > > added tests to verify > - constant child count after replacing skin > - no memory leak when switching skin while showing This pull request has now been integrated. Changeset: e9315014 Author:Jeanette Winzenburg URL: https://git.openjdk.java.net/jfx/commit/e9315014f4accd97d757689d5ff212dd536a6e61 Stats: 90 lines in 3 files changed: 68 ins; 18 del; 4 mod 8269081: Tree/ListViewSkin: must remove flow on dispose Reviewed-by: arapte - PR: https://git.openjdk.java.net/jfx/pull/612
Re: "Execution optimizations have been disabled for task" during building
Hi Nir, looks similar to https://mail.openjdk.java.net/pipermail/openjfx-dev/2021-May/030563.html which I'm seeing since May - don't know (and didn't dig ;) why or how to solve, I simply ignore them. -- Jeanette Zitat von Nir Lisker : Hi, When running builds, gradle reports Execution optimizations have been disabled for task ':base:copyClassFilesWin' to ensure correctness due to the following reasons: - Gradle detected a problem with the following location: 'C:\...\jfx\modules\javafx.base\build\module-classes'. Reason: Task ':base:copyClassFilesWin' uses this output of task ':base:buildModuleWin' without declaring an explicit or implicit dependency. This can lead to incorrect results being produced, depending on what order the tasks are executed. Please refer to https://docs.gradle.org/7.0.1/userguide/validation_problems.html#implicit_dependency for more details about this problem. Same happens on other tasks: :base:copyLibFilesWin :base:modularJarStandaloneWin :base:copyLibFilesStandaloneWin etc. I don't remember seeing this before. It doesn't cause any failures but thought I'd ask.
Re: Enhancements for JavaFX 18
Zitat von Kevin Rushforth : Yes, it would be great to finally solve this one. It would need someone who is willing and able to to drive it, though. -- Kevin Well, I think myself being both - with a little help of my friends ;) -- Jeanette On 8/4/2021 2:58 AM, Jeanette Winzenburg wrote: my suggestion would be the longstanding commit-edit-on-focus-lost - solved by an enhancement to the editing api on virtualized controls -- Jeanette Zitat von Kevin Rushforth : Now that JavaFX 17 is in RDP2, we can turn more attention to bug fixes and enhancement requests for JavaFX 18. It's the summer, so there may be delays as some people are out at various times (including me), but I would like to get some of the outstanding enhancement requests moving over the next few weeks. Specifically, I'd like to see the following proceed: * Transparent backgrounds in WebView JBS: https://bugs.openjdk.java.net/browse/JDK-8090547 PR: https://github.com/openjdk/jfx/pull/563 * Add DirectionalLight JBS: https://bugs.openjdk.java.net/browse/JDK-8234921 PR: https://github.com/openjdk/jfx/pull/548 * CSS pseudoclasses :focus-visible and :focus-within https://bugs.openjdk.java.net/browse/JDK-8268225 PR: https://github.com/openjdk/jfx/pull/475 * Improve property system to facilitate correct usage (minus the binary incompatible API change) JBS: https://bugs.openjdk.java.net/browse/JDK-8268642 PR: https://github.com/openjdk/jfx/pull/590 (Draft) And maybe the following: * Add CSS themes as a first-class concept (need a consensus on how to proceed) * Undecorated interactive stage style (still in early discussion, but the concept looks interesting and useful) There are probably others I'm forgetting. Each of the above should be discussed in their own thread on openjfx-dev rather than a reply to this thread. -- Kevin
RFR: 8269081: Tree/ListViewSkin: must remove flow on dispose
left-over issue from cleanup of Tree/ListViewSkin: direct children that have been added by the skin must be removed in dispose fixed by removing the flow (which allowed to revert the previous cleanup of event handlers/cellfactory) added tests to verify - constant child count after replacing skin - no memory leak when switching skin while showing - Commit messages: - 826908: Tree/ListViewSkin: must remove flow on dispose Changes: https://git.openjdk.java.net/jfx/pull/612/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx&pr=612&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8269081 Stats: 90 lines in 3 files changed: 68 ins; 18 del; 4 mod Patch: https://git.openjdk.java.net/jfx/pull/612.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/612/head:pull/612 PR: https://git.openjdk.java.net/jfx/pull/612
RFR: 8269871: CellEditEvent: must not throw NPE in accessors
The issue is unguarded access to tablePosition though it might be null (since [JDK-8120610](https://bugs.openjdk.java.net/browse/JDK-8120610) The fix is to check against null in each accessor. Also required to fix the default onEditCommit handlers in Tree/TableColumn to really cope with null TablePostion on the event. Added tests that failed/pass before/after the fix. Note that there was an old test (in Tree/TableColumnTest each), that failed after the fix because it expected the NPE. Fixed by removing the expected parameter. - Commit messages: - 8269871: CellEditEvent: must not throw NPE in accessors Changes: https://git.openjdk.java.net/jfx/pull/611/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx&pr=611&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8269871 Stats: 413 lines in 6 files changed: 393 ins; 5 del; 15 mod Patch: https://git.openjdk.java.net/jfx/pull/611.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/611/head:pull/611 PR: https://git.openjdk.java.net/jfx/pull/611
Re: Can't login to jbs ..
yeah, working again - thanks everybody :) Zitat von Kevin Rushforth : Can you try again? I was able to login a few minutes ago, so this outage might be resolved. -- Kevin On 8/26/2021 5:04 AM, Jayathirth D V wrote: I am also facing the issue. Looks like some generic issue. Thanks, Jay On 26-Aug-2021, at 5:33 PM, Jeanette Winzenburg wrote: .. general problem or just me? How to solve? Thanks for any help, Jeanette
Can't login to jbs ..
.. general problem or just me? How to solve? Thanks for any help, Jeanette
Integrated: 8244419: TextAreaSkin: throws UnsupportedOperation on dispose
On Sat, 14 Aug 2021 10:32:00 GMT, Jeanette Winzenburg wrote: > The issue was a glaring contract violation of TextAreaSkin which throws a > UnsupportedOperationException. The fix was to remove the throwing and cleanup > on dispose which implies > > in TextAreaBehavior: > - remove the listener to focusProperty in dispose > > in TextAreaSkin: > - register all listeners to control properties via skin api > - remove installed event filter in dispose > - remove direct children (here only the scrollPane) > > Added tests to guard the listener re-wiring (must pass before and after), and > tests to expose side-effects on replacing the skin (fail before, pass after) This pull request has now been integrated. Changeset: 5e9f6171 Author:Jeanette Winzenburg URL: https://git.openjdk.java.net/jfx/commit/5e9f6171289ea20e2d700f2422a4eae50287dd41 Stats: 391 lines in 8 files changed: 332 ins; 37 del; 22 mod 8244419: TextAreaSkin: throws UnsupportedOperation on dispose Reviewed-by: mhanl, arapte - PR: https://git.openjdk.java.net/jfx/pull/604
Re: RFR: 8188026: TextFieldXXCell: NPE on calling startEdit [v4]
On Tue, 17 Aug 2021 16:15:47 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
>
> Marius Hanl has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Separated test and made the cell a supplier instead
added inline comments, plus: there's an open change request in my last review:
- not yet covered: sanity test that startEdit really installs the editing
visuals - f.i. by checking that its graphic is null before/ not-null after
startEdit
might be arguable (might have found the failing tests, though, .. or not :) -
personally, I would prefer to only resolve if fully addressed: either after
adding the change or consenting to not adding it.
modules/javafx.controls/src/main/java/javafx/scene/control/cell/ChoiceBoxListCell.java
line 304:
> 302: if (isEditing()) {
> 303: setText(null);
> 304: setGraphic(choiceBox);
at this point, the cell is editing (backed out earlier, if not)
modules/javafx.controls/src/main/java/javafx/scene/control/cell/ChoiceBoxTreeCell.java
line 301:
> 299: return;
> 300: }
> 301:
(darn, can't add the important lines - which is backing out if treeItem is null)
The re-ordering leads to change of behavior, here's a test that's
passing/failing before/after:
/**
* change of behavior: cell must not be editing if treeItem == null.
* fails with fix, passes without
*/
@Test
public void testChoiceBoxTreeCellEditing() {
TreeView treeView = new TreeView<>();
treeView.setEditable(true);
ChoiceBoxTreeCell cell = new ChoiceBoxTreeCell<>();
cell.updateTreeView(treeView);
cell.updateItem("TEST", false);
cell.startEdit();
assertFalse(cell.isEditing());
assertNull(cell.getGraphic());
}
same for ComboBoxTreeCell
modules/javafx.controls/src/main/java/javafx/scene/control/cell/ComboBoxTreeCell.java
line 331:
> 329: if (!isEditing()) {
> 330: return;
> 331: }
same as ChoiceBoxTreeCell
modules/javafx.controls/src/main/java/javafx/scene/control/cell/TextFieldTreeCell.java
line 195:
> 193: if (!isEditing()) {
> 194: return;
> 195: }
similar to ChoiceBox/ComboBoxTreeCell, except that a similar test fails both
before/after the fix
modules/javafx.controls/src/test/java/test/javafx/scene/control/ListCellStartEditTest.java
line 27:
> 25:
> 26: package test.javafx.scene.control;
> 27:
the issue is about specialized cells in package xx.cell, the tests should be
also reside in that package
modules/javafx.controls/src/test/java/test/javafx/scene/control/ListCellStartEditTest.java
line 65:
> 63: public static Collection data() {
> 64: return wrapAsObjectArray(List.of(ListCell::new,
> ComboBoxListCell::new, TextFieldListCell::new,
> 65: ChoiceBoxListCell::new, () -> new CheckBoxListCell<>(obj
> -> new SimpleBooleanProperty(;
the issue is about the specialized cells, so I would suggest to remove the base
Cell here, it's fully (maybe :) already
Doing so, makes it also simpler to test the not/existance of the editing ui
(which is not yet addressed in the tests)
modules/javafx.controls/src/test/java/test/javafx/scene/control/ListCellStartEditTest.java
line 84:
> 82: @Test
> 83: public void testStartEditMustNotThrowNPE() {
> 84: ListCell listCell = listCellSupplier.get();
the usual pattern is to create the instances is to do so in setup, no
Re: RFR: 8188026: TextFieldXXCell: NPE on calling startEdit [v3]
On Mon, 16 Aug 2021 17:09:25 GMT, Marius Hanl wrote: >> modules/javafx.controls/src/test/java/test/javafx/scene/control/ListCellStartEditTest.java >> line 75: >> >>> 73: this.listCell = listCell; >>> 74: } >>> 75: >> >> this hit me when trying separate out the (accidentally?) single test method: >> all tests work on the same instance of the cell, so its state is >> unpredictable. Instead, parameterize on a supplier and let it provide a new >> cell in setup. > > I see, another point where I would love we actually use JUnit5. :/ ehh .. this is not resolved - still change required - or what do I miss? - PR: https://git.openjdk.java.net/jfx/pull/569
RFR: 8244419: TextAreaSkin: throws UnsupportedOperation on dispose
The issue was a glaring contract violation of TextAreaSkin which throws a UnsupportedOperationException. The fix was to remove the throwing and cleanup on dispose which implies in TextAreaBehavior: - remove the listener to focusProperty in dispose in TextAreaSkin: - register all listeners to control properties via skin api - remove installed event filter in dispose - remove direct children (here only the scrollPane) Added tests to guard the listener re-wiring (must pass before and after), and tests to expose side-effects on replacing the skin (fail before, pass after) - Commit messages: - 8244419: TextAreaSkin: throws UnsupportedOperation on dispose Changes: https://git.openjdk.java.net/jfx/pull/604/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx&pr=604&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8244419 Stats: 392 lines in 8 files changed: 332 ins; 37 del; 23 mod Patch: https://git.openjdk.java.net/jfx/pull/604.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/604/head:pull/604 PR: https://git.openjdk.java.net/jfx/pull/604
Integrated: 8271484: Tree-/TableCell: NPE when accessing edit event from startEdit
On Thu, 29 Jul 2021 14:11:15 GMT, Jeanette Winzenburg wrote: > The NPE was an indirect effect: > > - the bug on part of the cell was an incorrect (== missing) edit location in > cellEditEvent - that's fixed in this PR > - a related bug is on part of the implementation of CellEditEvent (assuming > that TablePosition != null, > [JDK-8269871](https://bugs.openjdk.java.net/browse/JDK-8269871) which is not > addressed in this PR > > Fix is to use the cell's state at startEdit for creating the editing > location, added tests starting the edit on the cell that failed/passed > before/after the fix, also added sanity tests with starting edit on the > control that passed before/after. This pull request has now been integrated. Changeset: 852d2875 Author:Jeanette Winzenburg URL: https://git.openjdk.java.net/jfx/commit/852d2875248040ea5eff2b41bd23a4fde02486a9 Stats: 57 lines in 4 files changed: 53 ins; 2 del; 2 mod 8271484: Tree-/TableCell: NPE when accessing edit event from startEdit Reviewed-by: aghaisas - PR: https://git.openjdk.java.net/jfx/pull/592
