|
From: | Robert Schuster |
Subject: | [cp-patches] RFC: fix for #11255 and other JComboBox problems - UPDATE 1 |
Date: | Sat, 18 Dec 2004 18:44:49 +0100 |
User-agent: | Mozilla/5.0 (X11; U; Linux i686; de-AT; rv:1.7.3) Gecko/20040930 |
Hi Michael. Thanks for your review. Attached is the revised version of my JComboBox patch. Ok.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. Fixed.<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. + 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. Yes you're right. I just could not stop me when I saw these many problems there where so tempting ... :-(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. Next time there will be smaller changes. Never mind, I take it as a good advice rather than personal critique. Isn't that what "RFC" is all about? :-)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. 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
[Prev in Thread] | Current Thread | [Next in Thread] |