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

ComboBox: incorrect selection update/notification on disjoint list changes

XMLWordPrintable

      There are several variant, related to incorrect handling of a list change event that has multiple sub-changes

      1. the selected index isn't always updated
      2. even if updated, selectedIndex fires too many events: must be one per change, must not fire for each sub-change
      3. an unchanged selectedItem must not fire any notification

      Technically, the basic error for 1. and 2. might be in itemsContentObserver, which does something like:

            while (c.next()) {
                   if (removedBeforeSelected) {
                        // must not access the list during subchanges!
                        // the list's change is already completed, that is oldLast - someShift
                        // is off range
                        clearAndSelect(getSelectedIndex() - someShift);
                   }
            }

      Instead the shift must be calculated during the loop and selection only updated after, something like (pseudo-code, untested ;-)

            int shift = 0;
            while (c.next()) {
                 if (removeBeforeSelected) {
                       shift += c.getRemovedSize();
                 } else {
                    break;
                 }
            }
            if (shift > 0)
                 clearAndSelect(getSelectedIndex() - shift);

      There might be some additional destructive interference from the ListSelectionModel of the popup though, didn't dig (my own implementation is too different from core to be sure about the exact reason for the failure).

      Similar incorrect notifications - the bullets 2 and 3 above) can be seen in list, table (probably treeTable as well) and tree. Similar incorrect index value (bullet 1) only in table.

      failing test snippets

          /**
           * Bullet 1: selected index must be updated
           * Corner case: last selected. Fails for core
           */
          @Test
          public void testSelectedAtLastOnDisjointRemoveItemsAbove() {
              int last = items.size() - 1;
              getSelectionModel().select(last);
              // disjoint remove of 2 elements above the last selected
              // removeAll(1, 3);
              items.removeAll(items.get(1), items.get(3));
              int expected = last - 2;
              assertEquals("selected index after disjoint removes above",
                      expected, getSelectionModel().getSelectedIndex());
          }

          /**
           * Variant of 1: if selectedIndex is not updated,
           * the old index is no longer valid
           * for accessing the items.
           */
          @Test
          public void testAccessSelectedAtLastOnDisjointRemoveItemsAbove() {
              int last = items.size() - 1;
              getSelectionModel().select(last);
              // disjoint remove of 2 elements above the last selected
              items.removeAll(items.get(1), items.get(3));
              int selected = getSelectionModel().getSelectedIndex();
              if (selected > -1)
                  items.get(selected);
          }

          /**
           * Bullet 2: selectedIndex notification count
           *
           * Note that we don't use the corner case of having the last index selected
           * (which fails already on updating the index)
           */
          @Test
          public void testSelectedIndexNotificationOnDisjointRemovesAbove() {
              int last = items.size() - 2;
              getSelectionModel().select(last);
              assertEquals(last, getSelectionModel().getSelectedIndex());
              ChangeReport report = new ChangeReport(getSelectionModel().selectedIndexProperty());
              // disjoint remove of 2 elements above the last selected
              items.removeAll(items.get(1), items.get(3));
              assertEquals("sanity: selectedIndex must be shifted by -2", last - 2,
                      getSelectionModel().getSelectedIndex());
              assertEquals("must fire single event on removes above", 1,
                      report.getEventCount());
          }

          /**
           * Bullet 3: unchanged selectedItem must not fire change
           */
          @Test
          public void testSelectedItemNotificationOnDisjointRemovesAbove() {
              int last = items.size() - 2;
              Object lastItem = items.get(last);
              getSelectionModel().select(last);
              assertEquals(lastItem, getSelectionModel().getSelectedItem());
              ChangeReport report = new ChangeReport(getSelectionModel().selectedItemProperty());
              // disjoint remove of 2 elements above the last selected
              items.removeAll(items.get(1), items.get(3));
              assertEquals("sanity: selectedItem unchanged", lastItem,
               getSelectionModel().getSelectedItem());
              assertEquals("must not fire on unchanged selected item", 0, report.getEventCount());
          }

            jgiles Jonathan Giles
            fastegal Jeanette Winzenburg
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

              Created:
              Updated:
              Resolved:
              Imported: