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
