United StatesChange Country, Oracle Worldwide Web Sites Communities I am a... I want to...
Bug ID: JDK-4457940 subclasses of AbstractButton don't handle Actions correctly
JDK-4457940 : subclasses of AbstractButton don't handle Actions correctly

Details
Type:
Bug
Submit Date:
2001-05-12
Status:
Resolved
Updated Date:
2001-06-22
Project Name:
JDK
Resolved Date:
2001-06-22
Component:
client-libs
OS:
generic
Sub-Component:
javax.swing
CPU:
generic
Priority:
P2
Resolution:
Fixed
Affected Versions:
1.3.0
Fixed Versions:
1.4.0 (beta2)

Related Reports
Duplicate:
Relates:

Sub Tasks

Description

Name: yyT116575			Date: 05/11/2001


The bugs described are platform-independent.

java version "1.3.0"
Java(TM) 2 Runtime Environment, Standard Edition (build 1.3.0-C)
Java HotSpot(TM) Client VM (build 1.3.0-C, mixed mode)


This bug report tries to identify the root cause of several bugs: 4281558,
4381361, 4347339, 4304129, 4391622, 4284401, 4396953, 4409290, 4106486, 4133256,
4304129, 4138015, 4284962, 4266524.
Some of these have been closed as duplicates. Some of these have been fixed.
Some of these have been marked "fixed" even though problems remain. The point
I'm trying to make is this: These problems are vexing enough that more than a
dozen people went to the trouble of submitting a bug report. Even though
work-arounds exist, these problems deserve to be fixed properly.

Here's what I've concluded about the cause of these problems.

Primary cause: The subclasses of AbstractButton are not taking advantage of
inheritance when handling Actions. When they override the
configurePropertiesFromAction() method, they take it upon themselves to
incorporate all the properties specified in the Action. While this is ok in
theory, it has resulted in subclasses remaining broken after the superclass has
been fixed, and other similar problems.

Secondary cause: The implementation of configurePropertiesFromAction() in class
AbstractButton has some simple flaws. These should be easy to fix.
  (flaw1) It doesn't honor ACTION_COMMAND_KEY.
  (flaw2) It doesn't honor ACCELERATOR_KEY.
  (flaw3) If no MNEMONIC_KEY is specified in the Action, it leaves the previous
mnemonic in effect instead of removing it.


  Fixing the Primary cause is less easy, but doable. For example, JCheckBox and
JRadioButton need to override configurePropertiesFromAction() because they can't
allow the SMALL_ICON specified in an Action to clobber their internal 'icon' and
'selectedIcon' properties (which by default are handled by the LnF).
But it shouldn't have to mess with NAME, ACTION_COMMAND_KEY, SHORT_DESCRIPTION,
MNEMONIC_KEY, or ACCELERATOR_KEY--that's what superclasses are for.
Their configurePropertiesFromAction() method should look something like this:

protected void configurePropertiesFromAction(Action a) {
    // define inner class
    class ActionWrapper implements Action {
        private Action w;
        public ActionWrapper(Action w) { this.w = w; }
        public Object getValue(String k) { return w.getValue(k); }
        public void putValue(String k, Object v) { w.putValue(k, v); }
        public boolean isEnabled() { return w.isEnabled(); }
        public void setEnabled(boolean b) { w.setEnabled(b); }
        public void addPropertyChangeListener(PropertyChangeListener l) {
            w.addPropertyChangeListener(l); }
        public void removePropertyChangeListener(PropertyChangeListener l) {
            w.removePropertyChangeListener(l); }
        public void actionPerformed(ActionEvent e) { w.actionPerformed(e); }
    }
    // create a new Action just like 'a' except for its Icon
    final Icon myIcon = getIcon();
    Action myIconAction = new ActionWrapper(a) {
        public Object getValue(String k) {
            if (Action.SMALL_ICON.equals(k)) return myIcon;
            return super.getValue(k);
        }
    };
    // invoke parent's method on the altered Action
    super.configurePropertiesFromAction(myIconAction);
}

