On Wed, 10 Feb 2021 13:12:17 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
>> not going for a full review - just a comment: agree with Kevin, the delegate >> methods are the way out, basically look good >> >> `No further changes in testing required as it is all covered` - a bold >> statement .. looks like there's a missed null check (which was in the >> listener code but didn't make it into the delegate) at the end of >> sceneChanged >> >> windowChanged(oldScene.getWindow(), newScene.getWindow()); >> >> any of old/new scene can be null (or what am I missing?) If that's not >> covered by the tests .. ;) > >> any of old/new scene can be null (or what am I missing?) If that's not >> covered by the tests .. ;) > > Yep. And if you look at the checks run by GitHub actions, there are failing > tests. For example: > > 2021-02-09T20:57:51.7835652Z > test.com.sun.javafx.scene.control.infrastructure.ControlSkinFactoryTest > > testAlternativeSkinAssignable FAILED > 2021-02-09T20:57:51.7838200Z java.lang.RuntimeException: > java.lang.reflect.InvocationTargetException > 2021-02-09T20:57:51.7841382Z at > test.com.sun.javafx.scene.control.infrastructure.ControlSkinFactory.createAlternativeSkin(ControlSkinFactory.java:323) > 2021-02-09T20:57:51.7845300Z at > test.com.sun.javafx.scene.control.infrastructure.ControlSkinFactory.replaceSkin(ControlSkinFactory.java:302) > 2021-02-09T20:57:51.7852081Z at > test.com.sun.javafx.scene.control.infrastructure.ControlSkinFactoryTest.testAlternativeSkinAssignable(ControlSkinFactoryTest.java:91) > 2021-02-09T20:57:51.7854816Z > 2021-02-09T20:57:51.7855105Z Caused by: > 2021-02-09T20:57:51.7855858Z > java.lang.reflect.InvocationTargetException > 2021-02-09T20:57:51.7857628Z at > java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native > Method) > 2021-02-09T20:57:51.7860264Z at > java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:64) > 2021-02-09T20:57:51.7863798Z at > java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45) > 2021-02-09T20:57:51.7866625Z at > java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:500) > 2021-02-09T20:57:51.7868214Z at > java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:481) > 2021-02-09T20:57:51.7870982Z at > test.com.sun.javafx.scene.control.infrastructure.ControlSkinFactory.createAlternativeSkin(ControlSkinFactory.java:321) > 2021-02-09T20:57:51.7873192Z ... 2 more > 2021-02-09T20:57:51.7873407Z > 2021-02-09T20:57:51.7873689Z Caused by: > 2021-02-09T20:57:51.7874236Z java.lang.NullPointerException > 2021-02-09T20:57:51.7875765Z at > javafx.graphics/com.sun.javafx.scene.TreeShowingExpression.sceneChanged(TreeShowingExpression.java:127) > 2021-02-09T20:57:51.7877830Z at > javafx.graphics/com.sun.javafx.scene.TreeShowingExpression.<init>(TreeShowingExpression.java:66) > 2021-02-09T20:57:51.7879836Z at > javafx.controls/javafx.scene.control.skin.ProgressIndicatorSkin.<init>(ProgressIndicatorSkin.java:130) > 2021-02-09T20:57:51.7881678Z at > javafx.controls/javafx.scene.control.skin.ProgressBarSkin.<init>(ProgressBarSkin.java:97) > 2021-02-09T20:57:51.7883928Z at > test.com.sun.javafx.scene.control.infrastructure.ControlSkinFactory$ProgressBarSkin1.<init>(ControlSkinFactory.java:515) > 2021-02-09T20:57:51.7885496Z ... 8 more good ;) ------------- PR: https://git.openjdk.java.net/jfx/pull/185
