<Swing Dev> Review-request for 8145547: JEP 283: [AWT/Swing] Conditional support for GTK 3 on Linux
Phil Race
philip.race at oracle.com
Thu Mar 17 19:57:37 UTC 2016
I ran your changes (which included my fix) through jprt and all Solaris
platforms failed as follows :-
jdk/src/java.desktop/unix/native/libawt_xawt/awt/sun_awt_X11_GtkFileDialogPeer.c", line 37: error: typedef redeclared: GtkWindow (E_TYPEDEF_REDECLARED)
cc: acomp failed for jdk/src/java.desktop/unix/native/libawt_xawt/awt/sun_awt_X11_GtkFileDialogPeer.c
-phil.
On 03/17/2016 09:42 AM, Phil Race wrote:
> This reminded me I had not yet tried the build myself.
> When I did I not get the error that Sergey did but I got this as well :-
>
> jdk9-client/jdk/src/java.desktop/unix/native/libawt_xawt/awt/gtk3_interface.c:
> In function ‘get_integer_property’:
> jdk9-client/jdk/src/java.desktop/unix/native/libawt_xawt/awt/gtk3_interface.c:2542:19:
> error: initialization makes integer from pointer without a cast [-Werror]
> jdk9-client/jdk/src/java.desktop/unix/native/libawt_xawt/awt/gtk3_interface.c:
> In function ‘get_boolean_property’:
> jdk9-client/jdk/src/java.desktop/unix/native/libawt_xawt/awt/gtk3_interface.c:2549:19:
> error: initialization makes integer from pointer without
>
> Both lines it complains about were :
>
> gint intval = NULL;
>
> I changed them to
> gint intval = 0;
>
> I am not using the 'official' compiler, but you really should make
> sure jprt is
> happy before confirming the webrev as final - and also fix all of
> these even if
> the official compiler does not complain.
>
> And I did not notice any problems running SwingSet2 with either of
> GTK2 or GTK3
> specified. It at least ran fine and clearly picked up different versions.
>
>
> -phil.
>
>
> On 03/17/2016 06:25 AM, Semyon Sadetsky wrote:
>> The awt_Desktop.c is missed in the webrev.
>> Please see the updated one:
>> http://cr.openjdk.java.net/~ssadetsky/8145547/webrev.02/
>>
>> --Semyon
>>
>> On 3/17/2016 3:57 PM, Sergey Bylokhov wrote:
>>> On 16.03.16 20:52, Semyon Sadetsky wrote:
>>>> Hi Phil,
>>>>
>>>> Thank for review. You will find my reply below in the text.
>>>>
>>>> The updated webrev is
>>>> http://cr.openjdk.java.net/~ssadetsky/8145547/webrev.01/
>>>
>>> On my system this version fails to build(Ubuntu 15.10 + gcc 5.2.1):
>>>
>>> ERROR: Build failed for target 'images' in configuration
>>> 'linux-x86_64-normal-server-release' (exit code 2)
>>> === Output from failing command(s) repeated here ===
>>> * For target support_native_java.desktop_libawt_xawt_awt_Desktop.o:
>>> /mnt/hgfs/moe/workspaces/jdk/9/client-gate/jdk/src/java.desktop/unix/native/libawt_xawt/xawt/awt_Desktop.c:
>>> In function ‘Java_sun_awt_X11_XDesktopPeer_init’:
>>> /mnt/hgfs/moe/workspaces/jdk/9/client-gate/jdk/src/java.desktop/unix/native/libawt_xawt/xawt/awt_Desktop.c:46:9:
>>> error: implicit declaration of function ‘gtk2_load’
>>> [-Werror=implicit-function-declaration]
>>> if (gtk2_load(env) && gtk2_show_uri_load(env)) {
>>> ^
>>> /mnt/hgfs/moe/workspaces/jdk/9/client-gate/jdk/src/java.desktop/unix/native/libawt_xawt/xawt/awt_Desktop.c:46:27:
>>> error: implicit declaration of function ‘gtk2_show_uri_load’
>>> [-Werror=implicit-function-declaration]
>>> if (gtk2_load(env) && gtk2_show_uri_load(env)) {
>>> ^
>>> cc1: all warnings being treated as errors
>>> === End of repeated output ===
>>> === Make failure sequence repeated here ===
>>> Awt2dLibraries.gmk:369: recipe for target
>>> '/mnt/hgfs/moe/workspaces/jdk/9/client-gate/build/linux-x86_64-normal-server-release/support/native/java.desktop/libawt_xawt/awt_Desktop.o'
>>> failed
>>> make/Main.gmk:175: recipe for target 'java.desktop-libs' failed
>>> === End of repeated output ===
>>>
>>>
>>>> It also contains:
>>>> - new properties jdk.gtk.version and jdk.gtk.verbose
>>>> - appearance tuning for Ubuntu 15 (GTK 3.14). It may require more
>>>> but we
>>>> can do this later as a separate bug.
>>>> The main implementation was done for Ubuntu 14.05 LTS (GTK 3.10) and
>>>> then tuned for OEL 7 (GTK 3.8). Each minor GTK version may have some
>>>> appearance changes.
>>>>
>>>> On 3/15/2016 10:39 PM, Phil Race wrote:
>>>>> There is a lot to read here. I think I need to patch and try it but
>>>>> first ...
>>>>>
>>>>> Two high level questions :
>>>>> 1) Have you verified that this behaves properly (or no worse than
>>>>> currently) with -Djava.awt.headless=true since Swing components
>>>>> are supposed to be able to draw off-screen in headless mode .. and
>>>>> yet a dependency on GTK and its dependency on xlibraries seems to
>>>>> mean
>>>>> that you can't load GTK in this case.
>>>>> BTW I think it may be painful to get them to layout in such a case
>>>>> but that's another issue.
>>>> I tested it by painting to a BufferedImage. Seems it is enough.
>>>>>
>>>>> 2) Have you tried a hi-dpi system ?
>>>> Yes I have. It is identical to the existing GTK2 based appearance.
>>>>>
>>>>> 3) Have you submitted a JPRT job ? It is essential to know that this
>>>>> builds cleanly on the "official" compilation environment.
>>>> I will do this before the push. I think it will be OK because I did
>>>> not
>>>> use any new C constructs and the new libraries are linked dynamically.
>>>>> 4)I expect you ran Swingset2 + GTK L&F but have you run any of the
>>>>> regression test suite ?
>>>> Yes I ran javax/swing tests but many of them fails with GTK2 as well.
>>>> With GTK3 the result was the same except for some unstable tests.
>>>> Unity
>>>> desktop has new window decorations like borderless windows which are
>>>> resized by dragging the outer window shadow, invisible overlay
>>>> scrollbars, etc. Many tests written for old window decorations fails.
>>>>>
>>>>> Minor comments :
>>>>>
>>>>> GTKEngine.java
>>>>>
>>>>> 494 Container parent = context.getComponent().getParent();
>>>>> 495 if(GTKLookAndFeel.is3()) {
>>>>> 496 if (parent != null && parent.getParent() instanceof
>>>>> JComboBox) {
>>>>> 497 if (parent.getParent().hasFocus()) {
>>>>> 498 synthState |= SynthConstants.FOCUSED;
>>>>> 499 }
>>>>> 500 }
>>>>> 501 }
>>>>>
>>>>> GTKPainter.java
>>>>>
>>>>> 746 if (GTKLookAndFeel.is3()) {
>>>>> 747 if (slider.getOrientation() == JSlider.VERTICAL) {
>>>>> 748 y += 1;
>>>>> 749 h -= 2;
>>>>> 750 } else {
>>>>> 751 x += 1;
>>>>> 752 w -= 2;
>>>>> 753 }
>>>>> 754 }
>>>>>
>>>>> I don't know where these numbers come from or what coordinate system
>>>>> is being used here but it seems you are changing them for gtk 2.2 as
>>>>> well as 3
>>>>> Can you speak to this ?
>>>> It is an appearance tuning for GTK3. I didn't change it for GTK2,
>>>> why do
>>>> you think so?
>>>> This was used before my fix as well, for example
>>>>
>>>> if (containerParent instanceof JComboBox) {
>>>> x += (focusSize + 2);
>>>> y += (focusSize + 1);
>>>> w -= (2 * focusSize + 1);
>>>> h -= (2 * focusSize + 2);
>>>> } else {
>>>> x += focusSize;
>>>> y += focusSize;
>>>> w -= 2 * focusSize;
>>>> h -= 2 * focusSize;
>>>> }
>>>>
>>>> The only place where I changed the existing GTK2 appearance is:
>>>>
>>>> 1121 CLASS_SPECIFIC_MAP.put("Slider.thumbWidth",
>>>> "slider-length");
>>>>
>>>> in GTKStyle.java, because this property was omitted by mistake.
>>>>>
>>>>> GTKStyle.java
>>>>>
>>>>> 735 if(!GTKLookAndFeel.is3()) {
>>>>>
>>>>> 840 } else if(GTKLookAndFeel.is3() &&
>>>>> "ComboBox.forceOpaque".equals(key)) {
>>>>>
>>>>>
>>>>> we prefer a space between "if" and "("
>>>> Accepted.
>>>>>
>>>>> sun_awt_X11_GtkFileDialogPeer.c
>>>>>
>>>>> 392 if (gtk->gtk_check_version(2, 8, 0) == NULL) {
>>>>>
>>>>>
>>>>> Maybe I am not looking at the right fn but I thought I saw
>>>>> this fn return a boolean so a check against NULL looks wrong.
>>>> The declaration is in GtkApi struct of gtk_interface.h. It returns
>>>> char*. NULL means that the version is compatible.
>>>>>
>>>>> 393
>>>>> gtk->gtk_file_chooser_set_do_overwrite_confirmation(GTK_FILE_CHOOSER(
>>>>> 394 dialog), TRUE);
>>>>>
>>>>>
>>>>> You didn't add this but it is awfully specific about the GTK
>>>>> version and
>>>>> I wonder if this test is doing what it should be doing on GTK 3?
>>>> Accepted.
>>>>>
>>>>> It is interesting that some equivalent looking Java level dialog
>>>>> checking in XToolkit.java
>>>>> checked for 3.0 too ..
>>>>>
>>>>> swing_GTKEngine.c :
>>>>>
>>>>> 30 /* Static buffer for conversion from java.lang.String to
>>>>> UTF-8 */
>>>>> 31 static char convertionBuffer[CONV_BUFFER_SIZE];
>>>>>
>>>>> So the variable name should be spelt conversionBuffer.
>>>> Accepted.
>>>>>
>>>>> awt_UNIXToolkit.c
>>>>>
>>>>> < 287 free(ret);
>>>>>
>>>>> You deleted this free(). Is that correct ? It seems to imply
>>>>> you now expect a boolean return as discussed above and
>>>>> so in that case NULL looks odd here (line 260) too.
>>>> The JNI exported method returns boolean while the GTK method returns
>>>> char*. free() is deleted intentionally according to the GTK docs it
>>>> belongs to the library and should not be freed by user code.
>>>>>
>>>>> gtk3_interface.h :
>>>>>
>>>>> 36 #define G_PI 3.1415926535897932384626433832795028841971693993751
>>>>>
>>>>> I don't think that is completely accurate :-) And I should have
>>>>> reviewed this yesterday [1].
>>>> :) This is glib's definition I just copied.
>>>>
>>>> --Semyon
>>>>>
>>>>> -phil.
>>>>>
>>>>> [1] http://www.piday.org/
>>>>>
>>>>>
>>>>> On 03/05/2016 01:14 PM, Semyon Sadetsky wrote:
>>>>>> Hello,
>>>>>>
>>>>>> Please review fix for JDK9:
>>>>>>
>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8145547
>>>>>> webrev: http://cr.openjdk.java.net/~ssadetsky/8145547/webrev.00/
>>>>>>
>>>>>> The fix contains GTK3 based implementation for Swing GTK LnF, AWT
>>>>>> FileChooser for Linux and AWT Robot for Linux.
>>>>>> Also the new system property is added to request specific GTK
>>>>>> version
>>>>>> swing.gtk.version.
>>>>>>
>>>>>> --Semyon
>>>>>
>>>>
>>>
>>>
>>
>
More information about the swing-dev
mailing list