Similarly, if JMenuItem is documented to not support toolTipText, it should do
the same kind of thing to mask out SHORT_DESCRIPTION without messing with the
other five properties.
Also, the button created by JToolBar.add(Action a) in
createActionComponent(Action a) should do the same kind of thing to mask out
NAME. (See bug 4106486, which is marked as fixed in swing1.0fcs even though it
is still broken in 1.3.)

Note that, while I do suggest this general approach, I'm not suggesting that
this code be used verbatim. At the very least, the ActionWrapper class should be
moved outside the body of the method, preferably to someplace where it can be
used by programmers who wish to subclass the existing button types.


I didn't provide any example programs that exhibit the bugs that need to be
fixed. Frankly, there are so many individual bugs with the way the various
button classes handle Actions that it would be unwieldy to demonstrate--or even
list--them all. (seven button classes, each with a setAction() method, an Action
constructor, and Action-related PropertyChangeListener code)
On request I can supply several small programs that exhibit one or two bugs
each. And there are several example programs in the other bug reports I
mentioned.


One final note: Anyone planning to fix this should also take a look at
AbstractButton.ButtonActionPropertyChangeListener and any other subclasses of
AbstractActionPropertyChangeListener (anonymous or otherwise) that subclasses of
AbstractButton create. These things maybe need the same kind of help.
(Review ID: 124245) 
======================================================================

                                    

Comments
EVALUATION



Name: pzR10082			Date: 06/05/2001


The 'primary cause' has been already fixed as part of fix for
bug 4383709. The same fix fixed flaw3 of the 'secondary cause'.
ACCELERATOR_KEY is to be used by menu items only. Thus we just
have to fix ACTION_COMMAND_KEY. It should be added to the list
of properties configured by default, in
AbstractButton.configurePropertiesFromAction(Action, String[]).

###@###.### 2001-06-05

======================================================================
                                     
2001-06-05
SUGGESTED FIX



Name: pzR10082			Date: 06/05/2001


*** /tmp/geta24446	Tue Jun  5 12:26:51 2001
--- AbstractButton.java	Tue Jun  5 12:23:43 2001
***************
*** 1015,1021 ****
       * <code>Action</code> instance.  The properties 
       * which are set may differ for subclasses.  By default,
       * the properties which get set are <code>Text, Icon,
!      * Enabled, ToolTipText</code> and <code>Mnemonic</code>.
       * <p>
       * If the <code>Action</code> passed in is <code>null</code>, 
       * the following things will occur:
--- 1015,1021 ----
       * <code>Action</code> instance.  The properties 
       * which are set may differ for subclasses.  By default,
       * the properties which get set are <code>Text, Icon,
!      * Enabled, ToolTipText, ActionCommand</code>, and <code>Mnemonic</code>.
       * <p>
       * If the <code>Action</code> passed in is <code>null</code>, 
       * the following things will occur:
***************
*** 1050,1055 ****
--- 1050,1057 ----
       *     from the <code>Action</code>,
       * <li><code>Action.MNEMONIC</code> - set the <code>Mnemonic</code>
       *     property from the <code>Action</code>,
+      * <li><code>Action.ACTION_COMMAND_KEY</code> - set the
+      *     <code>ActionCommand</code> property from the <code>Action</code>,
       * <li><code>"enabled"</code> - set <code>Enabled</code> property
       *     from the <code>Action</code>
       * </ul>
***************
*** 1077,1083 ****
          if (types == null) {
              String[] alltypes = { Action.MNEMONIC_KEY, Action.NAME,
                                    Action.SHORT_DESCRIPTION, Action.SMALL_ICON,
!                                   "enabled" };
              types = alltypes;
          }
          for (int i=0; i<types.length; i++) {
--- 1079,1085 ----
          if (types == null) {
              String[] alltypes = { Action.MNEMONIC_KEY, Action.NAME,
                                    Action.SHORT_DESCRIPTION, Action.SMALL_ICON,
!                                   Action.ACTION_COMMAND_KEY, "enabled" };
              types = alltypes;
          }
          for (int i=0; i<types.length; i++) {
***************
*** 1093,1098 ****
--- 1095,1102 ----
                  setToolTipText(a!=null ? (String)a.getValue(type) : null);
              } else if (type.equals(Action.SMALL_ICON)) {
                  setIcon(a!=null ? (Icon)a.getValue(type) : null);
+             } else if (type.equals(Action.ACTION_COMMAND_KEY)) {
+                 setActionCommand(a!=null? (String)a.getValue(type) : null);
              } else if (type.equals("enabled")) {
                  setEnabled(a!=null ? a.isEnabled() : true);
              }
***************
*** 1159,1165 ****
  			button.setMnemonic(mn.intValue());
  			button.invalidate();
  			button.repaint();
! 		    } 
  		}
  	}
      }
