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

SelectionModel: mismatches between api doc and implementations

    Details

      Description

      below is a bunch of failing tests: they (mostly) fail because they assert against the letters of the api doc. Possible reasons:

      - the specification is incomplete/unclear
      - the implementation violates the specification
      - me misunderstanding the specification :-)

      The tests basically address 3 issues:

      1. select index with index == -1
      2. select item with item == null
      3. select item that's not contained in the underlying data model

      Whatever the resolution for 1 and 2 (either clarify the api doc or change the implementation to comply to specification), it should be consistent across

      - views: table/list selection behave differently when selecting -1
      - selection modes: select(null) behaves differently for single vs. multiple selection

      As to 3, that's more open to discussion: supporting selection of uncontained items is a special case (combo box, f.i.) that doesn't add much value in the typical case (list/table selection). At the same time, It adds so much complexity, that the effort/gain factor explodes. Personally, I would suggest to drop it if not needed.

      The clashes as I see them are explained in the method comments. Test snippetsbelow, fully running tests at https://github.com/kleopatra/swingempire-fx/tree/master/fx8-swingempire/src/test/de/swingempire/fx/scene/control/selection - namely: TableViewSingleSelectionIssues, ChoiceBoxSelectionIssues, ListViewSingleSelectionIssues.

       

          /**
           * Bug or feature? Can select the item in empty selection
           * which then still reports to be empty.
           *
           * Violates doc of isEmpty:
           * "test whether there are any selected indices/items.
           * It will return true if there are no selected items."
           *
           */
          @Test
          public void testSelectUncontainedIfEmptySelection() {
              Object item = "uncontained";
              getSelectionModel().select(item);
              assertEquals("sanity: the item is selected", item, getSelectionModel().getSelectedItem());
              assertFalse("selection must not be empty", getSelectionModel().isEmpty());
          }
          
          /**
           * Issue: broken invariant: if selectedIndex >= 0 --> selectItem = get(selectedIndex)
           *
           * Bug or feature? select(T) explicitly states that it _attempts_ to select the item
           * (and stops at the first find), but doesn't specify what happens if T isn't found.
           *
           * This is done in getSelectedItem it specifies the item being the one
           * "which resides in the selectedIndex position". Also in selectedItemProperty:
           * "The selected item is either null,
           * to represent that there is no selection, or an Object that is retrieved
           * from the underlying data model"
           *
           * ... so looks like a bug to me.
           *
           * Actually, doesn't make sense to allow the index/item correlation being unsynched in the
           * _general_ case - it's a special need if we have selections that are not necessarily
           * backed in the model (like in swing comboboxModel)
           *
           */
          @Test
          public void testSelectUncontainedIfNotEmptySelection() {
              int index = 2;
              getSelectionModel().select(index);
              Object item = "uncontained";
              getSelectionModel().select(item);
              assertEquals(index, getSelectionModel().getSelectedIndex());
              assertEquals("selectedItem must be model item at selectedIndex",
                      items.get(index), getSelectionModel().getSelectedItem());
              // this is what passes, but is inconsistent with the doc of getSelectedItem
              assertEquals("uncontained item must be selected item", item, getSelectionModel().getSelectedItem());
          }
          
          /**
           * Issue: selectedIndex gets out off synch on select null item.
           * Strictly speaking, there are two issues:
           * a) the doc doesn't specify of what happens if the item is null (it does of index being
           * off range)
           * b) whatever happens, it must keep the invariant selectedItem == get(selectedIndex)
           *
           * The invariant isn't explicitly documented, just ...
           */
          @Test
          public void testSelectNullItem() {
              int index = 2;
              getSelectionModel().select(index);
              Object item = getSelectionModel().getSelectedItem();
              getSelectionModel().select(null);
              assertEquals("unspecified behaviour on select(null): what to expect?", item, getSelectionModel().getSelectedItem());
              fail("unspecified behaviour for selecting null item");
          }
          
          @Test
          public void testSelectNullIndex() {
              int index = 2;
              getSelectionModel().select(index);
              Object item = getSelectionModel().getSelectedItem();
              getSelectionModel().select(null);
              assertEquals("unspecified behaviour on select(null) ", index, getSelectionModel().getSelectedIndex());
              fail("unspecified behaviour for selecting null item");
          }

          /**
           * SelectionModel is documented to do nothing if the index is out of range.
           * Test that selectedItem is unchanged.
           */
          @Test
          public void testSelectMinusOneItem() {
              int index = 2;
              getSelectionModel().select(index);
              getSelectionModel().select(-1);
              assertEquals("selecting off-range (here: - 1) must have no effect on selectedItem", items.get(index), getSelectionModel().getSelectedItem());
          }

          /**
           * SelectionModel is documented to do nothing if the index is out of range.
           * Test that the selectedIndex is unchanged.
           */
          @Test
          public void testSelectMinusOneIndex() {
              int index = 2;
              getSelectionModel().select(index);
              getSelectionModel().select(-1);
              assertEquals("selecting off-range (here: -1) must have no effect on selectedIndex", index, getSelectionModel().getSelectedIndex());
           }

        Attachments

          Issue Links

            Activity

              People

              • Assignee:
                Unassigned
                Reporter:
                fastegal Jeanette Winzenburg
              • Votes:
                0 Vote for this issue
                Watchers:
                5 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Imported: