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

[Phil Race]

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.

2) Have you tried a hi-dpi system ?

3) Have you submitted a JPRT job ? It is essential to know that this
builds cleanly on the "official" compilation environment.

4)I expect you ran Swingset2 + GTK L&F but have you run any of the 
regression test suite ?

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 ?

GTKStyle.java

735         if(!GTKLookAndFeel.is3()) {

840      } else if(GTKLookAndFeel.is3() && "ComboBox.forceOpaque".equals(key)) {


we prefer a space between "if" and "("

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.

  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?

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.

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.

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

-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