On Tue, 4 Feb 2025 19:39:07 GMT, Michael Strauß <mstra...@openjdk.org> wrote:
>> This PR contains a definition of preview features for JavaFX, as well as a >> helper class to verify that an application has opted into preview features. > > Michael Strauß has updated the pull request incrementally with one additional > commit since the last revision: > > move enum field to top Overall, I like the direction of this. This isn't a review (yet), but just a few high-level comments (I also left a couple inline comments). As with Incubating Modules, the goal is to provide features that are not yet final, and are able to evolve before being finalized. To achieve this without surprising application developers and end users, we need to provide tools to allow developers and end users to know that they are using preview features that might work differently or not be available in subsequent versions of JavaFX. I think it generally does that, although I'll review it in more detail later. * I recommend a Draft PR with an example preview feature. That would also be a good place to host the JEP document (for now). You can see what I did in PR #1617 for Incubator Modules. * One of the important tenets of this is that the application developer needs to "opt in" in an obvious way. Since we can't have javac and java launcher support for this, your idea of a system property seems a good one. The system property should be read as early as possible to encourage passing it on the command line to make it more obvious (similarly our docs should show examples of running with `java -Djavafx.enablePreview=...` rather than a call to `System.setProperty`). Initializing the `PreviewFeature` class from the static initializer of `PlatformImpl` and `LauncherImpl` (as I did when adding the security manager check) might be helpful to this end. * Should the property be an integer (feature release) rather than boolean? Without some sort of annotation, I'm not sure it would help, but I'll throw it out there * I don't think the warning should be suppressible; the equivalent JDK warning isn't * Is there anything we could do with annotations that might be useful? Not sure on this one modules/javafx.base/src/main/java/com/sun/javafx/PreviewFeature.java line 52: > 50: private static final String SUPPRESS_WARNING_PROPERTY = > "javafx.suppressPreviewBanner"; > 51: > 52: private static final boolean enabled = > Boolean.getBoolean(ENABLE_PREVIEW_PROPERTY); The JDK requires that you opt into preview features _for a specific version_. That is, rather than a boolean, the JDK uses an integer feature release value that must match the current release. They do this by using the `--release` option (in connection with the `--enable-preview`), and compiling that into the class file, which we can't directly use. Maybe we can do something similar, though? ------------- PR Review: https://git.openjdk.org/jfx/pull/1359#pullrequestreview-2593835213 PR Review Comment: https://git.openjdk.org/jfx/pull/1359#discussion_r1941798607