--- 1163,1171 ----
  			button.setMnemonic(mn.intValue());
  			button.invalidate();
  			button.repaint();
! 		    } else if (e.getPropertyName().equals(Action.ACTION_COMMAND_KEY)) {
!                         button.setActionCommand((String)e.getNewValue());
!                     }
  		}
  	}
      }

------- JButton.java -------
*** /tmp/sccs.QCHsW7	Tue May 22 19:31:18 2001
--- JButton.java	Tue May 22 19:07:12 2001
***************
*** 216,222 ****
       * according to values from the <code>Action</code> instance. 
       * The properties which get set may differ for <code>AbstractButton</code>
       * subclasses.  By default, the properties which get set are
!      * <code>Text, Icon, Enabled, ToolTipText</code>, and
       * <code>Mnemonic</code>.
       *
       * @param a the <code>Action</code> from which to get the
--- 216,222 ----
       * according to values from the <code>Action</code> instance. 
       * The properties which get set may differ for <code>AbstractButton</code>
       * subclasses.  By default, the properties which get set are
!      * <code>Text, Icon, Enabled, ToolTipText, ActionCommand</code>, and
       * <code>Mnemonic</code>.
       *
       * @param a the <code>Action</code> from which to get the
***************
*** 230,236 ****
          // Action.NAME differently from AbstractButton, so we don't call
          // super.configurePropertiesFromAction(a) here.
          String[] types = { Action.MNEMONIC_KEY, Action.SHORT_DESCRIPTION,
!                            Action.SMALL_ICON, "enabled" };
          configurePropertiesFromAction(a, types);
  
  	Boolean hide = (Boolean)getClientProperty("hideActionText");
--- 230,237 ----
          // Action.NAME differently from AbstractButton, so we don't call
          // super.configurePropertiesFromAction(a) here.
          String[] types = { Action.MNEMONIC_KEY, Action.SHORT_DESCRIPTION,
!                            Action.SMALL_ICON, Action.ACTION_COMMAND_KEY,
!                            "enabled" };
          configurePropertiesFromAction(a, types);
  
  	Boolean hide = (Boolean)getClientProperty("hideActionText");
*** /tmp/geta24451	Tue Jun  5 12:27:09 2001
--- JCheckBox.java	Tue Jun  5 12:24:00 2001
***************
*** 220,230 ****
  
  
      /**
!      * Factory method which sets the ActionEvent source's properties
!      * according to values from the Action instance.  The properties 
!      * which are set may differ for subclasses.
       * By default, the properties which get set are <code>Text, Mnemonic,
!      * Enabled</code>, and <code>ToolTipText</code>.
       *
       * @param a the Action from which to get the properties, or null
       * @since 1.3
--- 220,230 ----
  
  
      /**
!      * Factory method which sets the <code>ActionEvent</code> source's
!      * properties according to values from the Action instance. The
!      * properties which are set may differ for subclasses.
       * By default, the properties which get set are <code>Text, Mnemonic,
!      * Enabled, ActionCommand</code>, and <code>ToolTipText</code>.
       *
       * @param a the Action from which to get the properties, or null
       * @since 1.3
***************
*** 233,239 ****
       */
      protected void configurePropertiesFromAction(Action a) {
          String[] types = { Action.MNEMONIC_KEY, Action.NAME,
!                            Action.SHORT_DESCRIPTION, "enabled" };
          configurePropertiesFromAction(a, types);
      }
  
