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

Reply via email to