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

[Anthony Petrov]

On 05/21/2013 08:52 PM, Jose Luis Martin wrote:
>> 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.
>
> Yes but, this overriden method will be called multiples times just from
> one user call to dispose() method. As you can see in the test:
>
> window.dispose() -> (*6)-> disposeAction.run() -> (*6) window.hide().
>
> Is this the correct behavior or should be fixed too?

If user code expects the hide() method to be invoked on every invocation 
of the dispose() method, then I don't see why we should change that now. 
Let it be called. This is a long-standing behavior and thus may be 
considered harmless.


>> 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?
>
> No, it's good for me. It's what I expected to happen.
>>
>> What are the advantages of your proposal?
>
> None, I'm only trying too keep the same behavior for  never shown
> windows, (that actually fire the event) but not because I think that it
> should do.
>>
>> 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?
>
> Yes, but not 100%.
>
> I suppose that the fix you are pointing is:
>
> doDispose() {
>     boolean fireClosedEvent = isDisplayable();
>
>     // The following block will run multiplies times from
>     // one user window.dispose() call
>
>     (create and run the dispose action in sync with EDT)
>
>    if (fireClosedEvent)
>       postWindowEvent(WindowEvent.WINDOW_CLOSED);
> }

Yes, this is basically what I'm suggesting.


> I'm not 100% sure that the dispose action is idempotent at all.
>
> It is actually calling:
>
> - hide() -> if overriden should be idempotent.
> - clearCurrentFocusCycleRootOnHide (actually idempotent).
> - removeNotify -> multiplies the HierarchyEvents so HierarchyListeners
> should be idempontent (actually not documented).
>
> Should we replay the same fix on the HirearchyEvent in removeNotify
> method.?

In theory this is the right thing to do. However, I doubt if many 
applications use the HierarchyEvent to handle displayability changes for 
top-level windows. I think that most apps use the WINDOW_CLOSED event 
for this purpose. So unless there's a bug report stating that this 
behavior affects existing apps in a bad way, I wouldn't change this now.


If you and other AWT folks agree with that, could you prepare a patch 
for this change and a jtreg-based regression test (it should be put 
somewhere in test/java/awt/Window/, I think)?

--
best regards,
Anthony


>
>
>
> Best Regards,
>
> -- Jose Luis Martin.
>
>>
>> --
>> 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
>>>>>
>>>
>>>
>
>
>
>