On Thu, 19 Jun 2025 08:47:08 GMT, Johan Vos <j...@openjdk.org> wrote:
>> After spending a year in the sandbox repository, the Headless Platform is
>> now ready to be reviewed in the main repository.
>>
>> ### the Headless Platform
>> The Headless Platform is a top-level com.sun.glass.ui platform that replaces
>> the second-level Monocle-Headless subplatform, that is part of the top-level
>> Monocle platform.
>> The platform can be used like any other platform, especially for running
>> headless JavaFX applications, or for running tests (e.g. on CI systems)
>>
>> ### changes
>> The code for the Headless Platform is in a new package
>> com.sun.glass.ui.headless in the javafx.graphics module, and it does not
>> require a code change in other packages.
>> This PR adds a simple change in the `build.gradle` file, to make the
>> Headless Platform the standard when running headless tests (instead of using
>> Monocle/Headless)
>>
>> ### enable the Headless Platform
>> Setting the system property `glass.platform` to `Headless` will select the
>> Headless Platform instead of the default one (either gtk, mac or win).
>>
>> ### testing
>> `gradlew --info -PHEADLESS_TEST=true -PFULL_TEST=true :systemTests:cleanTest
>> :systemTests:test`
>> runs all the system tests, apart from the robot tests. There are 2 failing
>> tests, but there are valid reasons for those to fail.
>>
>> ### robot tests
>> Most of the robot tests are working on headless as well. add `-PUSE_ROBOT`
>> to test those.
>
> Johan Vos has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix missing ;
My testing shows that the headless platform works as expected.
Some general comments:
1. All files need a copyright header.
2. Very long lines can benefit from a few line breaks.
modules/javafx.graphics/src/main/java/com/sun/glass/ui/headless/HeadlessApplication.java
line 23:
> 21: private final HeadlessWindowManager windowManager = new
> HeadlessWindowManager();
> 22: private Screen[] screens = null;
> 23: private HeadlessCursor cursor;
This field is not used in this class, other than assigning a value.
modules/javafx.graphics/src/main/java/com/sun/glass/ui/headless/HeadlessApplication.java
line 29:
> 27: private final int MULTICLICK_MAX_X = 20;
> 28: private final int MULTICLICK_MAX_Y = 20;
> 29: private final long MULTICLICK_TIME = 500;
These fields should be `static`.
modules/javafx.graphics/src/main/java/com/sun/glass/ui/headless/HeadlessApplication.java
line 173:
> 171:
> 172: @Override
> 173: protected CommonDialogs.FileChooserResult
> staticCommonDialogs_showFileChooser(Window owner, String folder, String
> filename, String title, int type, boolean multipleMode,
> CommonDialogs.ExtensionFilter[] extensionFilters, int defaultFilterIndex) {
This is an extremely long line...
modules/javafx.graphics/src/main/java/com/sun/glass/ui/headless/HeadlessPlatformFactory.java
line 52:
> 50: }
> 51:
> 52: class HeadlessSystemClipboard extends SystemClipboard {
This class can be `static`.
modules/javafx.graphics/src/main/java/com/sun/glass/ui/headless/HeadlessPlatformFactory.java
line 92:
> 90: }
> 91:
> 92: class HeadlessDnDClipboard extends SystemClipboard {
This class can be `static`.
modules/javafx.graphics/src/main/java/com/sun/glass/ui/headless/HeadlessRobot.java
line 63:
> 61: view.notifyKey(KeyEvent.TYPED, 0, keyval, mods);
> 62: }
> 63:
Minor: empty line
modules/javafx.graphics/src/main/java/com/sun/glass/ui/headless/HeadlessRobot.java
line 213:
> 211: HeadlessView view = (HeadlessView)activeWindow.getView();
> 212: int modifiers = 0;
> 213: view.notifyMouse(MouseEvent.ENTER, MouseEvent.BUTTON_NONE,
> (int)mouseX-wx, (int)mouseY-wy, (int)mouseX, (int)mouseY, modifiers, true,
> true);
Minor: spacing around operators
modules/javafx.graphics/src/main/java/com/sun/glass/ui/headless/HeadlessRobot.java
line 219:
> 217: int owx = oldWindow.getX();
> 218: int owy = oldWindow.getY();
> 219: oldView.notifyMouse(MouseEvent.EXIT,
> MouseEvent.BUTTON_NONE, (int)mouseX-owx, (int)mouseY-owy, (int)mouseX,
> (int)mouseY, modifiers, true, true);
Minor: spacing around operators
modules/javafx.graphics/src/main/java/com/sun/glass/ui/headless/HeadlessRobot.java
line 277:
> 275: modifiers |= KeyEvent.MODIFIER_BUTTON_FORWARD;
> 276: break;
> 277: }
Using an expression can help increase readibility and correctness:
Suggestion:
modifiers |= switch (buttons[i]) {
case NONE -> KeyEvent.MODIFIER_NONE;
case PRIMARY -> KeyEvent.MODIFIER_BUTTON_PRIMARY;
case MIDDLE -> KeyEvent.MODIFIER_BUTTON_MIDDLE;
case SECONDARY -> KeyEvent.MODIFIER_BUTTON_SECONDARY;
case BACK -> KeyEvent.MODIFIER_BUTTON_BACK;
case FORWARD -> KeyEvent.MODIFIER_BUTTON_FORWARD;
};
modules/javafx.graphics/src/main/java/com/sun/glass/ui/headless/HeadlessRobot.java
line 293:
> 291: if (button == MouseButton.BACK) return MouseEvent.BUTTON_BACK;
> 292: if (button == MouseButton.FORWARD) return
> MouseEvent.BUTTON_FORWARD;
> 293: return MouseEvent.BUTTON_NONE;
Use an expression to get compile-time exhaustiveness checks:
Suggestion:
return switch (button) {
case NONE -> MouseEvent.BUTTON_NONE;
case PRIMARY -> MouseEvent.BUTTON_LEFT;
case MIDDLE -> MouseEvent.BUTTON_OTHER;
case SECONDARY -> MouseEvent.BUTTON_RIGHT;
case BACK -> MouseEvent.BUTTON_BACK;
case FORWARD -> MouseEvent.BUTTON_FORWARD;
};
modules/javafx.graphics/src/main/java/com/sun/glass/ui/headless/HeadlessRobot.java
line 390:
> 388: if (this.specialKeys.keyShift) answer = answer |
> KeyEvent.MODIFIER_SHIFT;
> 389: if (this.specialKeys.keyCommand) answer = answer |
> KeyEvent.MODIFIER_COMMAND;
> 390: if (this.specialKeys.keyAlt) answer = answer |
> KeyEvent.MODIFIER_ALT;
You can remove four utterances of the word "answer" by using the `|=` operator.
modules/javafx.graphics/src/main/java/com/sun/glass/ui/headless/HeadlessTimer.java
line 11:
> 9:
> 10: private static ScheduledThreadPoolExecutor scheduler;
> 11: private ScheduledFuture task;
You can use the parameterized type `ScheduledFuture<?>`.
modules/javafx.graphics/src/main/java/com/sun/glass/ui/headless/HeadlessView.java
line 16:
> 14: private Pixels pixels;
> 15:
> 16: private boolean imeEnabled;
The following fields are not read, but only assigned to in this class:
* `capabilities`
* `x`
* `y`
* `imeEnabled`
The field `parentPtr` is neither assigned nor read.
modules/javafx.graphics/src/main/java/com/sun/glass/ui/headless/HeadlessView.java
line 46:
> 44: @Override
> 45: protected void _setParent(long ptr, long parentPtr) {
> 46: parentPtr = parentPtr;
You're assigning a variable to itself. But even if you qualify
`this.parentPtr`, the assignment seems pointless.
modules/javafx.graphics/src/main/java/com/sun/glass/ui/headless/HeadlessView.java
line 79:
> 77: }
> 78:
> 79: Pixels getPixels() {
This method is never used.
modules/javafx.graphics/src/main/java/com/sun/glass/ui/headless/HeadlessWindow.java
line 36:
> 34: private float alpha;
> 35: private Pixels icon;
> 36: private Cursor cursor;
The following fields are never used, other than assigning a value:
* `ptr`
* `resizable`
* `visible`
* `isFocusable`
* `enabled`
* `closed`
* `bg_r`, `bg_g`, `bg_b`
* `alpha`
* `icon`
* `cursor`
modules/javafx.graphics/src/main/java/com/sun/glass/ui/headless/HeadlessWindow.java
line 106:
> 104: this.originalY = this.y;
> 105: newX = 0;
> 106: newY = 0;
`newX` and `newY` already have the value 0.
modules/javafx.graphics/src/main/java/com/sun/glass/ui/headless/HeadlessWindow.java
line 259:
> 257:
> 258: @Override
> 259: protected void _requestInput(long ptr, String text, int type, double
> width, double height, double Mxx, double Mxy, double Mxz, double Mxt, double
> Myx, double Myy, double Myz, double Myt, double Mzx, double Mzy, double Mzz,
> double Mzt) {
Another very long line.
modules/javafx.graphics/src/main/java/com/sun/glass/ui/headless/HeadlessWindow.java
line 268:
> 266: }
> 267:
> 268: boolean setFullscreen(boolean full) {
None of the two callers use the return value of this method.
modules/javafx.graphics/src/main/java/com/sun/glass/ui/headless/HeadlessWindow.java
line 294:
> 292: private void notifyResizeAndMove(int x, int y, int width, int
> height) {
> 293: HeadlessView view = (HeadlessView) getView();
> 294: // if (getWidth() != width || getHeight() != height) {
Why is this code commented out?
modules/javafx.graphics/src/main/java/com/sun/glass/ui/headless/HeadlessWindow.java
line 306:
> 304:
> 305: public Color getColor(int lx, int ly) {
> 306: int mx = lx;// + getX();
Why is this code commented out?
modules/javafx.graphics/src/main/java/com/sun/glass/ui/headless/HeadlessWindow.java
line 350:
> 348: int val = intBuffer.get(i * pixels.getWidth() + j);
> 349: if (val != 0) {
> 350: }
The `if` statement has an empty body.
modules/javafx.graphics/src/main/java/com/sun/glass/ui/headless/HeadlessWindowManager.java
line 22:
> 20: }
> 21:
> 22: private HeadlessWindow getFocusedWindow() {
This private method is never used.
modules/javafx.graphics/src/main/java/com/sun/glass/ui/headless/NestedRunnableProcessor.java
line 20:
> 18: }
> 19:
> 20: void invokeLater(final Runnable r) {
The following methods are never used:
* `invokeLater`
* `runLater`
* `invokeAndWait`
* `stopProcessing`
modules/javafx.graphics/src/main/java/com/sun/glass/ui/headless/NestedRunnableProcessor.java
line 61:
> 59: } catch (Throwable e) {
> 60: e.printStackTrace();
> 61: Application.reportException(e);
Why do you print the exception, _and_ send it to
`Application.reportException()`? The latter already sends it to the uncaught
exception handler, where it is printed by default.
-------------
PR Review: https://git.openjdk.org/jfx/pull/1836#pullrequestreview-2944117528
PR Review Comment: https://git.openjdk.org/jfx/pull/1836#discussion_r2157728622
PR Review Comment: https://git.openjdk.org/jfx/pull/1836#discussion_r2157728091
PR Review Comment: https://git.openjdk.org/jfx/pull/1836#discussion_r2157718050
PR Review Comment: https://git.openjdk.org/jfx/pull/1836#discussion_r2157730230
PR Review Comment: https://git.openjdk.org/jfx/pull/1836#discussion_r2157730745
PR Review Comment: https://git.openjdk.org/jfx/pull/1836#discussion_r2157714682
PR Review Comment: https://git.openjdk.org/jfx/pull/1836#discussion_r2157722605
PR Review Comment: https://git.openjdk.org/jfx/pull/1836#discussion_r2157722822
PR Review Comment: https://git.openjdk.org/jfx/pull/1836#discussion_r2157714269
PR Review Comment: https://git.openjdk.org/jfx/pull/1836#discussion_r2157714007
PR Review Comment: https://git.openjdk.org/jfx/pull/1836#discussion_r2157716281
PR Review Comment: https://git.openjdk.org/jfx/pull/1836#discussion_r2157732726
PR Review Comment: https://git.openjdk.org/jfx/pull/1836#discussion_r2157735148
PR Review Comment: https://git.openjdk.org/jfx/pull/1836#discussion_r2157734090
PR Review Comment: https://git.openjdk.org/jfx/pull/1836#discussion_r2157735700
PR Review Comment: https://git.openjdk.org/jfx/pull/1836#discussion_r2157726326
PR Review Comment: https://git.openjdk.org/jfx/pull/1836#discussion_r2157726799
PR Review Comment: https://git.openjdk.org/jfx/pull/1836#discussion_r2157722231
PR Review Comment: https://git.openjdk.org/jfx/pull/1836#discussion_r2157727769
PR Review Comment: https://git.openjdk.org/jfx/pull/1836#discussion_r2157719923
PR Review Comment: https://git.openjdk.org/jfx/pull/1836#discussion_r2157720058
PR Review Comment: https://git.openjdk.org/jfx/pull/1836#discussion_r2157720342
PR Review Comment: https://git.openjdk.org/jfx/pull/1836#discussion_r2157737328
PR Review Comment: https://git.openjdk.org/jfx/pull/1836#discussion_r2157736712
PR Review Comment: https://git.openjdk.org/jfx/pull/1836#discussion_r2157721837
