On Sat, 9 Nov 2024 07:45:14 GMT, Michael Strauß <mstra...@openjdk.org> wrote:
>> None of these classes can be extended by user code, and any attempt to do so >> will fail at runtime with an exception. For this reason, we can seal the >> class hierarchy and remove the run-time checks to turn this into a >> compile-time error instead. >> >> In some cases, `Node` and `Shape` are extended by JavaFX classes in other >> modules, preventing those derived classes from being permitted subclasses. A >> non-exported `AbstractNode` and `AbstractShape` class is provided just for >> these scenarios. Note that introducing a new superclass is a source- and >> binary-compatible change (see [JLS ch. >> 13](https://docs.oracle.com/javase/specs/jls/se22/html/jls-13.html)). >> >> I'm not sure if this change requires a CSR, as it doesn't change the >> specification in any meaningful way. There can be no valid JavaFX program >> that is affected by this change. > > Michael Strauß has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains eight commits: > > - Merge branch 'master' into feature/sealed-classes > - Merge branch 'master' into feature/sealed-classes > - Merge branch 'master' into feature/sealed-classes > - Merge branch 'master' into feature/sealed-classes > - add comment > - Merge branch 'master' into feature/sealed-classes > - remove documentation > - Seal Node, Camera, LightBase, Shape, Shape3D This looks good. I left one question about whether (or not) to throw `InternalError` on a null (this was a case of a runtime check that should no longer be possible). I also left a question in the CSR about the two public classes (MediaView and SwingNode) that extend `AbstractNode`. They show up in the javadocs, even though they are inaccessible. I think this is OK, but wanted Joe's take on it. modules/javafx.graphics/src/main/java/com/sun/javafx/scene/NodeHelper.java line 66: > 64: > 65: protected static NodeHelper getHelper(Node node) { > 66: return nodeAccessor.getHelper(node); Is it worth checking for null and throwing an internal error if it is? ------------- Marked as reviewed by kcr (Lead). PR Review: https://git.openjdk.org/jfx/pull/1556#pullrequestreview-2462711912 PR Review Comment: https://git.openjdk.org/jfx/pull/1556#discussion_r1859251468
