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
8307960: Create Table Column PopupMenu lazily #1133
Conversation
👋 Welcome back mhanl! A progress list of the required criteria for merging this PR into |
Reviewer: @andy-goryachev-oracle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a couple very minor style-related comments
@@ -32,4 +34,11 @@ public static TableColumnHeader getColumnHeaderFor(TableHeaderRow tr, final Tabl | |||
return tr.getColumnHeaderFor(col); | |||
} | |||
|
|||
public static ContextMenu getColumnPopupMenu(TableHeaderRow tableHeaderRow) { | |||
return tableHeaderRow.getColumnPopupMenu(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: I would keep tr
instead of tableHeaderRow
for consistency with already existing static method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would too - avoids wrapping the line, and there is no need to spell the name for a one line method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am more a fan of descriptive variable names. I know this is a one-line method - but on the other hand tr
is just not a good understandable name, even for a very short method.
Since you read code more often than you write it, I'm for keeping the descriptive name, also in this simple case :)
} | ||
|
||
public static Pane getCornerRegion(TableHeaderRow tableHeaderRow) { | ||
return tableHeaderRow.getCornerRegion(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above
There is also a similar property in |
Verified that the code works as expected with Tree- and TableView, with no ill effects (on Mac), using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to add test(s) for TreeTableView? Otherwise, looks good.
edit: tests are there, I must be blind!
@@ -32,4 +34,11 @@ public static TableColumnHeader getColumnHeaderFor(TableHeaderRow tr, final Tabl | |||
return tr.getColumnHeaderFor(col); | |||
} | |||
|
|||
public static ContextMenu getColumnPopupMenu(TableHeaderRow tableHeaderRow) { | |||
return tableHeaderRow.getColumnPopupMenu(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would too - avoids wrapping the line, and there is no need to spell the name for a one line method.
@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:
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 2 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the ➡️ To integrate this PR with the above commit message to the |
@Maran23 I'll re-approve if you want to make minor changes. |
/integrate |
Going to push as commit 8aff525.
Your commit was automatically rebased without conflicts. |
This PR changes the
columnPopupMenu
, so that it is created lazily.The problem here is, that the
columnPopupMenu
is always initialized and updated via bindings, even if the table menu button is never shown (setTableMenuButtonVisible(false)
) or the user never clicked on it.This problem can be solved by creating the
columnPopupMenu
and related bindings when it should be shown the first time.I also added many tests to ensure that everything still works (there are no tests for that area as of now).
Side note: There are a bunch of tickets with the wish to customize the Popup shown by the table menu button or show it programmatically. This ticket will prepare this, as now all Popup related code is on one place and in the future we can think of implementing a way to override this behaviour in a way that the Popup and all related bindings are never created and therefore do not decrease performance.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1133/head:pull/1133
$ git checkout pull/1133
Update a local copy of the PR:
$ git checkout pull/1133
$ git pull https://git.openjdk.org/jfx.git pull/1133/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1133
View PR using the GUI difftool:
$ git pr show -t 1133
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1133.diff
Webrev
Link to Webrev Comment