<AWT Dev> [PATCH]: Fix modal dialog (was Problem with modal Dialog)

[Oleg Sukhodolsky]

2009/3/24 Artem Ananiev <Artem.Ananiev at sun.com>:
>
> Roman Kennke wrote:
>>
>> Hi Artem,
>>
>>> Looks fine.
>>
>> Great. Do we have a bug report that I can reference in my commit
>> message? And do I need another review before committing?
>
> I filed a bug some time ago:
>
> 6809233: Modal dialog blocks calling thread after it is hidden and disposed
>
> I hope Oleg will look at your patch soon, and this will be enough to commit
> the change. For all the subsequent bugs, new OpenJDK tools:

The fix looks fine for me. (It is just a little surprise that someone
still waits my reviews for AWT fixes ;)

Oleg.

>
> http://bugs.openjdk.java.net
>
> and
>
> https://cr.openjdk.java.net
>
> should be used.
>
> Thanks,
>
> Artem
>
>> /Roman
>>
>>> Thanks,
>>>
>>> Artem
>>>
>>> Roman Kennke wrote:
>>>>
>>>> So this is the patch I propose to fix this modal dialog problem.
>>>>
>>>> /Roman
>>>>
>>>> Am Donnerstag, den 19.03.2009, 12:27 +0300 schrieb Artem Ananiev:
>>>>>
>>>>> Roman Kennke wrote:
>>>>>>
>>>>>> Hi there,
>>>>>>
>>>>>>>>>>>>>>> BTW, simply sending this over the EQ is no solution either,
>>>>>>>>>>>>>>> because
>>>>>>>>>>>>>>> then
>>>>>>>>>>>>>>> later it will fail to invokeAndWait(). I will think a little
>>>>>>>>>>>>>>> more
>>>>>>>>>>>>>>> about
>>>>>>>>>>>>>>> this, or maybe anybody has a quick idea?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Just an idea (have not evaluated it carefully):
>>>>>>>>>>>>>> perhaps we should set keepBlockingEDT to false not in
>>>>>>>>>>>>>> hideAndDisposeHandler(),
>>>>>>>>>>>>>> but in WakingRunnable.run() instead.
>>>>>>>>>>>>>
>>>>>>>>>>>>> I'm looking at this problem at the moment. The problem is the
>>>>>>>>>>>>> instance
>>>>>>>>>>>>> of WakingRunnable is not run on EDT at all - I don't know why.
>>>>>>>>>>>>
>>>>>>>>>>>> I know why. It is because the DisposeAction is run
>>>>>>>>>>>> _immediately_, before
>>>>>>>>>>>> the WakingRunnable had a chance. This DisposeAction calls
>>>>>>>>>>>> removeNotify(),
>>>>>>>>>>>> which leads to all events on the EQ that are related to the
>>>>>>>>>>>> Dialog beeing
>>>>>>>>>>>> discarded.
>>>>>>>>>>>
>>>>>>>>>>> Could you, please, point to the place where all the events are
>>>>>>>>>>> discarded?
>>>>>>>>>>> I don't see any.
>>>>>>>>>>
>>>>>>>>>> In Component.removeNotify(), we call this:
>>>>>>>>>>
>>>>>>>>>> Toolkit.getEventQueue().removeSourceEvents(this, false);
>>>>>>>>>
>>>>>>>>> OK, I see. What a wonderful code...
>>>>>>>>>
>>>>>>>>>> which removes all events related to the component. removeNotify()
>>>>>>>>>> is
>>>>>>>>>> called from inside the DisposeAction.
>>>>>>>>>
>>>>>>>>> For this particular case it's enough to add some additional check
>>>>>>>>> to
>>>>>>>>> hideAndDisposeHandler:
>>>>>>>>>
>>>>>>>>>        if (showAppContext != curAppContext) {
>>>>>>>>>            // Wake up event dispatch thread on which the dialog was
>>>>>>>>>            // initially shown
>>>>>>>>>            SunToolkit.postEvent(showAppContext, wakingEvent);
>>>>>>>>>            showAppContext = null;
>>>>>>>>> +        } else if (EventQueue.isDispatchThread()) {
>>>>>>>>> +            waking.run();
>>>>>>>>>        } else {
>>>>>>>>>            Toolkit.getEventQueue().postEvent(wakingEvent);
>>>>>>>>>        }
>>>>>>>>
>>>>>>>> this change will fix this particular test, but the same problem may
>>>>>>>> exists even when
>>>>>>>> we call hide() not on EDT, it is just a little bit harder to write
>>>>>>>> test for this ;)
>>>>>>>>
>>>>>>>> What about moving resetting keepBlockingEDT to WakingEvent.run(),
>>>>>>>> or,
>>>>>>>> perhaps, we can simply change
>>>>>>>> target of WakingEvent from a dialog to toolkit.
>>>>>>>> What do you think?
>>>>>>>
>>>>>>> Setting the target of the WakingEvent to the toolkit sounds like an
>>>>>>> elegant and efficient solution to me. Simply executing the event directly
>>>>>>> when on the EDT (as Artem proposed) sounds like crying for more
>>>>>>> side-effects.
>>>>>>
>>>>>> So what should we do about it?
>>>>>
>>>>> I'm fine with changing event's target to Toolkit. Could you send a
>>>>> patch for review then?
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Artem
>>>>>
>>>>>> /Roman
>>>>>>
>