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
