SelectionModel: mismatches between api doc and implementations

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());
     }