Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-6516455

BasicListUI has duplicate branch in if-statement

    XMLWordPrintable

    Details

    • Subcomponent:
    • Resolved In Build:
      b10
    • CPU:
      generic
    • OS:
      generic
    • Verification:
      Verified

      Description

      A DESCRIPTION OF THE FIX :
      BasicListUI has a private class Handler which has a method propertyChange(PropertyChangeEvent e). In this method the propertyName is compared with the == operator. This is very bad style and can lead to strange problems (see attached testcases). In addition to that the property "cellRenderer" is checked twice.
      The submitter created a patch against build 1.7.0-ea-b06:

      -------------------
      --- original/BasicListUI.java 2007-01-20 22:24:31.000000000 +0100
      +++ BasicListUI.java 2007-01-20 22:28:14.000000000 +0100
      @@ -2436,7 +2436,7 @@
                   /* If the JList.model property changes, remove our listener,
                    * listDataListener from the old model and add it to the new one.
                    */
      - if (propertyName == "model") {
      + if ("model".equals(propertyName)) {
                       ListModel oldModel = (ListModel)e.getOldValue();
                       ListModel newModel = (ListModel)e.getNewValue();
                       if (oldModel != null) {
      @@ -2452,7 +2452,7 @@
                   /* If the JList.selectionModel property changes, remove our listener,
                    * listSelectionListener from the old selectionModel and add it to the new one.
                    */
      - else if (propertyName == "selectionModel") {
      + else if ("selectionModel".equals(propertyName)) {
                       ListSelectionModel oldModel = (ListSelectionModel)e.getOldValue();
                       ListSelectionModel newModel = (ListSelectionModel)e.getNewValue();
                       if (oldModel != null) {
      @@ -2464,48 +2464,44 @@
                       updateLayoutStateNeeded |= modelChanged;
        redrawList();
                   }
      - else if (propertyName == "cellRenderer") {
      + else if ("cellRenderer".equals(propertyName)) {
                       updateLayoutStateNeeded |= cellRendererChanged;
        redrawList();
                   }
      - else if (propertyName == "font") {
      + else if ("font".equals(propertyName)) {
                       updateLayoutStateNeeded |= fontChanged;
        redrawList();
                   }
      - else if (propertyName == "prototypeCellValue") {
      + else if ("prototypeCellValue".equals(propertyName)) {
                       updateLayoutStateNeeded |= prototypeCellValueChanged;
        redrawList();
                   }
      - else if (propertyName == "fixedCellHeight") {
      + else if ("fixedCellHeight".equals(propertyName)) {
                       updateLayoutStateNeeded |= fixedCellHeightChanged;
        redrawList();
                   }
      - else if (propertyName == "fixedCellWidth") {
      + else if ("fixedCellWidth".equals(propertyName)) {
                       updateLayoutStateNeeded |= fixedCellWidthChanged;
        redrawList();
                   }
      - else if (propertyName == "cellRenderer") {
      - updateLayoutStateNeeded |= cellRendererChanged;
      - redrawList();
      - }
      - else if (propertyName == "selectionForeground") {
      + else if ("selectionForeground".equals(propertyName)) {
        list.repaint();
                   }
      - else if (propertyName == "selectionBackground") {
      + else if ("selectionBackground".equals(propertyName)) {
        list.repaint();
                   }
      - else if ("layoutOrientation" == propertyName) {
      + else if ("layoutOrientation".equals(propertyName)) {
                       updateLayoutStateNeeded |= layoutOrientationChanged;
                       layoutOrientation = list.getLayoutOrientation();
                       redrawList();
                   }
      - else if ("visibleRowCount" == propertyName) {
      + else if ("visibleRowCount".equals(propertyName)) {
                       if (layoutOrientation != JList.VERTICAL) {
                           updateLayoutStateNeeded |= layoutOrientationChanged;
                           redrawList();
                       }
                   }
      - else if ("componentOrientation" == propertyName) {
      + else if ("componentOrientation".equals(propertyName)) {
        isLeftToRight = list.getComponentOrientation().isLeftToRight();
                       updateLayoutStateNeeded |= componentOrientationChanged;
                       redrawList();
      @@ -2513,10 +2509,10 @@
        InputMap inputMap = getInputMap(JComponent.WHEN_FOCUSED);
        SwingUtilities.replaceUIInputMap(list, JComponent.WHEN_FOCUSED,
        inputMap);
      - } else if ("List.isFileList" == propertyName) {
      + } else if ("List.isFileList".equals(propertyName)) {
        updateIsFileList();
        redrawList();
      - } else if ("dropLocation" == propertyName) {
      + } else if ("dropLocation".equals(propertyName)) {
                       JList.DropLocation oldValue = (JList.DropLocation)e.getOldValue();
                       repaintDropLocation(oldValue);
                       repaintDropLocation(list.getDropLocation());
      -------------------

      JUnit TESTCASE :
      Here is a testcase that verifies the problem:
      -------------------
      import java.beans.PropertyChangeEvent;
      import java.beans.PropertyChangeListener;
      import java.lang.reflect.Field;
      import java.lang.reflect.Method;
      import javax.swing.JList;
      import javax.swing.plaf.basic.BasicListUI;
      import junit.framework.TestCase;

      /**
       *
       * @author ###@###.###
       */
      public class BasicListUITest extends TestCase {
          
          public void testBasicListUI() {
              try {
                  JList list = new JList();
                  BasicListUI basicListUI = (BasicListUI) list.getUI();
                  
                  // get updateLayoutStateNeeded
                  Field ulsnField = BasicListUI.class.getDeclaredField("updateLayoutStateNeeded");
                  ulsnField.setAccessible(true);
                  int ulsnBefore = ulsnField.getInt(basicListUI);
                  
                  // get handler
                  Method getHandlerMethod =
                          BasicListUI.class.getDeclaredMethod("getHandler");
                  getHandlerMethod.setAccessible(true);
                  PropertyChangeListener propertyChangeListener =
                          (PropertyChangeListener) getHandlerMethod.invoke(basicListUI);
                  propertyChangeListener.propertyChange(
                          new PropertyChangeEvent(this, "model", null, null));
                  
                  // check updateLayoutStateNeeded again
                  int ulsnAfter = ulsnField.getInt(basicListUI);
                  assertFalse("The field \"updateLayoutStateNeeded\" was unchanged after property changes!",
                          ulsnBefore == ulsnAfter);
                  
              } catch (Exception ex) {
                  ex.printStackTrace();
              }
          }
      }
      -------------------



      Here is an example program that shows the problem in a real example without the reflection magic used in the JUnit test. I subclassed JList and suddenly setModel() stops working!
      -------------------
      import javax.swing.*;

      /**
       * @author ###@###.###
       */
      public class Test extends JFrame {
          
          private JList jList1;
          private JScrollPane jScrollPane1;
          
          public Test() {
              initComponents();
              DefaultListModel listModel = new DefaultListModel();
              listModel.addElement("test");
              jList1.setModel(listModel);
          }
          
          private void initComponents() {
              jScrollPane1 = new JScrollPane();
              jList1 = new MyJList();

              setDefaultCloseOperation(WindowConstants.EXIT_ON_CLOSE);
              jScrollPane1.setViewportView(jList1);

              GroupLayout layout = new GroupLayout(getContentPane());
              getContentPane().setLayout(layout);
              layout.setHorizontalGroup(
                  layout.createParallelGroup(GroupLayout.Alignment.LEADING)
                  .addGroup(layout.createSequentialGroup()
                      .addContainerGap()
                      .addComponent(jScrollPane1, GroupLayout.DEFAULT_SIZE, 376, Short.MAX_VALUE)
                      .addContainerGap())
              );
              layout.setVerticalGroup(
                  layout.createParallelGroup(GroupLayout.Alignment.LEADING)
                  .addGroup(layout.createSequentialGroup()
                      .addContainerGap()
                      .addComponent(jScrollPane1, GroupLayout.DEFAULT_SIZE, 276, Short.MAX_VALUE)
                      .addContainerGap())
              );
              pack();
          }
          
          public static void main(String args[]) {
              java.awt.EventQueue.invokeLater(new Runnable() {
                  public void run() {
                      new Test().setVisible(true);
                  }
              });
          }
          
          private class MyJList extends JList {
              
              public void setModel(ListModel model) {
                  if (model == null) {
                      throw new IllegalArgumentException("model must be non null");
                  }
                  ListModel dataModel = getModel();
                  ListModel oldValue = dataModel;
                  dataModel = model;
                  firePropertyChange("model", oldValue, dataModel);
                  clearSelection();
              }
          }
      }
      -------------------

        Attachments

          Activity

            People

            Assignee:
            shickeysunw Shannon Hickey (Inactive)
            Reporter:
            tbell Tim Bell
            Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved:
              Imported:
              Indexed: