<AWT Dev> Review request: 6913179 (The java.awt.FileDialog should use native GTK file chooser on linux distros)

Damjan Jovanovic damjan.jov at gmail.com
Wed Mar 10 05:05:45 PST 2010

Hi Costantino

+JNU_GetEnv(JavaVM *vm, jint version);
+JNU_ThrowInternalError(JNIEnv *env, const char *msg);
+char * strrchr ( const char *, int );

You should probably include the real header files here, like string.h
and jni_util.h, instead of declaring those functions yourself.

+JNIEXPORT void JNICALL Java_sun_awt_X11_GtkFileDialogPeer_quit
+(JNIEnv * env, jobject jpeer)
+       if (dialog != NULL)
+       {
+               fp_gtk_widget_hide (dialog);
+               fp_gtk_widget_destroy (dialog);
+               fp_gtk_main_quit ();
+               dialog = NULL;
+       }

Should we put the GDK lock statements around those 3 functions? But if
we should, make sure they aren't taken recursively: when called from
Java the GDK lock isn't taken and we must, but when called from the
handle_response callback function, GTK already holds that lock and we
must not.

You've still got some C++ comments:
$ grep '//' b6913179.patch
+            // no filter, accept all.
+        // We do not implement this method because we
+        // have delegated to FileDialog#setDirectory
+        // We do not implement this method because we
+        // have delegated to FileDialog#setFile
+        // We do not implement this method because we
+        // have delegated to FileDialog#setFilenameFilter
+        // The current GtkFileChooser is available from GTK+ 2.4
+    		// The current GtkFileChooser is available from GTK+ 2.4
+    	// Init the thread system to use GLib in a thread-safe mode
+    		//According the GTK documentation, gdk_threads_init() should be
+    		//called before gtk_init() or gtk_init_check()

Also the patch doesn't even compile from me, fpulling the awt forest
to the latest and trying again...

But - lol - even the latest unpatched awt forest no longer compiles:

# Running javac:
/home/damjan/.local/opt/jdk6/bin/java -XX:-PrintVMOptions
-XX:+UnlockDiagnosticVMOptions -XX:-LogVMOutput -client -Xmx896m
-Xms128m -XX:PermSize=32m -XX:MaxPermSize=160m
-jar /openjdk7/awt/build/linux-i586/langtools/dist/bootstrap/lib/javac.jar
-source 7 -target 7 -encoding ascii
-Xbootclasspath:/openjdk7/awt/build/linux-i586/classes -sourcepath
-d /openjdk7/awt/build/linux-i586/classes
cannot find symbol
  symbol:   method clearOverridesCache(JComponent)
  location: class NimbusDefaults
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.
1 error
make[5]: *** [.compile.classlist] Error 1
make[5]: Leaving directory `/openjdk7/awt/jdk/make/javax/swing/plaf'
make[4]: *** [build] Error 1
make: *** [build_product_image] Error 2

Uhm, Sun?

Otherwise good work Costantino, thank you and good luck

On Wed, Mar 10, 2010 at 1:17 AM, Costantino Cerbo <c.cerbo at gmail.com> wrote:
> Hi Peter, Anthony and Damjan,
> here is a new patch (the 4th one!) with these improvements:
>  * The copyright in GtkFileDialogPeer is now correctly dated.
>  * ANSI C conformity 1: // comments transformed to /* */
>  * ANSI C conformity 2: Variable declaration always at the beginning of a block
>  * All compiler warnings removed
>  * Error checking (in case of failure, JNU_ThrowInternalError(..) is used)
>  * Multiple selection mode set only in an OPEN action (otherwise Gtk
> throws a warning when we are saving a file)
> As also Peter said, the job is almost done and I hope to see this
> patch soon in the OpenJDK!
> Best Regards,
> Costantino
> P.S.: About the indentation, I have no idea why it isn't right.
> I set it in my eclipse workspace according the Anthony's suggestions:
>  Tab policy: Spaces only
>  Indentation size: 4
>  Tab site: 4
>  Line width: 80
> 2010/3/2 Peter Zhelezniakov <Peter.Zhelezniakov at sun.com>:
>> Hi Costantino,
>> Seems almost done! Changes to GTK code seem right thing to do indeed.
>> I'd agree that we'd better stay away from any C++-isms such as // and
>> declaring variables in the middle of a block. This code is built on a
>> variety of platforms and we don't want to break older ones without a strong
>> reason.
>> Thanks!
>> --
>> Peter              |  x33066  |  location: SPB04  |  timezone: GMT+03

More information about the awt-dev mailing list