--- 233,240 ----
       */
      protected void configurePropertiesFromAction(Action a) {
          String[] types = { Action.MNEMONIC_KEY, Action.NAME,
!                            Action.SHORT_DESCRIPTION,
!                            Action.ACTION_COMMAND_KEY, "enabled" };
          configurePropertiesFromAction(a, types);
      }
  
***************
*** 263,279 ****
  		    Action action = (Action)e.getSource();
  		    action.removePropertyChangeListener(this);
  		} else {
! 		    if (e.getPropertyName().equals(Action.NAME)) {
  			String text = (String) e.getNewValue();
  			button.setText(text);
  			button.repaint();
! 		    } else if (e.getPropertyName().equals(Action.SHORT_DESCRIPTION)) {
  			String text = (String) e.getNewValue();
  			button.setToolTipText(text);
  		    } else if (propertyName.equals("enabled")) {
  			Boolean enabledState = (Boolean) e.getNewValue();
  			button.setEnabled(enabledState.booleanValue());
  			button.repaint();
  		    } 
  		}
  	    }
--- 264,282 ----
  		    Action action = (Action)e.getSource();
  		    action.removePropertyChangeListener(this);
  		} else {
! 		    if (propertyName.equals(Action.NAME)) {
  			String text = (String) e.getNewValue();
  			button.setText(text);
  			button.repaint();
! 		    } else if (propertyName.equals(Action.SHORT_DESCRIPTION)) {
  			String text = (String) e.getNewValue();
  			button.setToolTipText(text);
  		    } else if (propertyName.equals("enabled")) {
  			Boolean enabledState = (Boolean) e.getNewValue();
  			button.setEnabled(enabledState.booleanValue());
  			button.repaint();
+ 		    } else if (propertyName.equals(Action.ACTION_COMMAND_KEY)) {
+                         button.setActionCommand((String)e.getNewValue());
  		    } 
  		}
  	    }
