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

[Anthony Petrov]

Thank you very much, Damjan!

I published your latest patch at:

http://cr.openjdk.java.net/~anthony/7-43-GTKFileDialog-6913179.4/

However, it still contains incorrectly formatted lines which makes the 
code unreadable. You might want to explore the Sdiffs in the webrev to 
notice that (eg., UNIXToolkit.java, awt_UNIXToolkit.c, 
sun_awt_X11_GtkFileDialogPeer.c, etc.). Would you or Costantino please 
correct that?

The fix looks fine in overall. Peter might provide some comments after 
reviewing that as well.


PS. The nimbusL&F issue does not seem reproducible locally. Perhaps 
you're using an outdated repository, or your building environment has 
slightly changed.

--
best regards,
Anthony


On 03/11/2010 01:09 PM, Damjan Jovanovic wrote:
> On Wed, Mar 10, 2010 at 11:35 PM, Costantino Cerbo <c.cerbo at gmail.com> wrote:
>> Hallo,
> 
> Hi
> 
>> the only change in this new patch is the include of <string.h> and and
>> <jni_util.h>.
>>
>> The answers for Damjan:
>> 1) Using gdk_threads_enter() and gdk_threads_leave() in
>> Java_sun_awt_X11_GtkFileDialogPeer_quit(..) freezes the dialog on
>> close.
> 
> Sun, your latest awt tree still doesn't compile for me, I had to hack
> it to not call a method that doesn't exist so it does compile:
> 
> diff -r d6d2de6ee2d1
> src/share/classes/javax/swing/plaf/nimbus/NimbusLookAndFeel.java
> --- a/src/share/classes/javax/swing/plaf/nimbus/NimbusLookAndFeel.java
>  Fri Feb 19 15:13:37 2010 -0800
> +++ b/src/share/classes/javax/swing/plaf/nimbus/NimbusLookAndFeel.java
>  Thu Mar 11 11:33:29 2010 +0200
> @@ -288,7 +288,7 @@
>              "JComponent.sizeVariant" == eName) {
> 
>              JComponent c = (JComponent) ev.getSource();
> -            defaults.clearOverridesCache(c);
> +            //defaults.clearOverridesCache(c);
>              return true;
>          }
> 
> Now onto your patch.
> 
> You probably didn't listen to my warning about not calling
> gdk_threads_enter when Java_sun_awt_X11_GtkFileDialogPeer_quit is
> called from handle_response (because GTK itself holds that GDK lock
> during the entire handle_response invocation, and that lock is not
> reentrant). Hence you got the freeze.
> 
> My tests show the way I described does work, and the patch that uses
> it correctly and doesn't freeze is attached.
> 
>> 2) This patch is tested with the last code in the OpenJDK repository
>> and compiles correctly.
>>
>> I think, we all have done a good job and now it's time to put the
>> patch in the OpenJDK.
> 
> With my patch and some heavier testing, I managed to get an assertion failure:
> 
> java: ../../src/xcb_io.c:176: process_responses: Assertion `!(req &&
> current_request && !(((long) (req->sequence) - (long)
> (current_request)) <= 0))' failed.
> Aborted
> 
> I couldn't reproduce it but I'll keep trying.
> 
> There's probably still some place where we're not locking properly.
> 
>> Best Regards,
>> Costantino
> 
> Regards
> Damjan
> 
>> 2010/3/10 Costantino Cerbo <c.cerbo at gmail.com>:
>>> Hello Damjan,
>>>
>>> 2010/3/10 Damjan Jovanovic <damjan.jov at gmail.com>:
>>>> You should probably include the real header files here, like string.h
>>>> and jni_util.h, instead of declaring those functions yourself.
>>> You're right. I will do so.
>>>
>>>
>>>> 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.
>>> I don't know. I'll try later and let you know.
>>>
>>>
>>>> You've still got some C++ comments:
>>>> $ grep '//' b6913179.patch
>>> They are comments in Java classes ;-)
>>>
>>>
>>>> Also the patch doesn't even compile from me, fpulling the awt forest
>>>> to the latest and trying again...
>>> Yesterday I could compile without problems.
>>> When I go home later, I check out the last code in the repository and try again.
>>>
>>>
>>> Thanks for your comments and best regards,
>>> Costantino
>>>