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

8258663: Fixed size TableCells are not removed from sene graph when column is removed #444

Closed
wants to merge 3 commits into from

Conversation

@Maran23
Copy link
Member

@Maran23 Maran23 commented Mar 28, 2021

This PR fixes an issue, where table cells are not removed from the table row when the corresponding table column got removed. This will lead to empty "ghost" cells laying around in the table.
This bug only occurs, when a fixed cell size is set to the table.

I also added 3 more tests beside the mandatory test, which tests my fix.
1 alternative test, which tests the same with no fixed cell size set.
The other 2 tests are testing the same, but the table columns are set invisible instead.

-> Either the removal or setVisible(false) of a column should both do the same: Remove the corresponding cells, so that there are no empty cells.
Therefore, the additional tests make sure, that the other use cases (still) works and won't break in future (at least, without red tests ;)).
See also: TableRowSkinBase#updateCells(boolean)


Progress

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

Issue

  • JDK-8258663: Fixed size TableCells are not removed from sene graph when column is removed

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 444

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

Using diff file

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

…emove cells when the corresponding column is removed
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 28, 2021

👋 Welcome back Maran23! 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 Mar 28, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 28, 2021

Webrevs

private TableRow<Person> setRowFactoryVerifyReturnFirstRow(boolean useFixedCellSize) {
// We save the first table row to check it later.
AtomicReference<TableRow<Person>> tableRowRef = new AtomicReference<>();

Copy link
Collaborator

@kleopatra kleopatra Mar 30, 2021

Choose a reason for hiding this comment

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

wondering a bit about this complicated test setup .. are you aware of the VirtualFlowTestUtils (in test.something.infrastructure)? Using it, a test would shrink down to something like:

@Test
public void testRemoveColumnsFixed() {
    tableView.setFixedCellSize(20);
    tableView.getColumns().remove(0, 2);
    Toolkit.getToolkit().firePulse();
    assertEquals(tableView.getVisibleLeafColumns().size(), 
            VirtualFlowTestUtils.getCell(tableView, 0).getChildrenUnmodifiable().size());
}

Or what am I missing?

Copy link
Member Author

@Maran23 Maran23 Mar 30, 2021

Choose a reason for hiding this comment

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

Nice catch! I tried it out and it works! And indeed the code looks much better now.
To be fair, I had a brief look at VirtualFlowTestUtils, as other table/cell tests uses it. Next time I should take a closer look. :P
I pushed your suggested change.

Copy link
Collaborator

@kleopatra kleopatra left a comment

Fix looks good, verified failing/passing test before/after fix. Left minor comments inline.

As to the test - good to increase test coverage by including tests for invisible columns, IMO :)

Two thingies you might consider (your decision, I'm fine either way)

  • test TreeTable also? Which doesn't seem to have the issue, so would be okay not to .. just for sanity.
  • parameterize the test in fixedSize false/true

TableColumnBase<T, ?> tableColumn = getTableColumn((R) cell);
if (!getVisibleLeafColumns().contains(tableColumn) || !tableColumn.isVisible()) {
Copy link
Collaborator

@kleopatra kleopatra Mar 31, 2021

Choose a reason for hiding this comment

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

coming back, now that I understand the overall logic :)

checking column.isVisible is not necessary: not being contained in the visibleLeafColumns implies that it is either not visible or not in the column hierarchy of table's columns.

Copy link
Member Author

@Maran23 Maran23 Mar 31, 2021

Choose a reason for hiding this comment

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

oh, right. It's called getVisibleLeafColumns() for a reason. 😃
I will remove the visibility check.

import static junit.framework.Assert.assertEquals;

Copy link
Collaborator

@kleopatra kleopatra Mar 31, 2021

Choose a reason for hiding this comment

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

shouldn't that be org.junit.Assert.* ?

Copy link
Member Author

@Maran23 Maran23 Mar 31, 2021

Choose a reason for hiding this comment

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

Don't know, is there any kind of convention for that? Or is a static import fine?

Copy link
Member

@kevinrushforth kevinrushforth Mar 31, 2021

Choose a reason for hiding this comment

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

I don't think @kleopatra was referring to whether or not to use a wild-card import, but rather that the Assert class should be imported from the org.junit package instead of junit.framework. We are inconsistent in our usage in some of the older tests, but yes, we should use org.junit.

Copy link
Member Author

@Maran23 Maran23 Mar 31, 2021

Choose a reason for hiding this comment

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

Oh, alright, didn't saw that. I will change it. :)

// Remove the last 2 columns.
tableView.getColumns().remove(tableView.getColumns().size() - 2, tableView.getColumns().size() - 1);
Copy link
Collaborator

@kleopatra kleopatra Mar 31, 2021

Choose a reason for hiding this comment

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

code comment is not correct - the last parameter of remove(int, int) is exclusive. While it doesn't change the outcome (failing/passing before/after fix), it might confuse future readers :)

Copy link
Member Author

@Maran23 Maran23 Mar 31, 2021

Choose a reason for hiding this comment

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

Thanks, didn't recognized that. I will change it so that really the last 2 columns are removed.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Mar 31, 2021

/reviewers 2

@kevinrushforth kevinrushforth requested a review from aghaisas Mar 31, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 31, 2021

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

@Maran23
Copy link
Member Author

@Maran23 Maran23 commented Mar 31, 2021

Fix looks good, verified failing/passing test before/after fix. Left minor comments inline.

As to the test - good to increase test coverage by including tests for invisible columns, IMO :)

