Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8235491: Tree/TableView: implementation of isSelected(int) violates contract #839

Closed
wants to merge 17 commits into from
+316 −40
Closed

8235491: Tree/TableView: implementation of isSelected(int) violates contract #839

Changes from 2 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
a87821c
8235491: Tree/TableView: implementation of isSelected(int) violates c…
andy-goryachev-oracle Jul 19, 2022
82f0db7
8235491: 2022
andy-goryachev-oracle Jul 19, 2022
784833d
8235491: javadoc
andy-goryachev-oracle Jul 21, 2022
2b76edd
8235491: whitespace
andy-goryachev-oracle Jul 21, 2022
fcdf223
8235491: review comments
andy-goryachev-oracle Jul 26, 2022
74e8b15
Merge remote-tracking branch 'origin/master' into 8235491.isselected
andy-goryachev-oracle Jul 27, 2022
be4825a
8235491: tree table view
andy-goryachev-oracle Jul 27, 2022
6267a0e
8235491: javadoc
andy-goryachev-oracle Jul 28, 2022
de8cd41
Merge remote-tracking branch 'origin/master' into 8235491.isselected
andy-goryachev-oracle Aug 3, 2022
cb3a809
8235491: additional tests
andy-goryachev-oracle Aug 3, 2022
ad3c70b
8235491: whitespace
andy-goryachev-oracle Aug 3, 2022
7fa2588
8235491: revert clear selection
andy-goryachev-oracle Aug 8, 2022
9de5858
8235491: delegate to super()
andy-goryachev-oracle Aug 8, 2022
cc7953b
8235491: removed unnecessary method
andy-goryachev-oracle Aug 9, 2022
7646693
8235491: updated test
andy-goryachev-oracle Aug 9, 2022
35247bc
Merge remote-tracking branch 'origin/master' into 8235491.isselected
andy-goryachev-oracle Aug 10, 2022
9ad9352
8235491: added tests
andy-goryachev-oracle Aug 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions modules/javafx.controls/src/main/java/javafx/scene/control/SelectionModel.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2010, 2021, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2010, 2022, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -199,8 +199,8 @@ public SelectionModel() { }

