On Mon, 23 Jan 2023 19:24:37 GMT, Thiago Milczarek Sayao <tsa...@openjdk.org>
wrote:
>> Simple PR to remove gtk2 library compilation and loading.
>
> Thiago Milczarek Sayao has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Improve SWT detection
This generally works, except in the case of FXCanvas with an older SWT library.
I tested the three cases where we currently would load the GTK 2 library (and
no longer will). I recommend a somewhat clearer message for cases 1 and 3. Case
2, where the GTK 2 library is already loaded (e.g., by an older SWT library)
now hangs, so that needs to be fixed.
1. Specifying -Djdk.gtk.version=2 as a command line option
I get the expected warning message, although I left a minor wording suggestion
inline.
2. Another native library has already loaded the GTK 2 library (e.g., using
FXCanvas with an older SWT library)
This is now an error case, but isn't detected, and the application hangs in
initGTK trying to load GTK 3. I recommend to detect this case so that an
exception will be thrown. I left suggestions inline.
3. As a fallback if the gtk3 libraries are not available on the system
We get an UnsupportedOperationException, which is ultimately thrown back to the
application (good), but without an informative error message.
...
Caused by: java.lang.UnsupportedOperationException: Internal Error
at
javafx.graphics@21-internal/com.sun.glass.ui.gtk.GtkApplication.lambda$new$4(GtkApplication.java:173)
at
java.base/java.security.AccessController.doPrivileged(AccessController.java:318)
at
javafx.graphics@21-internal/com.sun.glass.ui.gtk.GtkApplication.<init>(GtkApplication.java:160)
...
I recommend changing "Internal Error" to "Unable to load glass GTK library".
modules/javafx.graphics/src/main/java/com/sun/glass/ui/gtk/GtkApplication.java
line 62:
> 60:
> 61: private static final String GTK2_REMOVED_WARNING =
> 62: "WARNING: The JavaFX GTK 2 library was removed.";
Maybe add something like: `, so this option will be ignored` to make it more
clear?
modules/javafx.graphics/src/main/java/com/sun/glass/ui/gtk/GtkApplication.java
line 73:
> 71:
> 72: if (ver != 3) {
> 73: logger.warning("SWT-GTK uses unsupported major GTK
> version "
This needs to be turned into a runtime exception, perhaps
`UnsupportedOperationException`, which is use in other error cases. A quick
check shows that this is not sufficient for older SWT libraries (e.g., SWT 3,
which uses GTK 2), since this system property is not set. Changes will also be
needed in the native code.
modules/javafx.graphics/src/main/native-glass/gtk/launcher.c line 128:
> 126: }
> 127:
> 128: if (!found) {
I think the detection logic for an already existing version of GTK needs to be
enhanced now. There are two cases to consider. First, a GTK library is loaded
that we do support (as of this PR, only GTK 3). Second, a GTK library we don't
support (which would be GTK 2 for now, and maybe we could add GTK 4 in the
future). Something like this could be added above line 128:
if (!found) {
char *** bad_use_chain = gtk_2_lib_options;
for (i = 0; bad_use_chain[i] && !found; i++) {
found = try_libraries_noload(bad_use_chain[i]);
if (found && gtk_versionDebug) {
printf("found already loaded GTK library %s\n",
bad_use_chain[i][1]);
}
}
if (found) {
return -1;
}
}
The above code assumes that you restore the removed `gtk2_versioned` and
`gtk2_not_versioned` arrays and create a `gtk_2_lib_options` that includes them.
tests/system/src/test/java/test/com/sun/glass/ui/gtk/Gtk2Deprecation1Test.java
line 57:
> 55: System.err.println(output);
> 56: assertTrue("Missing warning message", output.contains("WARNING"));
> 57: assertTrue("Missing warning message",
> output.contains("deprecated"));
The modified test would fail before and after the fix. Maybe assert that the
text does _not_ contain "deprecated"?
-------------
Changes requested by kcr (Lead).
PR: https://git.openjdk.org/jfx/pull/999
