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.

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

8273324: IllegalArgumentException: fromIndex(0) > toIndex(-1) after clear and select TableCell #617

Closed
wants to merge 3 commits into from

Conversation

@mstr2
Copy link
Collaborator

@mstr2 mstr2 commented Sep 3, 2021

This PR fixes the exception thrown by the sample code in 8273324, while retaining the incorrect behavior in the scenario described.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8273324: IllegalArgumentException: fromIndex(0) > toIndex(-1) after clear and select TableCell

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/617/head:pull/617
$ git checkout pull/617

Update a local copy of the PR:
$ git checkout pull/617
$ git pull https://git.openjdk.java.net/jfx pull/617/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 617

View PR using the GUI difftool:
$ git pr show -t 617

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/617.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 3, 2021

👋 Welcome back mstrauss! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Sep 3, 2021
@mstr2
Copy link
Collaborator Author

@mstr2 mstr2 commented Sep 3, 2021

It's quite hard to come up with a meaningful test for this fix. While I can reproduce the failure mode, "passing" the test would simply mean not failing with an exception. There are no meaningful assertions until the underlying problem is fixed.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 3, 2021

Webrevs

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Sep 3, 2021

The fix looks correct to me.

Can we get a unit test for this case?

/reviewers 2

@openjdk
Copy link

@openjdk openjdk bot commented Sep 3, 2021

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@jperedadnr
Copy link
Collaborator

@jperedadnr jperedadnr commented Sep 3, 2021

Looks good to me too.

It solves the exception, and also solves the incorrect null element that was logged:
Change: { [null] removed at 1 }, selected items: [] now reports correctly as Change: { [Person{a b}] removed at 0 }, selected items: [] ).

(However, the deselected cell is still showing as selected, but that is for JDK-8273336.)

@mstr2
Copy link
Collaborator Author

@mstr2 mstr2 commented Sep 3, 2021

Can we get a unit test for this case?

This is a test that fails without the fix, and passes with the fix. However, there are no assertions, which is... strange. Once JDK-8273336 is fixed, the test could validate the selection state.

    @Test
    public void test_jdk_8273324() {
        class Person {
            final String firstName, lastName;
            Person(String firstName, String lastName) {
                this.firstName = firstName;
                this.lastName = lastName;
            }
            public String getFirstName() { return firstName; }
            public String getLastName() { return lastName; }
        }

        var tableView = new TableView<Person>();
        tableView.getSelectionModel().setCellSelectionEnabled(true);
        tableView.getSelectionModel().setSelectionMode(SelectionMode.MULTIPLE);

        var col1 = new TableColumn<Person, String>();
        col1.setCellValueFactory(new PropertyValueFactory<>("firstName"));
        var col2 = new TableColumn<Person, String>();
        col2.setCellValueFactory(new PropertyValueFactory<>("lastName"));
        tableView.getColumns().addAll(List.of(col1, col2));

        var ab = new Person("a", "b");
        tableView.getItems().add(ab);
        var cd = new Person("c", "d");
        tableView.getItems().add(cd);

        tableView.getSelectionModel().select(0);
        tableView.getSelectionModel().clearAndSelect(0, col1);
    }

@Maran23
Copy link
Member

@Maran23 Maran23 commented Sep 4, 2021

Can we get a unit test for this case?

This is a test that fails without the fix, and passes with the fix. However, there are no assertions, which is... strange. Once JDK-8273336 is fixed, the test could validate the selection state.

That is still fine. I also don't like to write tests without at least one assertion, but there is not other way in JUnit4. I ended up writing a comment above the problematic call, e.g.: // Should not throw an NPE.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Sep 4, 2021

Your proposed unit test looks fine to me, and catches the bug. I agree with @Maran23 that as long as you add a comment that it should run without throwing NPE, it's OK to not have any asserts. One final point is that we have moved away from the practice of including the bug ID in the name of the method (or class, which we also used to do), and instead just use a meaningful name. You can add the bug ID in a comment.

@arapte
Copy link
Member

@arapte arapte commented Sep 7, 2021

The exception can be observed in one other similar scenario.

  1. Run the program attached to JDK-8273324.
  2. Click on a
  3. Shift + Click on d
  4. Click on d
    -> The same exception can be observed.

@jperedadnr
Copy link
Collaborator

@jperedadnr jperedadnr commented Sep 7, 2021

@arapte yes, you are right, I can reproduce it too after the proposed fix is applied.

For this case, insertionPoint = 2, so probably we need:

-if (insertionPoint == 0) {
+if (insertionPoint >= 0) {

?

@mstr2
Copy link
Collaborator Author

@mstr2 mstr2 commented Sep 7, 2021

@arapte yes, you are right, I can reproduce it too after the proposed fix is applied.

For this case, insertionPoint = 2, so probably we need:

-if (insertionPoint == 0) {
+if (insertionPoint >= 0) {

?

Yes, basically all cases in which the cleared rows contain the retained row.

selectionModel.select(0);
selectionModel.clearAndSelect(0, col1);

// The following asserts should work once JDK-8273336 is fixed:
Copy link
Collaborator

@jperedadnr jperedadnr Sep 7, 2021

Choose a reason for hiding this comment

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

@mstr2 About this issue, I've added a comment, in case you have some time to check it.
With my possible fix, these assertions work fine, but fail without it.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

The updated fix and new test both look good.

@mstr2 mstr2 requested a review from jperedadnr Sep 8, 2021
Copy link
Collaborator

@jperedadnr jperedadnr left a comment

Looks good to me

@openjdk
Copy link

@openjdk openjdk bot commented Sep 8, 2021

@mstr2 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8273324: IllegalArgumentException: fromIndex(0) > toIndex(-1) after clear and select TableCell

Reviewed-by: jpereda, kcr

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 4 new commits pushed to the master branch:

  • 26d6438: 8273138: BidirectionalBinding fails to observe changes of invalid properties
  • 5552213: Merge
  • 24d0600: 8273343: Create release notes for JavaFX 17
  • 78ae4a8: 8269871: CellEditEvent: must not throw NPE in accessors

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@jperedadnr, @kevinrushforth) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label Sep 8, 2021
@mstr2
Copy link
Collaborator Author

@mstr2 mstr2 commented Sep 8, 2021

/integrate

@openjdk openjdk bot added the sponsor label Sep 8, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 8, 2021

@mstr2
Your change (at version c853c3e) is now ready to be sponsored by a Committer.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Sep 8, 2021

/sponsor

@openjdk
Copy link

@openjdk openjdk bot commented Sep 8, 2021

Going to push as commit a272c4f.
Since your change was applied there have been 4 commits pushed to the master branch:

  • 26d6438: 8273138: BidirectionalBinding fails to observe changes of invalid properties
  • 5552213: Merge
  • 24d0600: 8273343: Create release notes for JavaFX 17
  • 78ae4a8: 8269871: CellEditEvent: must not throw NPE in accessors

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Sep 8, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 8, 2021

@kevinrushforth @mstr2 Pushed as commit a272c4f.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants