<AWT Dev> Bug 6913179, FileDialog uses native GTK under Linux: first patch
Anthony.Petrov at Sun.COM
Mon Jan 25 05:30:34 PST 2010
Good job. The webrev for that patch is available at:
Just a couple of minor comments:
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!
> 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.
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):
This way your source files will always conform to the code conventions
used across the OpenJDK code.
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,
> * Callbacks methods IDs cached in static variables
> (filenameFilterCallbackMethodID, setFileInternalMethodID)
> * Check if GTK+ >= 2.4 before invoking gtk_file_chooser_load() or in
> 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,
More information about the awt-dev