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

[Anthony Petrov]

Hi Constantino,

Here's the continuation of my review notes.

On a general note, could you please follow the code conventions [1] that 
are recommended for the OpenJDK [2]? Especially, the indentation style 
(e.g. see the gtk_interface.c gtk_file_chooser_load()) and line-length 
(e.g. see your modifications to the XToolkit.java). Well, the code 
guidelines mainly cover the Java code, but most rules apply to C/C++ 
native code as well.

src/solaris/native/sun/awt/gtk_file_chooser_interface.h
I don't think we need to separate the interfaces of general gtk APIs and 
the file chooser API. Let's keep both in one file (gtk_interface.h). 
That would simplify a possible rework of gtk-usage implementation in jdk 
in the future.


src/solaris/native/sun/awt/sun_awt_X11_GtkFileDialogPeer.c
>    7 static JavaVM *java_vm;

That would be great if you rename the variable to jvm - this is a common 
convention for (at least) awt-related native code.


>    9 union env_union {
>   10         void *void_env;
>   11         JNIEnv *jni_env;
>   12 };
>   13 
>   14 JNIEnv *env() {
>   15         union env_union tmp;
>   16         (*java_vm)->GetEnv(java_vm, &tmp.void_env, JNI_VERSION_1_2) == JNI_OK;
>   17         return tmp.jni_env;
>   18 }

I wonder why do you need such sort of a trick for that? Wouldn't that be 
a bit less cryptic:

JNIEnv *env = (JNIEnv *)JNU_GetEnv(jvm, JNI_VERSION_1_2);

? :)
Just put this line at the beginning of each method that needs the env 
and use that instead of calling the env() every time.


>   20 static gboolean filenameFilterCallback(const GtkFileFilterInfo *filter_info,
>   22         jclass cx = (*env())->GetObjectClass(env(), (jobject) obj);
>   24         jmethodID id = (*env())->GetMethodID(env(), cx, "filenameFilterCallback",

I guess that the callback is going to be called quite often. Could we 
cache the method id in a static variable? For an example please refer to 
solaris\native\sun\xawt\XToolkit.c : get_xawt_root_shell(JNIEnv *env) 
function. The same pattern could be used in the handle_response() as well.


>  113         if (fp_gtk_check_version(2, 8, 0) == NULL) {
>  114                 fp_gtk_file_chooser_set_do_overwrite_confirmation (GTK_FILE_CHOOSER(dialog), TRUE);

Shouldn't we also guard the dl_symbol() call at gtk_file_chooser_load() 
with the same condition?

The rest looks quite good to me. Great job, Constantino! Thanks!

[1] http://java.sun.com/docs/codeconv/index.html
[2] http://openjdk.java.net/guide/

--
best regards,
Anthony

On 1/20/2010 7:53 PM Anthony Petrov wrote:
> Hi Constantino,
> 
> Thank you very much for the patch! I've generated a webrev for it at:
> 
> http://cr.openjdk.java.net/~anthony/7-43-GTKFileDialog-6913179.0/
> 
> I'm also CC'ing Peter as he'll be a reviewer for the fix as well.
> 
> Some comments from my side follow:
> 
> src/solaris/classes/sun/awt/X11/GtkFileDialogPeer.java
>> 15 class GtkFileDialogPeer extends XFileDialogPeer {
> 
> We don't really use much from the XFileDisalogPeer, do we need this 
> class to be its descendant? Why not extend the XDialogPeer and implement 
> the java.awt.peer.FileDialogPeer interface directly?
> 
> Some methods in that class (like start(),filenameFilterCallback, etc.) 
> may safely be made private, I think. The less a class exports, the better.
> 
> 
>>   55     public void setVisible(boolean b) {
>>   56         if (b) {
> 
> And what if b == false? I can imagine calling that from another thread.
> 
> 
> On 1/18/2010 1:14 AM Costantino Cerbo wrote:
>> 1) The FileDialog isn't disposed. I cannot understand why, because in
>> 'sun_awt_X11_GtkFileDialogPeer.c#handle_response' I call:
>>     fp_gtk_widget_hide(dialog);
>>     fp_gtk_widget_destroy(dialog);
>>     fp_gtk_main_quit();
> 
> That must be the result of inheriting from XFileDialog: see 
> XFileDialog.setVisible() for some tips.
> 
> 
>> 2) The shortcut "Search" on the left up side is missing because we
>> should initialize the thread system in GLib by calling:
>>
>>   if (!g_thread_supported ()) {
>>       g_thread_init (NULL);
>>   }
>>
>> but I wasn't able to find a way to dynamically load these functions
>> (they are linked with gthread-2.0).
> 
> I haven't yet reviewed the native part of the code (will finish 
> tomorrow), but it looks like we could just load this library along with 
> the gtk-2.0, couldn't we?
> 
> 
>> Last but not least, I've some problems with the incremental make.
>> When I make a full build, the output goes to
>>      ~/openjdk/jdk7/mytl/build/linux-i586
>> while with and incremental make in a subfolder, the result is in
>>     ~/openjdk/jdk7/mytl/jdk/build/linux-i586
> 
> [...]
> 
>> That means that I have to make each time a full build, and you can
>> imagine how time consuming is this!
> 
> On linux? Mine takes about 10 minutes. However, even that is too much, 
> that's why incremental building is a must. The secret is as follows:
> 
> I suppose that you're cloning the full forest with all the subtrees (tl, 
> build, etc. etc.), but what you really need is just the jdk repository. 
> Get it alone (remember to use clone instead of fclone when cloning a 
> subtree), set the ALT_JDK_IMPORT_PATH as recommended in the README-build 
> documents, and you're ready to build as quickly as possible. And the 
> incremental builds will start working properly then.
> 
> -- 
> best regards,
> Anthony
>