/**
* <p>Convenience method to inform if the given index is currently selected
* in this SelectionModel. Is functionally equivalent to calling
* <code>getSelectedIndices().contains(index)</code>.
* in this SelectionModel. It will return true when cell selection is enabled
* and at least one cell is selected in the specified row index.
*
* @param index The index to check as to whether it is currently selected
* or not.
18 changes: 15 additions & 3 deletions modules/javafx.controls/src/main/java/javafx/scene/control/TableView.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2011, 2021, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2011, 2022, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -2813,8 +2813,20 @@ private void quietClearSelection() {
stopAtomic();
}

@Override public boolean isSelected(int index) {
return isSelected(index, null);
@Override
public boolean isSelected(int index) {
final boolean isCellSelectionEnabled = isCellSelectionEnabled();
if (isCellSelectionEnabled) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Style : Why to create a local variable with the same name as method it gets its value from?
In this case changing to if (isCellSelectionEnabled()) condition check looks simple enough.

Copy link
Contributor Author

@andy-goryachev-oracle andy-goryachev-oracle Jul 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you for your comments, Ajit! below are responses, please let me know if you agree or not:

  1. javadoc for TableSelectionModel.isSelected(int, TablecolumnBase) already describes the logic:
    `
    /**

    • Convenience function which tests whether the given row and column index
    • is currently selected in this table instance. If the table control is in its
    • 'cell selection' mode (where individual cells can be selected, rather than
    • entire rows), and if the column argument is null, this method should return
    • true only if all cells in the given row are selected.
    • @param row the row
    • @param column the column
    • @return true if the given row and column index is currently selected in
    • this table instance
      */
      public abstract boolean isSelected(int row, TableColumnBase<T,?> column);
      `

  2. TreeTableView.TreeTableViewSelectionModel extends TableSelectionModel, so the changes affect both.

  3. good point, fixed.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

1. javadoc for TableSelectionModel.isSelected(int, TablecolumnBase) already describes the logic:

Yes. I am aware of this documentation. I was thinking of making it clear for the users if they see their application breaks due to this change. I see that you have captured this in "Compatibility Impact" section of the CSR.

2. TreeTableView.TreeTableViewSelectionModel extends TableSelectionModel, so the changes affect both.

I see that there are overridden methods public boolean isSelected(int index) and public boolean isSelected(int row, TableColumnBase<TreeItem<S>,?> column) in class TreeTableViewArrayListSelectionModel present in TreeTableView.java. Hence, I think that the change that you have made in TableView.java - isSelected(int index) method should also be made in TreeTableView.java - isSelected(int index) as well.

3. good point, fixed.

Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Would you suggest possible clarification please? The javadoc for SelectionModel.isSelected(int) is changed to say
Convenience method to inform if the given index is currently selected
     * in this SelectionModel.  It will return true when at least one cell
     * is selected in the specified row index.
  1. you are right! fixed.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aghaisas

Yes. I am aware of this documentation. I was thinking of making it clear for the users if they see their application breaks due to this change.

If any application breaks, it's its own fault: they coded against the implementation of TableXXSelectionModels which was (incorrectly) isSelected(int) == isSelected(int, null).

That might be due to the specification mess, though:

  • the old spec on SelectionModel.isSelected(int) was incorrectly arguing with api on MultipleSelectionModel, that is Is functionally equivalent to calling getSelectedIndices().contains(index)
  • that invariant actually belongs into MultipleSelectionModel.isSelected(int) which has no specialized spec
  • the method is first implemented in the (unfortunately hidden) MultipleSelectionModelBase.isSelected(int) fulfilling the constraint but also without spec.

To clean this up completely, we could also change the of MultipleSelectionModel and move the old isSelected(int) == getSelectedIndices().contains(int) into the spec of its isSelected. Probably could be done in a follow-up issue (or added to one of the open doc errors around selection models).

What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clean this up completely, we could also change the of MultipleSelectionModel and move the old isSelected(int) == getSelectedIndices().contains(int) into the spec of its isSelected. Probably could be done in a follow-up issue (or added to one of the open doc errors around selection models).

What do you think?

I like this idea. Either filing a new bug or fold into an existing one seems fine.

int columnCount = tableView.getVisibleLeafColumns().size();
for (int col = 0; col < columnCount; col++) {
if (selectedCellsMap.isSelected(index, col)) {
return true;
}
}
return false;
} else {
return selectedCellsMap.isSelected(index, -1);
}
}

@Override
9 changes: 5 additions & 4 deletions modules/javafx.controls/src/test/java/test/javafx/scene/control/TableViewMouseInputTest.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2011, 2016, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2011, 2022, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -554,6 +554,7 @@ private void test_rt_38306(boolean selectTwoRows) {
TableView.TableViewSelectionModel sm = table.getSelectionModel();
sm.setCellSelectionEnabled(true);
sm.setSelectionMode(SelectionMode.MULTIPLE);
sm.clearSelection();

TableColumn firstNameCol = new TableColumn("First Name");
firstNameCol.setCellValueFactory(new PropertyValueFactory<Person, String>("firstName"));
@@ -714,7 +715,7 @@ private void test_rt_38306(boolean selectTwoRows) {
assertFalse(sm.isSelected(row, firstNameCol));
assertFalse(sm.isSelected(row, lastNameCol));
assertTrue(sm.isSelected(row, emailCol));
assertFalse(sm.isSelected(row));
assertTrue(sm.isSelected(row)); // FIX

// and assert that the visuals are accurate
// (some TableCells should be selected, but TableRows should not be)
@@ -787,7 +788,7 @@ private void test_rt_38464_selectedColumnChangesWhenCellsInRowClicked(boolean ce
if (cellSelection) {
// Because we are in cell selection mode, this has the effect of
// selecting just the one cell.
assertFalse(sm.isSelected(0));
assertTrue(sm.isSelected(0)); // FIX
assertTrue(sm.isSelected(0, firstNameCol));
assertFalse(sm.isSelected(0, lastNameCol));
assertFalse(sm.isSelected(0, emailCol));
@@ -813,7 +814,7 @@ private void test_rt_38464_selectedColumnChangesWhenCellsInRowClicked(boolean ce
if (cellSelection) {
// Everything should remain the same, except the
// column of the single selected cell should change to lastNameCol.
assertFalse(sm.isSelected(0));
assertTrue(sm.isSelected(0));
assertFalse(sm.isSelected(0, firstNameCol));
assertTrue(sm.isSelected(0, lastNameCol));
assertFalse(sm.isSelected(0, emailCol));
26 changes: 23 additions & 3 deletions ...afx.controls/src/test/java/test/javafx/scene/control/TableViewSelectionModelImplTest.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2010, 2016, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2010, 2022, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -172,7 +172,7 @@ private TablePosition pos(int row, TableColumn<String,?> col) {
assertFalse(model.isSelected(1, col0));
assertFalse(model.isSelected(3, col0));
assertFalse(cells(model), model.isSelected(3, null));
assertFalse(model.isSelected(3));
assertTrue(model.isSelected(3));
assertEquals(1, model.getSelectedCells().size());
}

Comment on lines -175 to 180
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we have tests isSelected(index) for all combinations of single/multiple/cellSelection? If not, now might be a good time to add them :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a very good point!
should definitely cover all the scenarios.

@@ -185,7 +185,7 @@ private TablePosition pos(int row, TableColumn<String,?> col) {
assertFalse(model.isSelected(1, col0));
assertFalse(model.isSelected(3, col0));
assertFalse(cells(model), model.isSelected(3, null));
assertFalse(model.isSelected(3));
assertTrue(model.isSelected(3));
assertEquals(1, model.getSelectedCells().size());
}

@@ -852,4 +852,24 @@ private TablePosition pos(int row, TableColumn<String,?> col) {
model.clearSelection(2);
model.getSelectedItems().removeListener(listener);
}

/**
* Analysing failing tests when fixing JDK-8219720.
*
* Suspect: isSelected(int row) violates contract.
*
* @see #selectRowWhenInSingleCellSelectionMode()
* @see #selectRowWhenInSingleCellSelectionMode2()
*/
@Test
public void testSelectRowWhenInSingleCellSelectionModeIsSelected() {
model.setSelectionMode(SelectionMode.SINGLE);
model.setCellSelectionEnabled(true);
model.select(3);
// test against contract
assertEquals("selected index", 3, model.getSelectedIndex());
assertTrue("contained in selected indices", model.getSelectedIndices().contains(3));
// test against spec
assertEquals("is selected index", model.getSelectedIndices().contains(3), model.isSelected(3));
}
}