Two thingies you might consider (your decision, I'm fine either way)

* test TreeTable also? Which doesn't seem to have the issue, so would be okay not to .. just for sanity.

* parameterize the test in fixedSize false/true

I didn't wrote tests for TreeTableView, as they have no issue (and I'm lazy :P). But it might be a good idea to do so in future.

I chose not to make the tests parameterized so other people can add tests as well (without the parameterized 'dependency'). As far as I saw, you can't just define some tests to be parameterized (only the whole class is). I know that his is possible in JUnit5 with @MethodSource, but unfortunatly we run on JUnit4.

Copy link
Collaborator

@kleopatra kleopatra left a comment

looks good :)

@openjdk
Copy link

@openjdk openjdk bot commented Apr 8, 2021

@Maran23 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:

8258663: Fixed size TableCells are not removed from sene graph when column is removed

Reviewed-by: fastegal, aghaisas

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 10 new commits pushed to the master branch:

  • cc94e96: 8217955: Problems with touch input and JavaFX 11
  • 9796a83: 8263322: Calling Application.launch on FX thread should throw IllegalStateException, but causes deadlock
  • 5898858: 8263169: [macos] JavaFX windows open as tabs when system preference for documents is set
  • e63931e: 8262366: Update glib to version 2.66.7
  • b2f842d: 8259555: Webkit crashes on Apple Silicon
  • 052e9f7: 8258381: [macos] Exception when input emoji using Chinese input method
  • 015dad0: 8264330: Scene MouseHandler is referencing removed nodes
  • eec2f39: 8264536: Building OpenJFX on Apple AARCH64 not possible
  • f3e27a0: 8264162: PickResult.toString() is missing the closing square bracket
  • d80b8ad: 8264501: UIWebView for iOS is deprecated

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 (@kleopatra, @aghaisas) 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 Apr 8, 2021
@Maran23
Copy link
Member Author

@Maran23 Maran23 commented Apr 8, 2021

/integrate

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

@openjdk openjdk bot commented Apr 8, 2021

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

@aghaisas
Copy link
Collaborator

@aghaisas aghaisas commented Apr 8, 2021

/sponsor

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

@openjdk openjdk bot commented Apr 8, 2021

@aghaisas @Maran23 Since your change was applied there have been 11 commits pushed to the master branch:

  • 28475cb: 8263807: Button types of a DialogPane are set twice, returns a wrong button
  • cc94e96: 8217955: Problems with touch input and JavaFX 11
  • 9796a83: 8263322: Calling Application.launch on FX thread should throw IllegalStateException, but causes deadlock
  • 5898858: 8263169: [macos] JavaFX windows open as tabs when system preference for documents is set
  • e63931e: 8262366: Update glib to version 2.66.7
  • b2f842d: 8259555: Webkit crashes on Apple Silicon
  • 052e9f7: 8258381: [macos] Exception when input emoji using Chinese input method
  • 015dad0: 8264330: Scene MouseHandler is referencing removed nodes
  • eec2f39: 8264536: Building OpenJFX on Apple AARCH64 not possible
  • f3e27a0: 8264162: PickResult.toString() is missing the closing square bracket
  • ... and 1 more: https://git.openjdk.java.net/jfx/compare/ed5cfe72e0da4e4b90eb83cac7c50fe761f28c04...master

Your commit was automatically rebased without conflicts.

Pushed as commit d808dd1.

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

@Maran23 Maran23 deleted the 8258663-1 branch Jun 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants