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

[Michael Koch]

Am Samstag, 18. Dezember 2004 16:37 schrieb Robert Schuster:
> Hi,
> attached is a patch that fixes bug #11255
> <https://savannah.gnu.org/bugs/?func=detailitem&item_id=11255> and
> a bunch of other problems that I found while investigating
> JComboBox.java

Thanks for your work.

> --- 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

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.

> ===================================================================
> 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

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

>    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

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

> +       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.

>  
>    /**
> @@ -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

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.

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. 
That's why give comments to stuff that other people normally don't 
care about.

The functional changes are totally okay for immidiate commit.


Michael
-- 
Homepage: http://www.worldforge.org/