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

[Robert Schuster]

Hi Michael.
Thanks for your review.

Attached is the revised version of my JComboBox patch.

Please, don't put the ChangeLog entry into the patch. It makes the 
patch not cleanly applyable when someone else commited something 
after you created the diff. Instead just put the ChangeLog entry into 
the mail body.

  

Ok.

<br /> look bad in the resutling javadocs. Please use <p>....</p> 
around your paragraph.

  

Fixed.

Comments should be grammatically correct sentences starting with an 
upper-case letter and ending with a dot.

  

Fixed.

+       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); }
    

The old code is perfectly working afaik. I really wonder why you 
needed to change it.
  

This line was formerly the first:

if (this.dataModel == newDataModel)
-      return;

It would allow having dataModel being null if someone does
     new JComboBox((ComboBoxModel) null);

This line was executed before checking that newDataModel is non-null:

ComboBoxModel oldDataModel = this.dataModel;
-    this.dataModel = newDataModel;

Again this would allow dataModel being null.

if (this.dataModel != null)
-      // register all listeners with the new data model
-      dataModel.addListDataListener(this);

This one makes JComboBoxModel.setModel(null) perfectly legal. But JDK throws a NullPointerException here.

You put many non-functional changes into this patch which had nothing 
to do with the needed fixes. Its much easier to review such stuff 
when a patch only contains functional changes or only reformatting, 
etc.

  

Yes you're right. I just could not stop me when I saw these many problems there where so tempting ... :-(
Next time there will be smaller changes.

Robert, it's nothing personal against you. I just know that our code 
quality is sometimes not really good and I wanna improve it by time. 
  

Never mind, I take it as a good advice rather than personal critique. Isn't that what "RFC" is all about? :-)
I do appreciate good code myself and with your suggestions applied to my changes
I feel much better about my contribution.

Btw: This was my first contribution to Swing code ever. I can hardly think of a criticism that kills the motivation
that I gain from the knowledge that with my contribution someone on this planet can use JComboBox on a Free VM.

cu
Robert

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 17:29:58 -0000
@@ -50,6 +50,7 @@
  *
  * @author Andrew Selkirk
  * @author Olga Rodimina
+ * @author Robert Schuster
  * @version 1.0
  */
 public class DefaultComboBoxModel extends AbstractListModel
@@ -182,13 +183,23 @@
    * 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.
+   * 
+   * <p>If the given object is not contained in the combo box list then nothing
+   * happens.</p>
    *
    * @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 17:30:00 -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 editing an object in a combo box list.
    */
   protected ComboBoxEditor editor;
 
@@ -205,9 +206,10 @@
     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
+     * or select nothing if there are no elements.
+     */
+    setSelectedIndex(getItemCount() != 0 ? 0 : -1);    
 
     lightWeightPopupEnabled = true;
     isEditable = false;
@@ -322,21 +324,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 this method is called from inside the 
constructors.
+    if(dataModel != null) {
+       // Prevents unneccessary updates.
+       if (dataModel == newDataModel)
+               return;
+
+       // Removes itself (as DataListener) from the to-be-replaced model.
+       dataModel.removeListDataListener(this);
+    }
+    
+    /* Adds itself as a DataListener 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);
+
+    // Stores old data model for event notification.
+    ComboBoxModel oldDataModel = dataModel;
+    dataModel = newDataModel;
 
-    firePropertyChange(MODEL_CHANGED_PROPERTY, oldDataModel, this.dataModel);
+    // Notifies the listeners of the model change.
+    firePropertyChange(MODEL_CHANGED_PROPERTY, oldDataModel, dataModel);
   }
 
   /**
@@ -351,8 +361,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 +370,7 @@
    */
   public void setLightWeightPopupEnabled(boolean enabled)
   {
-    this.lightWeightPopupEnabled = enabled;
+    lightWeightPopupEnabled = enabled;
   }
 
   /**
@@ -388,9 +398,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 +417,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 +447,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);
       }
   }
 
@@ -481,7 +491,7 @@
   }
 
   /**
-   * Returns editor component that is responsible for displaying/editting
+   * Returns editor component that is responsible for displaying/editing
    * selected item in the combo box.
    *
    * @return ComboBoxEditor
@@ -503,45 +513,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.
+   * <p>If the index is below -1 or exceeds the upper bound an
+   * <code>IllegalArgumentException</code> is thrown.<p/>
+   * <p>If the index is -1 then no item gets selected.</p>
    *
    * @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.
+   * 
+   * <p>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) where
+   * n is the number of elements in the combo box.</p>
    *
-   * @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: Is 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 +591,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.
+   * <p>A <code>RuntimeException</code> is thrown if the data model is not
+   * an instance of address@hidden MutableComboBoxModel}.</p>
    *
    * @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.
+   * <p>A <code>RuntimeException</code> is thrown if the data model is not
+   * an instance of address@hidden MutableComboBoxModel}.</p>
    *
    * @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.
+   * <p>A <code>RuntimeException</code> is thrown if the data model is not
+   * an instance of address@hidden MutableComboBoxModel}.</p>
    *
    * @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.
+   * <p>A <code>RuntimeException</code> is thrown if the data model is not
+   * an instance of address@hidden MutableComboBoxModel}.</p>
    *
    * @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.
+   * <p>A <code>RuntimeException</code> is thrown if the data model is not
+   * an instance of address@hidden MutableComboBoxModel}.</p>
+   * 
    */
   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 +886,7 @@
    */
   public Object[] getSelectedObjects()
   {
-    Object selectedObject = getSelectedItem();
-    return new Object[] { selectedObject };
+    return new Object[] { getSelectedItem() };
   }
 
   /**
@@ -951,7 +1035,7 @@
    */
   public int getItemCount()
   {
-    return ((DefaultComboBoxModel) dataModel).getSize();
+    return dataModel.getSize();
   }
 
   /**
@@ -963,7 +1047,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 17:30:03 -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