<AWT Dev> Bug 6913179, FileDialog uses native GTK under Linux: first patch

[Anthony Petrov]

Hi Constantino,

Good job. The webrev for that patch is available at:

http://cr.openjdk.java.net/~anthony/7-43-GTKFileDialog-6913179.1/

Just a couple of minor comments:

1. src/solaris/classes/sun/awt/UNIXToolkit.java

    a) Even though the new method is actually native, we would still 
like to see its name formed according to the Java coding conventions: 
native_gtk_check_version() -> nativeGtkCheckVersion(). Also, usually we 
don't use the word 'native' in such methods, but instead add the 'Impl' 
  suffix (an abbreviation from 'implementation'), i.e gtkCheckVersionImpl().

    b) Code formatting: too many spaces for the indentation of the 
checkGtkVersion() method and its specification. BTW, very nice 
specification! I like it!

2. src/solaris/classes/sun/awt/X11/GtkFileDialogPeer.java
>   86                 fd.setVisible(false);
>   89                 fd.setVisible(false);

I'm not really sure if you need these calls. Also, I think that the 
second one could cause an endless loop if it weren't guarded on the 
shared level in Component.show(boolean). I think calling run() or quit() 
should be enough for it to function correctly.

3. src/solaris/classes/sun/awt/X11/XToolkit.java
Indentation: too many spaces again.

4. I haven't reviewed the native code precisely, I think Peter is going 
to review that, but again: you're using too many spaces for indentation. 
I suggest you configure you text editor so that it performs TAB 
expansion to spaces, uses 4 spaces for TABs, and uses 4 spaces for 
indentation. E.g. the following commands do that to my vi (in ~/.vimrc):

set tabstop=4
set shiftwidth=4
set expandtab

This way your source files will always conform to the code conventions 
used across the OpenJDK code.

Thanks!

--
best regards,
Anthony

On 1/24/2010 10:26 PM Costantino Cerbo wrote:
> Hello Anthony,
> 
> 2010/1/22 Anthony Petrov <Anthony.Petrov at sun.com>:
>> Since you're working on an AWT fix, I suggest using the awt repository. The
>> following command does the job for me:
>>
>> $ hg clone http://hg.openjdk.java.net/jdk7/awt/jdk
> 
> Yes! After cloning the awt repository (instead of the tl) I could
> finally make an incremental build with success!
> Maybe somebody should document it a little bit better on the OpenJdk
> website. Anyway thanks a lot!
> 
> In this new patch I put into practice your previous comments:
>   * GtkFileDialogPeer extends XDialogPeer and implements
> FileDialogPeer instead of extending XFileDialogPeer
>   * Limited visibility to private when necessary
>   * setVisible(false) hide the dialog if called in different thread.
>   * Code formatted according C and Sun java convention (with line
> width 80 columns)
>   * java_vm renamed to jvm
>   * Function env() replaced by JNIEnv *env = (JNIEnv *)JNU_GetEnv(jvm,
> JNI_VERSION_1_2);
>   * Callbacks methods IDs cached in static variables
> (filenameFilterCallbackMethodID, setFileInternalMethodID)
>   * Check if GTK+ >= 2.4 before invoking gtk_file_chooser_load() or in
> XToolkit#createFileDialog(.)
> 
> Then I solved also the remaining problems:
>   * The dialog peer is disposed on close (I should also dispose the
> original FileDialog)
>   * The shortcut "Search" is available: I dynamically load also the
> library "libgthread-2.0.so"
> 
> I also added to the UNIXToolkit a method to check the GTK+ Version.
> It's used to enable the new FileDialog only when GTK+ >= 2.4 and it
> may be also useful for the GTK L&F (for example from GTK 2.18 the most
> buttons have no icon).
> 
> I've extensively manually tested the new FileDialog and haven't found
> any bug: I think this patch is quit "production-ready".
> 
> I'm looking forward to hearing you feedback!
> 
> Best Regards,
> Costantino