[cp-patches] RFC: fix for #11255 and other JComboBox problems

[Robert Schuster]

Hi,
attached is a patch that fixes bug #11255 and a bunch of other problems that I found while investigating JComboBox.java

I have tested this with JamVM 1.2.1 .

cu
Robert

Index: ChangeLog
===================================================================
RCS file: /cvsroot/classpath/classpath/ChangeLog,v
retrieving revision 1.2917
diff -u -r1.2917 ChangeLog
--- ChangeLog   17 Dec 2004 21:54:48 -0000      1.2917
+++ ChangeLog   18 Dec 2004 15:26:39 -0000
@@ -1,3 +1,36 @@
+2004-12-18  Robert Schuster <address@hidden>
+
+       * javax/swing/JComboBox.java
+       added support for no item being selected
+       (JComboBox): select first or nothing depending on element
+       count
+       (setModel): cleaned up unneeded "this." usage, added more
+       docs, made exception behavior match that of the JDK
+       (setLighWeightPopupEnabled): removed unneeded "this." usage
+       (setEditable): dito
+       (setMaximumRowCount): dito
+       (setRenderer): dito
+       (setPrototypeDisplayValue): dito
+       (getSelectedItem): simplified, added more user doc
+       (setSelectedIndex): corrected exception behavior, added more user
+       doc
+       (getSelectedIndex): fixed hardcoded dependency on DefaultComboBoxModel
+        (see bug #11255), added performance warning to user doc
+       (addItem): fixed exception behavior, added user doc
+       (insertItemAt): dito
+       (removeItem): dito
+       (removeItemAt): dito
+       (removeAll): fixed exception behavior, added user doc, added support
+       for model not being instance of DefaultComboBoxModel (see bug #11255)
+       (getSelectedItemObjects): simplified
+       (getItemCount): fixed dependency on DefaultComboBoxModel (see bug 
#11255)
+       (getItemAt): fixed dependency on MutableComboBoxModel (see bug #11255)
+       * javax/swing/DefaultComboBoxModel.java:
+       (setSelectedItem): updates selected item only if new 
+       value is null or known (match JDK behavior)
+       * javax/swing/plaf/basic/BasicComboBoxUI.java:
+       (paintCurrentValue): renders "" if no item is selected
+
 2004-12-17  Michael Koch  <address@hidden>
 
        * gnu/java/locale/LocaleInformation_de.java,
Index: javax/swing/DefaultComboBoxModel.java
===================================================================
RCS file: /cvsroot/classpath/classpath/javax/swing/DefaultComboBoxModel.java,v
retrieving revision 1.4
diff -u -r1.4 DefaultComboBoxModel.java
--- javax/swing/DefaultComboBoxModel.java       5 Sep 2004 11:31:06 -0000       
1.4
+++ javax/swing/DefaultComboBoxModel.java       18 Dec 2004 15:26:50 -0000
@@ -50,6 +50,7 @@
  *
  * @author Andrew Selkirk
  * @author Olga Rodimina
+ * @author Robert Schuster
  * @version 1.0
  */
 public class DefaultComboBoxModel extends AbstractListModel
@@ -181,14 +182,24 @@
    * Selects given object in the combo box list. This method fires
    * ListDataEvent to all registered ListDataListeners of the JComboBox. The
    * start and end index of the event is set to -1 to indicate combo box's
-   * selection has changed, and not its contents.
+   * selection has changed, and not its contents.<br/>
+   * <br/>
+   * If the given object is not contained in the combo box list then nothing
+   * happens.
    *
    * @param object item to select in the JComboBox
    */
   public void setSelectedItem(Object object)
   {
-    selectedItem = object;
-    fireContentsChanged(this, -1, -1);
+    
+    /* Updates the selected item only if the given object
+     * is null or in the list (this is how the JDK behaves).
+     */ 
+    if(object == null || list.contains(object)) {
+       selectedItem = object;
+       fireContentsChanged(this, -1, -1);
+    }
+       
   }
 
   /**
Index: javax/swing/JComboBox.java
===================================================================
RCS file: /cvsroot/classpath/classpath/javax/swing/JComboBox.java,v
retrieving revision 1.10
diff -u -r1.10 JComboBox.java
--- javax/swing/JComboBox.java  22 Oct 2004 12:43:59 -0000      1.10
+++ javax/swing/JComboBox.java  18 Dec 2004 15:26:52 -0000
@@ -60,7 +60,6 @@
 import javax.swing.event.PopupMenuListener;
 import javax.swing.plaf.ComboBoxUI;
 
-
 /**
  * JComboBox. JComboBox is a container, that keeps track of elements added to
  * it by the user. JComboBox allows user to select any item in its list and
@@ -69,12 +68,14 @@
  *
  * @author Andrew Selkirk
  * @author Olga Rodimina
+ * @author Robert Schuster
  */
 public class JComboBox extends JComponent implements ItemSelectable,
                                                      ListDataListener,
                                                      ActionListener,
                                                      Accessible
 {
+
   private static final long serialVersionUID = 5654585963292734470L;
 
   /**
@@ -143,7 +144,7 @@
   protected ListCellRenderer renderer;
 
   /**
-   * editor that is responsible for editting an object in a combo box list
+   * Editor that is responsible for editting an object in a combo box list.
    */
   protected ComboBoxEditor editor;
 
@@ -205,9 +206,8 @@
     setModel(model);
     setActionCommand("comboBoxChanged");
 
-    // by default set selected item to the first element in the combo box    
-    if (getItemCount() != 0)
-      setSelectedItem(getItemAt(0));
+    // by default set selected item to the first element in the combo box
+    setSelectedIndex(getItemCount() != 0 ? 0 : -1);    
 
     lightWeightPopupEnabled = true;
     isEditable = false;
@@ -322,21 +322,29 @@
    */
   public void setModel(ComboBoxModel newDataModel)
   {
-    if (this.dataModel == newDataModel)
-      return;
 
-    if (this.dataModel != null)
-      // remove all listeners currently registered with the model.
-      dataModel.removeListDataListener(this);
-
-    ComboBoxModel oldDataModel = this.dataModel;
-    this.dataModel = newDataModel;
-
-    if (this.dataModel != null)
-      // register all listeners with the new data model
-      dataModel.addListDataListener(this);
+    // dataModel is null if it is called from inside the constructors
+    if(dataModel != null) {
+       // prevents unneccessary updates
+       if (dataModel == newDataModel)
+               return;
+
+       // removes itself from the to-be-replaced model
+       dataModel.removeListDataListener(this);
+    }
+    
+    /* adds itself to the new model
+     * It is intentioned that this operation will fail with a 
NullPointerException if the
+     * caller delivered a null argument.
+     */
+    newDataModel.addListDataListener(this);
 
-    firePropertyChange(MODEL_CHANGED_PROPERTY, oldDataModel, this.dataModel);
+    // stores old data model for event notification
+    ComboBoxModel oldDataModel = dataModel;
+    dataModel = newDataModel;
+
+    // notifies the listeners of the model change
+    firePropertyChange(MODEL_CHANGED_PROPERTY, oldDataModel, dataModel);
   }
 
   /**
@@ -351,8 +359,8 @@
 
   /**
    * This method sets JComboBox's popup to be either lightweight or
-   * heavyweight. If 'enabled' is true then lightweight popup is  used and
-   * heavyweight otherwise. By default lightweight popup is  used to display
+   * heavyweight. If 'enabled' is true then lightweight popup is used and
+   * heavyweight otherwise. By default lightweight popup is used to display
    * this JComboBox's elements.
    *
    * @param enabled indicates if lightweight popup or heavyweight popup should
@@ -360,7 +368,7 @@
    */
   public void setLightWeightPopupEnabled(boolean enabled)
   {
-    this.lightWeightPopupEnabled = enabled;
+    lightWeightPopupEnabled = enabled;
   }
 
   /**
@@ -388,9 +396,9 @@
    */
   public void setEditable(boolean editable)
   {
-    if (this.isEditable != editable)
+    if (isEditable != editable)
       {
-       this.isEditable = editable;
+       isEditable = editable;
        firePropertyChange(EDITABLE_CHANGED_PROPERTY, ! isEditable, isEditable);
       }
   }
@@ -407,10 +415,10 @@
   {
     if (maximumRowCount != rowCount)
       {
-       int oldMaximumRowCount = this.maximumRowCount;
-       this.maximumRowCount = rowCount;
+       int oldMaximumRowCount = maximumRowCount;
+       maximumRowCount = rowCount;
        firePropertyChange(MAXIMUM_ROW_COUNT_CHANGED_PROPERTY,
-                          oldMaximumRowCount, this.maximumRowCount);
+                          oldMaximumRowCount, maximumRowCount);
       }
   }
 
@@ -437,12 +445,12 @@
    */
   public void setRenderer(ListCellRenderer aRenderer)
   {
-    if (this.renderer != aRenderer)
+    if (renderer != aRenderer)
       {
-       ListCellRenderer oldRenderer = this.renderer;
-       this.renderer = aRenderer;
+       ListCellRenderer oldRenderer = renderer;
+       renderer = aRenderer;
        firePropertyChange(RENDERER_CHANGED_PROPERTY, oldRenderer,
-                          this.renderer);
+                          renderer);
       }
   }
 
@@ -503,45 +511,76 @@
 
   /**
    * Returns currently selected item in the combo box.
+   * The result may be <code>null</code> to indicate that nothing is
+   * currently selected.
    *
    * @return element that is currently selected in this combo box.
    */
   public Object getSelectedItem()
   {
-    Object item = dataModel.getSelectedItem();
-
-    if (item == null && getItemCount() != 0)
-      item = getItemAt(0);
-
-    return item;
+    return dataModel.getSelectedItem();
   }
 
   /**
-   * Forces JComboBox to select component located in the  given index in the
+   * Forces JComboBox to select component located in the given index in the
    * combo box.
    *
+   * If the index is below -1 or exceeds the upper bound an
+   * <code>IllegalArgumentException</code> is thrown.<br/>
+   * If the index is -1 then no item gets selected.
+   *
    * @param index index specifying location of the component that  should be
    *        selected.
    */
   public void setSelectedIndex(int index)
   {
-    // FIXME: if index == -1 then nothing should be selected
-    setSelectedItem(dataModel.getElementAt(index));
+       if(index < -1 || index >= dataModel.getSize()) {
+               /* fails because index is out of bounds. */ 
+               throw new IllegalArgumentException("illegal index: " + index);
+       } else {
+               /* selects the item at the given index or clears the selection 
if the
+                * index value is -1.
+                */
+               setSelectedItem((index == -1) ? null : 
dataModel.getElementAt(index));
+       }
   }
 
   /**
    * Returns index of the item that is currently selected  in the combo box.
    * If no item is currently selected, then -1 is returned.
+   * <br>
+   * Note: For performance reasons you should minimize invocation of this
+   * method. If the data model is not an instance of
+   * <code>DefaultComboBoxModel</code> the complexity is O(n).
    *
-   * @return int index specifying location of the currently  selected item in
+   * @return int Index specifying location of the currently selected item in
    *         the combo box or -1 if nothing is selected in the combo box.
    */
   public int getSelectedIndex()
   {
     Object selectedItem = getSelectedItem();
-    if (selectedItem != null && (dataModel instanceof DefaultComboBoxModel))
-      return ((DefaultComboBoxModel) dataModel).getIndexOf(selectedItem);
+    
+    if (selectedItem != null) {
+       
+               if(dataModel instanceof DefaultComboBoxModel) {
+                       // uses special method of DefaultComboBoxModel to 
retrieve the index
+                       return ((DefaultComboBoxModel) 
dataModel).getIndexOf(selectedItem);
+               } else {
+                       // iterates over all items to retrieve the index
+                       int size = dataModel.getSize();
+                       
+                       for(int i=0; i < size; i++) {
+                               Object o = dataModel.getElementAt(i);
+                               
+                               // XXX: special handling of ComparableS 
neccessary?
+                               if((selectedItem != null) ? 
selectedItem.equals(o) : o == null) {
+                                       return i;
+                               }
+                       }
+               }
+    }
 
+    // returns that no item is currently selected
     return -1;
   }
 
@@ -550,60 +589,104 @@
     return prototypeDisplayValue;
   }
 
-  public void setPrototypeDisplayValue(Object prototypeDisplayValue)
+  public void setPrototypeDisplayValue(Object newPrototypeDisplayValue)
   {
-    this.prototypeDisplayValue = prototypeDisplayValue;
+    prototypeDisplayValue = newPrototypeDisplayValue;
   }
 
   /**
-   * This method adds given element to this JComboBox.
+   * This method adds given element to this JComboBox.<br/>
+   * A <code>RuntimeException</code> is thrown if the data model is not
+   * an instance of address@hidden MutableComboBoxModel}.
    *
    * @param element element to add
    */
   public void addItem(Object element)
   {
-    ((MutableComboBoxModel) dataModel).addElement(element);
+       if(dataModel instanceof MutableComboBoxModel) {
+               ((MutableComboBoxModel) dataModel).addElement(element);
+       } else {
+               throw new RuntimeException("Unable to add the item because the 
data model it is not an instance of MutableComboBoxModel.");
+       }
   }
 
   /**
-   * Inserts given element at the specified index to this JComboBox
+   * Inserts given element at the specified index to this JComboBox.<br/>
+   * A <code>RuntimeException</code> is thrown if the data model is not
+   * an instance of address@hidden MutableComboBoxModel}.
    *
    * @param element element to insert
    * @param index position where to insert the element
    */
   public void insertItemAt(Object element, int index)
   {
-    ((MutableComboBoxModel) dataModel).insertElementAt(element, index);
+       if(dataModel instanceof MutableComboBoxModel) {
+               ((MutableComboBoxModel) dataModel).insertElementAt(element, 
index);
+       } else {
+               throw new RuntimeException("Unable to insert the item because 
the data model it is not an instance of MutableComboBoxModel.");
+       }
   }
 
   /**
-   * This method removes given element from this JComboBox.
+   * This method removes given element from this JComboBox.<br/>
+   * A <code>RuntimeException</code> is thrown if the data model is not
+   * an instance of address@hidden MutableComboBoxModel}.
    *
    * @param element element to remove
    */
   public void removeItem(Object element)
   {
-    ((MutableComboBoxModel) dataModel).removeElement(element);
+       if(dataModel instanceof MutableComboBoxModel) {
+               ((MutableComboBoxModel) dataModel).removeElement(element);
+       } else {
+               throw new RuntimeException("Unable to remove the item because 
the data model it is not an instance of MutableComboBoxModel.");
+       }
   }
 
   /**
    * This method remove element location in the specified index in the
-   * JComboBox.
+   * JComboBox.<br/>
+   * A <code>RuntimeException</code> is thrown if the data model is not
+   * an instance of address@hidden MutableComboBoxModel}.
    *
    * @param index index specifying position of the element to remove
    */
   public void removeItemAt(int index)
   {
-    ((MutableComboBoxModel) dataModel).removeElementAt(index);
+       if(dataModel instanceof MutableComboBoxModel) {
+               ((MutableComboBoxModel) dataModel).removeElementAt(index);
+       } else {
+               throw new RuntimeException("Unable to remove the item because 
the data model it is not an instance of MutableComboBoxModel.");
+       }
   }
 
   /**
-   * This method removes all elements from this JComboBox.
+   * This method removes all elements from this JComboBox.<br/>
+   * A <code>RuntimeException</code> is thrown if the data model is not
+   * an instance of address@hidden MutableComboBoxModel}.
+   * 
    */
   public void removeAllItems()
   {
-    if (dataModel instanceof DefaultComboBoxModel)
-      ((DefaultComboBoxModel) dataModel).removeAllElements();
+    if (dataModel instanceof DefaultComboBoxModel) {
+       // uses special method if we have a DefaultComboBoxModel
+       ((DefaultComboBoxModel) dataModel).removeAllElements();
+    } else if(dataModel instanceof MutableComboBoxModel){
+       // iterates over all items and removes each
+       MutableComboBoxModel mcbm = (MutableComboBoxModel) dataModel;
+
+       /* We intentionally remove the items backwards to support
+        * models which shift their content to the beginning (e.g.
+        * linked lists) 
+        */             
+       for(int i=mcbm.getSize()-1; i >= 0; i--) {
+               mcbm.removeElementAt(i);
+       }
+       
+    } else {
+       throw new RuntimeException("Unable to remove the items because the data 
model it is not an instance of MutableComboBoxModel.");
+    }
+      
   }
 
   /**
@@ -801,8 +884,7 @@
    */
   public Object[] getSelectedObjects()
   {
-    Object selectedObject = getSelectedItem();
-    return new Object[] { selectedObject };
+    return new Object[] { getSelectedItem() };
   }
 
   /**
@@ -951,7 +1033,7 @@
    */
   public int getItemCount()
   {
-    return ((DefaultComboBoxModel) dataModel).getSize();
+    return dataModel.getSize();
   }
 
   /**
@@ -963,7 +1045,7 @@
    */
   public Object getItemAt(int index)
   {
-    return ((MutableComboBoxModel) dataModel).getElementAt(index);
+    return dataModel.getElementAt(index);
   }
 
   /**
Index: javax/swing/plaf/basic/BasicComboBoxUI.java
===================================================================
RCS file: 
/cvsroot/classpath/classpath/javax/swing/plaf/basic/BasicComboBoxUI.java,v
retrieving revision 1.4
diff -u -r1.4 BasicComboBoxUI.java
--- javax/swing/plaf/basic/BasicComboBoxUI.java 22 Oct 2004 12:44:00 -0000      
1.4
+++ javax/swing/plaf/basic/BasicComboBoxUI.java 18 Dec 2004 15:26:56 -0000
@@ -80,6 +80,7 @@
  * UI Delegate for JComboBox
  *
  * @author Olga Rodimina
+ * @author Robert Schuster
  */
 public class BasicComboBoxUI extends ComboBoxUI
 {
@@ -783,22 +784,25 @@
       {
        Object currentValue = comboBox.getSelectedItem();
        boolean isPressed = arrowButton.getModel().isPressed();
-       if (currentValue != null)
-         {
-           Component comp = comboBox.getRenderer()
+
+       /* Gets the component to be drawn for the current value.
+        * If there is currently no selected item we will take an empty
+        * String as replacement.
+        */
+       Component comp = comboBox.getRenderer()
                                     .getListCellRendererComponent(listBox,
-                                                                  currentValue,
+                                                                  
(currentValue != null ? currentValue : ""),
                                                                   -1,
                                                                   isPressed,
                                                                   hasFocus);
-           if (! comboBox.isEnabled())
+       if (! comboBox.isEnabled())
              comp.setEnabled(false);
 
-           g.translate(borderInsets.left, borderInsets.top);
+       g.translate(borderInsets.left, borderInsets.top);
            comp.setBounds(0, 0, bounds.width, bounds.height);
            comp.paint(g);
            g.translate(-borderInsets.left, -borderInsets.top);
-         }
+           
        comboBox.revalidate();
       }
     else