*** /tmp/geta24622	Tue Jun  5 12:28:16 2001
--- JMenu.java	Tue Jun  5 12:24:49 2001
***************
*** 620,637 ****
          }
          public void propertyChange(PropertyChangeEvent e) {
              String propertyName = e.getPropertyName();
!             if (e.getPropertyName().equals(Action.NAME)) {
                  String text = (String) e.getNewValue();
                  menuItem.setText(text);
              } else if (propertyName.equals("enabled")) {
                  Boolean enabledState = (Boolean) e.getNewValue();
                  menuItem.setEnabled(enabledState.booleanValue());
!             } else if (e.getPropertyName().equals(Action.SMALL_ICON)) {
                  Icon icon = (Icon) e.getNewValue();
                  menuItem.setIcon(icon);
                  menuItem.invalidate();
                  menuItem.repaint();
!             } 
          }
  	public void setTarget(JMenuItem b) {
  	    this.menuItem = b;
--- 620,639 ----
          }
          public void propertyChange(PropertyChangeEvent e) {
              String propertyName = e.getPropertyName();
!             if (propertyName.equals(Action.NAME)) {
                  String text = (String) e.getNewValue();
                  menuItem.setText(text);
              } else if (propertyName.equals("enabled")) {
                  Boolean enabledState = (Boolean) e.getNewValue();
                  menuItem.setEnabled(enabledState.booleanValue());
!             } else if (propertyName.equals(Action.SMALL_ICON)) {
                  Icon icon = (Icon) e.getNewValue();
                  menuItem.setIcon(icon);
                  menuItem.invalidate();
                  menuItem.repaint();
!             } else if (propertyName.equals(Action.ACTION_COMMAND_KEY)) {
!                 menuItem.setActionCommand((String)e.getNewValue());
!             }
          }
  	public void setTarget(JMenuItem b) {
  	    this.menuItem = b;
***************
*** 1093,1099 ****
       * <code>Action</code> instance.  The properties 
       * which are set may differ for subclasses.  By default,
       * the properties which get set are <code>Text, Icon, Enabled,
!      * ToolTipText</code> and <code>Mnemonic</code>.
       *
       * @param a the <code>Action</code> from which to get the properties,
       *		or <code>null</code>
--- 1095,1101 ----
       * <code>Action</code> instance.  The properties 
       * which are set may differ for subclasses.  By default,
       * the properties which get set are <code>Text, Icon, Enabled,
!      * ToolTipText, ActionCommand</code>, and <code>Mnemonic</code>.
       *
       * @param a the <code>Action</code> from which to get the properties,
       *		or <code>null</code>
*** /tmp/geta24436	Tue Jun  5 12:26:26 2001
--- JRadioButton.java	Tue Jun  5 12:23:18 2001
***************
*** 185,195 ****
  
  
      /**
!      * Factory method which sets the ActionEvent source's properties
!      * according to values from the Action instance.  The properties 
!      * which are set may differ for subclasses.
       * By default, the properties which get set are <code>Text, Mnemonic,
!      * Enabled,</code> and <code>ToolTipText</code>.
       *
       * @param a the Action from which to get the properties, or null
       * @since 1.3
--- 185,195 ----
  
  
      /**
!      * Factory method which sets the <code>ActionEvent</code> source's
!      * properties according to values from the Action instance. The
!      * properties which are set may differ for subclasses.
       * By default, the properties which get set are <code>Text, Mnemonic,
!      * Enabled, ActionCommand</code>, and <code>ToolTipText</code>.
       *
       * @param a the Action from which to get the properties, or null
       * @since 1.3
***************
*** 198,204 ****
       */
      protected void configurePropertiesFromAction(Action a) {
          String[] types = { Action.MNEMONIC_KEY, Action.NAME,
!                            Action.SHORT_DESCRIPTION, "enabled" };
          configurePropertiesFromAction(a, types);
      }
  
--- 198,205 ----
       */
      protected void configurePropertiesFromAction(Action a) {
          String[] types = { Action.MNEMONIC_KEY, Action.NAME,
!                            Action.SHORT_DESCRIPTION,
!                            Action.ACTION_COMMAND_KEY, "enabled" };
          configurePropertiesFromAction(a, types);
      }
  
***************
*** 228,244 ****
  		    Action action = (Action)e.getSource();
  		    action.removePropertyChangeListener(this);
  		} else {
! 		    if (e.getPropertyName().equals(Action.NAME)) {
  			String text = (String) e.getNewValue();
  			button.setText(text);
  			button.repaint();
! 		    } else if (e.getPropertyName().equals(Action.SHORT_DESCRIPTION)) {
  			String text = (String) e.getNewValue();
  			button.setToolTipText(text);
  		    } else if (propertyName.equals("enabled")) {
  			Boolean enabledState = (Boolean) e.getNewValue();
  			button.setEnabled(enabledState.booleanValue());
  			button.repaint();
  		    } 
  		}
  	    }
--- 229,247 ----
  		    Action action = (Action)e.getSource();
  		    action.removePropertyChangeListener(this);
  		} else {
! 		    if (propertyName.equals(Action.NAME)) {
  			String text = (String) e.getNewValue();
  			button.setText(text);
  			button.repaint();
! 		    } else if (propertyName.equals(Action.SHORT_DESCRIPTION)) {
  			String text = (String) e.getNewValue();
  			button.setToolTipText(text);
  		    } else if (propertyName.equals("enabled")) {
  			Boolean enabledState = (Boolean) e.getNewValue();
  			button.setEnabled(enabledState.booleanValue());
  			button.repaint();
+                     } else if (propertyName.equals(Action.ACTION_COMMAND_KEY)) {
+                         button.setActionCommand((String)e.getNewValue());
  		    } 
  		}
  	    }

###@###.### 2001-06-05

======================================================================
                                     
2001-06-05
CONVERTED DATA

BugTraq+ Release Management Values

COMMIT TO FIX:
merlin-beta2

FIXED IN:
merlin-beta2

INTEGRATED IN:
merlin-beta2


                                     
2004-08-24



Hardware and Software, Engineered to Work Together