Hi Alexander,
Based on your comments last time, I refined my patch of v6 and
offered v7 @
http://cr.openjdk.java.net/~dingxmin/7189299/webrev.07/
v7 preserves AbstractButton$Handler listener in the model and only
makes sure only stale listeners are removed.
Your questions and concerns are answered below one by one
1. Could you give an example there this check is useful?
Answer: Ops, it's a mistake and the check is, as you pointed out,
useless. It has been removed in v7.
2. Could you preserve the listeners location and just remove old
listeners from the model?
Answer: done in v7.
3. The fix sets the model to the JButtonWrapper first and than
removes unnecessary listeners from the model. Is it possible to
rearrange order of these operations?
Answer: Excellent comment. Done in v7.
4. Could you add the check that the action listener is invoked only
once after a component tree updating and the action does the same that
it does before a component tree updating?
Answer: I am afraid I could not make it in the auto test
(bug7189299.java) but I can achieve it to some degree in manual test
FormSubmit, the one you illustrated below.
My idea of checking it in FormSubmit.java is subclassing JEditorPane
and overriding 'public EditorKit getEditorKit()' where stack traces can
be examined in the overridden method to make sure FormView.submitData
occurs only once. This approach works because FormView.submitData()
calls JEditorPane.getEditorKit but is tricky. However, it's the only
way I can think of without any additional framework support. If you are
in favor of adding the check in FormSubmit.java, I am willing to do that.
Thanks again for all your comments and your support. Once again, if
you have any further concern or comment, please don't hesitate to let me
know.
Best regards,
Frank
On 11/1/2012 8:08 PM, Alexander Scherbatiy wrote:
On 11/1/2012 10:35 AM, Frank Ding wrote:
Hi Alexandr,
Thanks for your review. The problem that the JButton does not
release itself as you mentioned is, after taking closer look at
relevant code again, that I removed instance AbstractButton.Handler
as change listener where repainting the button press animation is
defined. So this time I get around this issue and here comes v6 patch @
http://cr.openjdk.java.net/~dingxmin/7189299/webrev.06/
Below are answers to what you have raised in previous email.
1. if ( if (!listener.equals(this)) { that 'this' means the
JButtonWrapper instead of an action listener.
'this' refers to JButtonWrapper.this because the closest class
definition is JButtonWrapper. I also verified it during my debugging.
Could you give an example there this check is useful? It seems
that the JButtonWrapper does not implement ActionListener interface
and so the listener and the this always should not be equal.
I dumped action listeners from the button and model before the fix
and after it.
Before the fix the button has the latest FormView listener and the
model has AbstractButton$Handler added each time after a component
input creation.
After the fix both the button and the model have the FormView
listener and the button does not have the AbstractButton$Handler.
Could you preserve the listeners location and just remove old
listeners from the model?
The fix sets the model to the JButtonWrapper first and than removes
unneccessary listeners from the model. Is it possible to rearrange
order of these operations?
I see that you check number of the listeners in the test. Could you
add the check that the action listener is invoked only once after a
component tree updating and
the action does the same that it does before a component tree
updating.
Thanks,
Alexandr.
2. Why are two FormViews created in this sample?
SwingUtilities.updateComponentTreeUI() triggers html View hierarchy
to update by instantiating new UI instances involved in the html
form. It could have been avoided to have 2 View instances but we are
restricted to current design that LF change causes creation of View
instance which further leads to creation of new UI instances.
Creation of new UI instances upon LF change can be referenced in
BasicTextUI.modelChanged().
3. Why are there 2 listeners added to the button model in this sample?
The issue is caused by combination of the 2 factors. First, a shared
ButtonModel is created per html Element. Second, JButton.setModel()
automatically registers its private Handler in the new model but no
way to deregister it. The first is a design choice but the second is
a design flaw. Consequence is that the shared ButtonModel instance
bears more and more JButton.Handler.
I think v6 patch is promising as a final fix. If you have any
further concern or comment, please don't hesitate to let me know.
Thanks again for your careful review.
Best regards,
Frank
On 10/30/2012 9:38 PM, Alexander Scherbatiy wrote:
Hi Frank,
I tried your V5 patch and run the test described in the issue (see
below).
I pressed the button and it does not release. This is because the
AbstractButton.Handler listener is removed because of the fix.
You could check the line
941 if (!listener.equals(this)) {
that 'this' means the JButtonWrapper instead of an action listener.
Could you also look deeper in the code and say why two FormViews are
created (and each adds it's own listener to the button model) in
this sample?
-----------------------------------------------
import java.awt.*;
import javax.swing.*;
public class FormSubmit {
public static void main(String[] args) throws Exception {
SwingUtilities.invokeAndWait(new Runnable() {
@Override
public void run() {
JEditorPane html = new JEditorPane("text/html",
"<html><body><FORM ACTION=\"examplescript.cgi\"><INPUT
type=\"submit\" value=\"Submit\"></FORM></BODY></HTML>");
JFrame frame = new JFrame()
frame.setLayout(new BorderLayout());
frame.add(html, BorderLayout.CENTER);
frame.setSize(200, 100);
frame.setDefaultCloseOperation(frame.EXIT_ON_CLOSE);
frame.setVisible(true); // Uncomment this line to
see the Exception on JDK 7
SwingUtilities.updateComponentTreeUI(html);
}
});
}
}
-----------------------------------------------
Thanks,
Alexandr.
