<Swing Dev> Review-request for 8145547: JEP 283: [AWT/Swing] Conditional support for GTK 3 on Linux

Thu Mar 17 12:57:02 UTC 2016

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
>>
>

-- 
Best regards, Sergey.