Re: [PATCH] fixes to swing button implementation

[Sascha Brawer <brawer at dandelis dot ch>]

graydon hoare <graydon@redhat.com> wrote on Tue, 6 Jan 2004 15:05:53 -0500:

>+  // ChangeEvents are not AWTEvents, can't use multicaster
>+  Vector changeListeners = new Vector ();

Using Vector means a lot of synchronization operations.
java.util.ArrayList/LinkedList are not synchronized, which makes their
usage more efficient. But probably, it would best tp use
javax.swing.event.EventListenerList here (which, by coincidence, became
functional this afternoon...). See the Javadoc in Classpath for how to
use it efficiently. Or, for a more concrete example of its use, have a
look at the method javax.swing.DefaultBoundedRangeModel.fireStateChanged.

A general stylistic question: foo ().bar (baz) or foo().bar(baz)?

AbstractButton.getMargin(): I'm not sure whether or not it should return
a clone of the internal Insets object... A simple Mauve testcase could
check for this.

>Index: javax/swing/DefaultButtonModel.java
>===================================================================
>+    ActionListener actionListener;
>+    ItemListener itemListener;
>+
>+    // ChangeEvents are not AWTEvents
>+    Vector changeListeners = new Vector ();

See above: it probably would be advisable to use
javax.swing.event.EventListenerList instead of Vector.

>+    boolean armed = false;
>+    boolean enabled = true;
>+    boolean pressed = false;
>+    boolean rolledOver = false;
>+    boolean selected = false;
>+    int mnemonic;  
>+    String actionCommand;

Weird. From looking at the J2SE 1.4.2 API specification, I would expect:

  protected int stateMask; // e.g. ARMED | SELECTED
  protected int mnemonic;
  protected String actionCommand;

>Index: javax/swing/plaf/basic/BasicButtonUI.java
>===================================================================

>     public void installUI(final JComponent c) 
>     {
> 	super.installUI(c);
>@@ -68,6 +124,12 @@
> 	pressedBackgroundColor   = new Color(150,150,150);
> 	pressedBackgroundColor   = new Color(150,150,150);
> 	normalBackgroundColor    = new Color(192,192,192);

It seems strange that these colors are hard-coded. Shouldn't they come
from the UIManager?

A final comment: Please, please, please write documentation and Mauve
test cases. With javax.swing.DefaultBoundedRangeModel, I've just made the
experience myself how immensely useful this is for finding bugs, and for
making sure that changes don't break things.

A part from this, IMHO the code look good.

Best regards,

-- Sascha

Sascha Brawer, brawer@dandelis.ch, http://www.dandelis.ch/people/brawer/