-
Notifications
You must be signed in to change notification settings - Fork 546
8296653: ComboBox promptText is not displayed when the value is reset #1989
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
base: master
Are you sure you want to change the base?
Conversation
|
👋 Welcome back zelmidaoui! A progress list of the required criteria for merging this PR into |
|
@Ziad-Mid This change is no longer ready for integration - check the PR body for details. |
Webrevs
|
arapte
left a comment
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.
Manually tested the program attached to the bug. Change fixes the bug.
Can you please try for adding a test.
| if (empty) { | ||
| if (cell == null) return true; | ||
|
|
||
| // JDK-8296653 |
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.
We generally don't include the bug id. Please remove.
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.
May be we can add a comment to explain the change.
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 have updated the code could you please re review
arapte
left a comment
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.
Correcting the comment
|
I think a test would be good here. |
I have added one could you please re-review |
|
|
||
| @Test | ||
| public void testPromptTextRestoredAfterSetValueNull() { | ||
| comboBox.setPromptText("Select Value"); |
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 suggest to have a String variable with "Select Value" as value. And then use it here and the other two occurrences
| SingleSelectionModel<String> sm = comboBox.getSelectionModel(); | ||
| sl = new StageLoader(comboBox); | ||
| comboBox.applyCss(); | ||
| comboBox.show(); |
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.
applyCss and show are not needed, should be removed
| // Select an item | ||
| sm.select(2); | ||
| Toolkit.getToolkit().firePulse(); | ||
|
|
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.
An assert that the buttonCell is not Select Value would make sense here (assertNotEquals)
|
|
||
| if (cell == buttonCell) { | ||
| final String promptText = comboBox.getPromptText(); | ||
| if (comboBox.getValue() == null |
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.
is the comboBox.getValue() check needed? Asking, as we are in here only if empty is true, so maybe this is not needed to check?
The issue occurred because the skin marks the button cell as “empty” when the value becomes null.
With the fix clearing the ComboBox value re-shows the prompt text.
Tested using the code present in the bug.
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1989/head:pull/1989$ git checkout pull/1989Update a local copy of the PR:
$ git checkout pull/1989$ git pull https://git.openjdk.org/jfx.git pull/1989/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1989View PR using the GUI difftool:
$ git pr show -t 1989Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1989.diff
Using Webrev
Link to Webrev Comment