<AWT Dev> <Swing Dev> [PATCH] 7168064: SwingUtilities.sharedOwnerFrame multiplies window close event

[Anthony Petrov]

Hi,

I'm getting back to this issue. Please see my comments inline:
On 05/09/2013 02:23 PM, Jose Luis Martin wrote:
> I update the Test and include two new TestCases:
>
> - Test if a Window that has been never shown receive the WINDOW_CLOSED
> event.
>
> - Test if a child window that never has been shown receive the
> WINDOW_CLOSED event on parent dispose().

Please note that in JDK we use jtreg as a test harness, not JUnit. See 
jdk/test/java/awt/ for examples of such tests.


> With the patch applied the second test fails and the doDispose() method
> in the child Window is never called so I suppose that the patch is
> wrong.
>
> Looking at code I don't find any reason to call doDispose() on already
> disposed window or a in a Window that never has been shown. Is there?

As I explained below, the doDispose() calls methods that may be 
overridden in user applications (e.g. Window.hide()). If you stop 
calling this method under certain conditions, you may break those apps 
for no good reason.


> If not, we can simply test if window is displayable on doDispose() but
> then the following code will not fire a WINDOW_CLOSED event.
>
> new JFrame().dispose();
>
> If it's not acceptable, I think that the fix should:
>
> - Fire the event and run the dispose action on never shown Windows.
> - Run the dispose action but not fire the event when re-disposed.
>
> Something like:
>
> doDispose() {
> boolean fireClosedEvent = isDisplayable() || hasBeenNeverShown;
>
> ...
>
> if (fireClosedEvent)
>     postWindowEvent(WindowEvent.WINDOW_CLOSED);
> }

The spec for WINDOW_CLOSED is very vague and reads as:

> The window closed event. This event is delivered after the window has been closed as the result of a call to dispose.

The current implementation is such that the event gets sent 
unconditionally upon every call to dispose(). This doesn't make sense if 
the window has already been disposed previously, or has never allocated 
its native resources (i.e. the peer) actually.

Is this the problem we're trying to resolve?

If so, then the event must only be sent if the removeNotify() call in 
doDispose() actually destroys the peer. In other words, if before the 
call the window has been displayable, and after the call it's no longer 
displayable, then the event must be sent. It shouldn't be sent under 
other circumstances.

Do you see a problem with this solution?

What are the advantages of your proposal?

Why do you think we should send the event in case hasBeenNeverShown == 
true? What would the event mean? Also, please note that 
hasBeenNeverShown is not equal to "native resources have never been 
created", because in fact an app may call window.addNotify() directly 
(which is odd and incorrect, but oh, well..) and this will trigger the 
creation of a native peer. In this case the WINDOW_CLOSED event must be 
sent by the dispose() method. Do you agree?

--
best regards,
Anthony

>
> As Anthony suggested.
>
>
> Best Regards,
>
> -- Jose Luis Martin
>
>
>
> El mié, 08-05-2013 a las 16:29 +0400, Anthony Petrov escribió:
>> Hi,
>>
>> This indeed looks like a bug in AWT. I'm BCC'ing swing-dev@, and CC'ing
>> awt-dev@ instead (please subscribe to this mailing list before posting
>> to it).
>>
>> Here's a question: why do we consider this issue affects child windows
>> only? If you call dispose() on a regular window several times in a row,
>> you'll receive that many WINDOW_CLOSED events. Isn't it the real bug
>> here actually?
>>
>> Note that simply guarding a call to dispose()/disposeImpl() with a check
>> is not the best option because it changes the behavior. If user code
>> overrides certain methods, they may be stopped being called after your
>> fix, and this may break that code.
>>
>> Since the DisposeAction is executed synchronously anyway, perhaps it
>> should set a flag based on which the doDispose() method would decide to
>> send this event or not. What do you think? Also, have you run existing
>> jtreg tests (in jdk/test/java/awt/) to verify that this fix doesn't
>> introduce a regression?
>>
>> PS. I can't find the bug filed by you in our bug database. I'll file a
>> new one once we review your fix on this mailing list.
>>
>> --
>> best regards,
>> Anthony
>>
>> On 05/06/2013 12:28 PM, Jose Luis Martin wrote:
>>> Hello,
>>>
>>> I reported this issue one year ago but seems that it has been removed
>>> from the public bug database and is still not fixed so I try now to
>>> speak directly with you to see if this really is a bug or not.
>>>
>>> The problem that I see is that a child window already disposed is
>>> re-disposed when the owner frame is disposed, multiplying the
>>> WINDOW_CLOSED events.
>>>
>>> I guess that it could be fixed by testing if the child window is
>>> displayable before disposing it in the Window dispose action. (May be
>>> better in the dispose method itself).
>>>
>>> I attach patch over last jdk8 repository and a test case for the issue.
>>>
>>> Thanks,
>>>
>>> Best Regards.
>>>
>>>
>>> -- Jose Luis Martin
>>>
>
>