8350048: Enforce threading restrictions for show and hide methods in Window, Control, and Skin #1717
Conversation
|
👋 Welcome back angorya! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
/reviewers 2 reviewers |
|
@kevinrushforth |
|
@kevinrushforth has indicated that a compatibility and specification (CSR) request is needed for this pull request. @andy-goryachev-oracle please create a CSR request for issue JDK-8350048 with the correct fix version. This pull request cannot be integrated until the CSR request is approved. |
|
Reviewers: @kevinrushforth @arapte |
There was a problem hiding this comment.
The API doc changes look good, although I noted a couple additional changes that are needed.
The API docs for Dialog::hide should also be documented to throw ISE (since it calls close() directly the implementation is fine)
The API docs for Stage::close should also be documented to throw ISE (since it calls hide() directly the implementation is fine)
For ComboBoxBase, were you going to get rid of the word "aspect" in the opening sentence "... display the popup aspect of the user interface."? I think that's a little awkward even with that word removed. Maybe it could just be: "... display the popup associated with this control."
As for the implementation, There are a few places where the skin will call hide(), and I can't tell by just looking at the code that they are all guaranteed to be on the FX app thread.
For example, I recommend checking the following:
MenuBarSkinline 799ComboBoxListViewSkinline 199
There may be others.
| * | ||
| * This test ensures that the threading restrictions are in place where required. | ||
| */ | ||
| @TestMethodOrder(MethodOrderer.MethodName.class) |
There was a problem hiding this comment.
This is unnecessary (and not a usual practice).
|
|
||
| test(TPopupWindow::new, (p) -> p.show(stage)); | ||
| test(TPopupWindow::new, (p) -> p.show(stage, 0, 0)); | ||
| test(TPopupWindow::new, (p) -> p.show(contentPane, 0, 0)); |
There was a problem hiding this comment.
It's probably worth adding a test for hide when it isn't showing (like you've done for other objects).
| Util.runAndWait(() -> { | ||
| p.show(); | ||
| }); | ||
| p.hide(); // do we need to fail early here, or only when showing? |
There was a problem hiding this comment.
We should fail early (although this comment seems misplaced, since it is the case where it is showing).
| } | ||
|
|
||
| @Test | ||
| @Timeout(value = 1, unit = TimeUnit.DAYS) |
There was a problem hiding this comment.
Um, 1 DAY???
Seriously, though: Remove the timeout (it isn't needed or wanted).
TestThreadingRestrictionsNote to the Reviewers
To avoid merge conflicts, the preferred order of integrations:
#1697
#1713
#1717
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1717/head:pull/1717$ git checkout pull/1717Update a local copy of the PR:
$ git checkout pull/1717$ git pull https://git.openjdk.org/jfx.git pull/1717/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1717View PR using the GUI difftool:
$ git pr show -t 1717Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1717.diff
Using Webrev
Link to Webrev Comment