Re: RFR: 8185887: TableRowSkinBase fails to correctly virtualize cells in horizontal direction [v4]
On Wed, 9 Sep 2020 02:46:37 GMT, yosbits wrote: >> @yososs Per [this >> message](https://mail.openjdk.java.net/pipermail/openjfx-dev/2020-September/027534.html) >> on the >> openjfx-dev mailing list, I have closed >> [JDK-8185886](https://bugs.openjdk.java.net/browse/JDK-8185886) as a >> duplicate, >> and suggested another existing JBS issue for this PR to use. Please change >> the title to: 8185887: TableRowSkinBase >> fails to correctly virtualize cells in horizontal direction > > Below are the answers to JBS's suggestions. > >> The patch below resolves the issue, but it is not likely the correct >> solution (we are trying to determine the table >> width, but we are getting the width and padding on the underlying >> virtualflow (the width is fine, the padding should >> really come from tableview directly): > > You probably mention this part, > At least padding refers to Skinnable(TableRow or TreeTableRow). This is the > same as before (L683). The width of > VirtualFlow is determined by the header of Table. > modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkinBase.java#L360-L365 > Java > final VirtualFlow virtualFlow = getVirtualFlow(); > final double scrollX = virtualFlow == null ? 0.0 : > virtualFlow.getHbar().getValue(); > final Insets padding = getSkinnable().getPadding(); > final double vfWidth = virtualFlow == null ? 0.0:virtualFlow.getWidth(); > final double headerWidth = vfWidth - (padding.getLeft() + > padding.getRight()); > > For example, can you assume a case that does not work properly? This is the answer to JBS's comment. The BigTreeTableViewTest.java attached to this PR commentary is a modified version of the JDK-8166956 test code. The original test code is good for reproducing the problem, but I have decided that it cannot be used as a test code because the cell values are random and the cell values change randomly with the close / expand operation. - PR: https://git.openjdk.java.net/jfx/pull/125
Re: RFR: 8252935: Add treeShowing listener only when needed
On Fri, 17 Apr 2020 17:18:00 GMT, John Hendrikx wrote: >> These listeners exist for a good reason. Removing them will almost certainly >> cause regressions in behavior. See >> [JDK-8090322](https://bugs.openjdk.java.net/browse/JDK-8090322) as well as >> the follow-on fixes (since that was a rather >> involved fix in the first place). > > @kevinrushforth I don't propose to remove them, only to move them to where > they are needed. > > Currently they are part of Node, which means all nodes, whether they need to > know the state of the Window or not are > registering a listener. However, only PopupWindow and the > ProgressIndicatorSkin are registering a listener on this > property (other uses are limited to a simple "isTreeShowing" checks which can > be determined directly).It is very > wasteful to have all nodes register this listener, as there are potentially > tens of thousands of nodes. All of these > nodes are listening on the exact same two properties (when there is only one > Scene and Window), causing huge listener > lists there. The listener is not registered in a lazy fashion (ie, only > registered if something else is listening to > the property downstream, like reactfx would do).This also means that when > a Scene change or Window showing change > occurs, thousands of nodes will receive a change event, but only 2 types of > nodes are actually interested. The other > nodes are just doing a lot of house keeping to keep watching the correct > property (as there is an indirection from > Scene to Window). Therefore my proposal is to have the two cases involved > register their own listener on Scene and > Window. There will not be any regressions. The two tests that currently > fail in this PR are expected to fail as I > commented out the listener code in the classes involved, but that can easily > be fixed by adding the correct listeners > there. I'll update it so you can see all tests will pass. @kevinrushforth I've updated this PR now with proper listeners added in again for PopupWindow and ProgressIndicatorSkin. In short, the functionality to support the `treeShowingProperty` has been extracted to a separate class `TreeShowingExpression` which is now used by the two classes mentioned. All tests pass, including the memory leak tests that failed before. The issue JDK-8090322 you mentioned actually cautioned for not adding such listeners for all nodes and seemed to propose the kind of solution I constructed here with a separate class for the `treeShowingProperty`: > This is too expensive to calculate for all nodes by default. So the simplest > way to provide it would be a special > binding implementation or a util class. Where you create a instance and pass > in the node you are interested in. It can > then register listeners all the way up the tree and listen to what it needs. - PR: https://git.openjdk.java.net/jfx/pull/185
Re: RFR: 8252935: Add treeShowing listener only when needed
On Fri, 17 Apr 2020 23:47:42 GMT, John Hendrikx wrote: >> @kevinrushforth I don't propose to remove them, only to move them to where >> they are needed. >> >> Currently they are part of Node, which means all nodes, whether they need to >> know the state of the Window or not are >> registering a listener. However, only PopupWindow and the >> ProgressIndicatorSkin are registering a listener on this >> property (other uses are limited to a simple "isTreeShowing" checks which >> can be determined directly).It is very >> wasteful to have all nodes register this listener, as there are potentially >> tens of thousands of nodes. All of these >> nodes are listening on the exact same two properties (when there is only one >> Scene and Window), causing huge listener >> lists there. The listener is not registered in a lazy fashion (ie, only >> registered if something else is listening to >> the property downstream, like reactfx would do).This also means that >> when a Scene change or Window showing change >> occurs, thousands of nodes will receive a change event, but only 2 types of >> nodes are actually interested. The other >> nodes are just doing a lot of house keeping to keep watching the correct >> property (as there is an indirection from >> Scene to Window). Therefore my proposal is to have the two cases involved >> register their own listener on Scene and >> Window. There will not be any regressions. The two tests that currently >> fail in this PR are expected to fail as I >> commented out the listener code in the classes involved, but that can easily >> be fixed by adding the correct listeners >> there. I'll update it so you can see all tests will pass. > > @kevinrushforth I've updated this PR now with proper listeners added in again > for PopupWindow and > ProgressIndicatorSkin. In short, the functionality to support the > `treeShowingProperty` has been extracted to a > separate class `TreeShowingExpression` which is now used by the two classes > mentioned. All tests pass, including the > memory leak tests that failed before. > The issue JDK-8090322 you mentioned actually cautioned for not adding such > listeners for all nodes and seemed to > propose the kind of solution I constructed here with a separate class for the > `treeShowingProperty`: >> This is too expensive to calculate for all nodes by default. So the simplest >> way to provide it would be a special >> binding implementation or a util class. Where you create a instance and pass >> in the node you are interested in. It can >> then register listeners all the way up the tree and listen to what it needs. @kevinrushforth I have another working alternative, but won't make a PR for it until it is more clear which direction the TableView / TreeTableView performance fixes are going to take. The alternative would convert the `treeShowing` property in `Node` into a "lazy" property (something which JavaFX does not support directly at the moment). A lazy property only binds to its dependencies when it is observed itself (so in this specific case, only when PopupWindow or ProgressIndicatorSkin are making use of it). This means the property stays a part of `NodeHelper` but will only register its listeners on Window and Scene when it is observed itself. Such lazy properties could be of great use in JavaFX in general, not just in this case. - PR: https://git.openjdk.java.net/jfx/pull/185
Re: RFR: 8252935: Add treeShowing listener only when needed
On Tue, 21 Apr 2020 14:22:46 GMT, John Hendrikx wrote: >> @kevinrushforth I've updated this PR now with proper listeners added in >> again for PopupWindow and >> ProgressIndicatorSkin. In short, the functionality to support the >> `treeShowingProperty` has been extracted to a >> separate class `TreeShowingExpression` which is now used by the two classes >> mentioned. All tests pass, including the >> memory leak tests that failed before. >> The issue JDK-8090322 you mentioned actually cautioned for not adding such >> listeners for all nodes and seemed to >> propose the kind of solution I constructed here with a separate class for >> the `treeShowingProperty`: >>> This is too expensive to calculate for all nodes by default. So the >>> simplest way to provide it would be a special >>> binding implementation or a util class. Where you create a instance and >>> pass in the node you are interested in. It can >>> then register listeners all the way up the tree and listen to what it needs. > > @kevinrushforth > > I have another working alternative, but won't make a PR for it until it is > more clear which direction the TableView / > TreeTableView performance fixes are going to take. > The alternative would convert the `treeShowing` property in `Node` into a > "lazy" property (something which JavaFX does > not support directly at the moment). A lazy property only binds to its > dependencies when it is observed itself (so in > this specific case, only when PopupWindow or ProgressIndicatorSkin are making > use of it). This means the property > stays a part of `NodeHelper` but will only register its listeners on Window > and Scene when it is observed itself. > Such lazy properties could be of great use in JavaFX in general, not just in > this case. @hjohn Per [this message](https://mail.openjdk.java.net/pipermail/openjfx-dev/2020-September/027534.html) on the openjfx-dev mailing list, I have filed a new JBS issue for this PR to use. Please change the title to: 8252935: Add treeShowing listener only when needed - PR: https://git.openjdk.java.net/jfx/pull/185
Re: RFR: 8252935: Add treeShowing listener only when needed
On Fri, 17 Apr 2020 16:59:29 GMT, Kevin Rushforth wrote: >> This is a PoC for performance testing. >> >> It contains commented out code in PopupWindow and ProgressIndicatorSkin and >> two tests are failing because of that. >> >> This code avoids registering two listeners (on Scene and on Window) for each >> and every Node to support the >> aforementioned classes. The complete change should update these two classes >> to add their own listeners instead. > > These listeners exist for a good reason. Removing them will almost certainly > cause regressions in behavior. See > [JDK-8090322](https://bugs.openjdk.java.net/browse/JDK-8090322) as well as > the follow-on fixes (since that was a rather > involved fix in the first place). @kevinrushforth I don't propose to remove them, only to move them to where they are needed. Currently they are part of Node, which means all nodes, whether they need to know the state of the Window or not are registering a listener. However, only PopupWindow and the ProgressIndicatorSkin are registering a listener on this property (other uses are limited to a simple "isTreeShowing" checks which can be determined directly). It is very wasteful to have all nodes register this listener, as there are potentially tens of thousands of nodes. All of these nodes are listening on the exact same two properties (when there is only one Scene and Window), causing huge listener lists there. The listener is not registered in a lazy fashion (ie, only registered if something else is listening to the property downstream, like reactfx would do). This also means that when a Scene change or Window showing change occurs, thousands of nodes will receive a change event, but only 2 types of nodes are actually interested. The other nodes are just doing a lot of house keeping to keep watching the correct property (as there is an indirection from Scene to Window). Therefore my proposal is to have the two cases involved register their own listener on Scene and Window. There will not be any regressions. The two tests that currently fail in this PR are expected to fail as I commented out the listener code in the classes involved, but that can easily be fixed by adding the correct listeners there. I'll update it so you can see all tests will pass. - PR: https://git.openjdk.java.net/jfx/pull/185
Re: RFR: 8252935: Add treeShowing listener only when needed
On Fri, 17 Apr 2020 08:06:23 GMT, John Hendrikx wrote: > This is a PoC for performance testing. > > It contains commented out code in PopupWindow and ProgressIndicatorSkin and > two tests are failing because of that. > > This code avoids registering two listeners (on Scene and on Window) for each > and every Node to support the > aforementioned classes. The complete change should update these two classes > to add their own listeners instead. These listeners exist for a good reason. Removing them will almost certainly cause regressions in behavior. See [JDK-8090322](https://bugs.openjdk.java.net/browse/JDK-8090322) as well as the follow-on fixes (since that was a rather involved fix in the first place). - PR: https://git.openjdk.java.net/jfx/pull/185
RFR: 8252935: Add treeShowing listener only when needed
This is a PoC for performance testing. It contains commented out code in PopupWindow and ProgressIndicatorSkin and two tests are failing because of that. This code avoids registering two listeners (on Scene and on Window) for each and every Node to support the aforementioned classes. The complete change should update these two classes to add their own listeners instead. - Commit messages: - WIP: Moved treeShowingProperty into its own class Changes: https://git.openjdk.java.net/jfx/pull/185/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx&pr=185&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8252935 Stats: 275 lines in 6 files changed: 161 ins; 109 del; 5 mod Patch: https://git.openjdk.java.net/jfx/pull/185.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/185/head:pull/185 PR: https://git.openjdk.java.net/jfx/pull/185
Cannot extend javafx.scene.transform.Transform
I would like to extend from javafx.scene.transform.Transform Two methods are preventing this … abstract void apply(Affine3D t); abstract BaseTransform derive(BaseTransform t); I accomplished this in jdk 8 by overriding these two methods … public abstract void impl_apply(final Affine3D trans); public abstract BaseTransform impl_derive(final BaseTransform trans); Which of course I should not have done. Is there any change the apply and derive methods could open to permit derivation?
Re: RFR: 8209788: Left/Right/Ctrl+A keys not working in editor of ComboBox if popup showing [v11]
On Mon, 7 Sep 2020 13:33:48 GMT, Ambarish Rapte wrote:
>> The issue occurs because the key events are consumed by the `ListView` in
>> `Popup`, which displays the items.
>> This is a regression of
>> [JDK-8077916](https://bugs.openjdk.java.net/browse/JDK-8077916). This change
>> aadded several
>> `KeyMapping`s for focus traversals to `ListView`, which consume the `Left`,
>> `Right` and `Ctrl+A` key events.
>> Fix:
>> 1. Remove the four focus traversal arrow `KeyMapping`s from
>> `ListViewBehavior`.
>> 2. Add the `Ctrl + A` `KeyMapping` to `ListViewBehavior` only if the
>> `ListView`'s selection mode is set to
>> `SelectionMode.MULTIPLE`. `ComboBox` uses the `ListView` with
>> `SelectionMode.SINGLE` mode.
>> Change unrelated to fix:
>> `ComboBoxListViewBehavior` adds `KeyMapping` for `Up` and `Down` keys, which
>> are not invoked when the `ComboBox` popup
>> is showing. When the popup is shown, the Up and Down key events are handled
>> by the `ListView` and the `KeyMapping` code
>> from `ComboBoxListViewBehavior` does not get executed. These KeyMapping are
>> removed. However this change is not needed
>> for the fix. But this seems to be dead code. Verification:
>> Added new unit tests to verify the change.
>> Also verified that the behavior ListView behaves same before and after this
>> change.
>
> Ambarish Rapte has updated the pull request incrementally with one additional
> commit since the last revision:
>
> rename tests
Fix looks fine and indeed pretty :) Verified tests failing before and after the
fix.
Left some minor comments inline.
modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/ListViewBehavior.java
line 87:
> 85: Predicate pIsInComboBox = e -> isListViewOfComboBox !=
> null;
> 86: Predicate pIsInEditableComboBox =
> 87: e -> isListViewOfComboBox != null &&
> isListViewOfComboBox.get();
nit-pick about naming: I think we don't use (crippled) hungarian notation, or
do we? If we don't, the leading "p"
should be removed, isIn/Editable/Combo is expressive enough (and no need to
differentiate by type of functional
interface, IMO)
modules/javafx.controls/src/main/java/javafx/scene/control/skin/ComboBoxListViewSkin.java
line 509:
> 507: getProperties().put("selectFirstRowByDefault", false);
> 508: getProperties().put("editableComboBoxEditor",
> (Supplier) () ->
> getSkinnable().isEditable()); 509: }
nit-pick about naming: it's the editable state of the combo (vs. the editable
state of the editor) that's in the center
of interest, so maybe rename the key to express that? Like "editableComboBox"?
Another thingy (which I think is a bit under-used in the fx code-base;) - how
about a code comment to why this marker
is added and which collaborator is using it?
modules/javafx.controls/src/test/java/test/javafx/scene/control/ComboBoxTest.java
line 1347:
> 1345: final ComboBox cb = new
> ComboBox<>(FXCollections.observableArrayList("a", "b", "c"));
> 1346: cb.setEditable(true);
> 1347: StageLoader sl = new StageLoader(cb);
shouldn't there be an analogous functional test for not-editable combo? There
are both functional and low-level for
editable, but only low-level for not editable.
modules/javafx.controls/src/test/java/test/javafx/scene/control/ComboBoxTest.java
line 1514:
> 1512: return
> ((KeyMapping)mappings.get(i)).getInterceptor().test(null);
> 1513: }
> 1514:
his throws an NPE without the fix - which makes the test using this method
still fail (good!), but a bit unspecific for
my taste. A slight re-organisation of the helpers might help: move the asserts
down instead of returning a boolean.
modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java
line 2083:
> 2081: private boolean testInterceptor(ObservableList mappings,
> KeyBinding binding) {
> 2082: int i = mappings.indexOf(new KeyMapping(binding, null));
> 2083: return
> ((KeyMapping)mappings.get(i)).getInterceptor().test(null);
same as for ComboBoxTest
-
Changes requested by fastegal (Committer).
PR: https://git.openjdk.java.net/jfx/pull/172
