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 <[email protected]>:
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
