<Swing Dev> Review-request for 8145547: JEP 283: [AWT/Swing] Conditional support for GTK 3 on Linux
Alexander Scherbatiy
alexandr.scherbatiy at oracle.com
Wed Apr 6 07:32:31 UTC 2016
The fix looks good to me.
Thanks,
Alexandr.
On 4/5/2016 8:04 PM, Semyon Sadetsky wrote:
> The fix is updated because JEP 272 was pushed. JEP 272 has dependency
> on GTK.
>
> http://cr.openjdk.java.net/~ssadetsky/8145547/webrev.04/
>
> --Semyon
>
> On 3/18/2016 4:02 PM, Semyon Sadetsky wrote:
>>
>>
>> On 3/17/2016 10:57 PM, Phil Race wrote:
>>> 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
>>>
>> I have fixed the Solaris build:
>> http://cr.openjdk.java.net/~ssadetsky/8145547/webrev.03/.
>>
>> > gint intval = NULL;
>> I think this happens with you specific compiler only. Old
>> gtk2_interface.c always contained the same line.
>>
>> I ran JPRT with webrev.03. The URL is:
>> http://sthjprt.uk.oracle.com/archives/2016/03/2016-03-18-070745.ssadetsk.client/
>>
>> It seem macosx_x64_10.9-product-c2-jdk_math build failed because of
>> another change. (?) Could you help me to interpret the JPRT job status?
>>
>> --Semyon
>>>
>>> -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