8087555: [ChoiceBox] uncontained value not shown #191
The issue is that ChoiceBoxSkin
Added test for text updates that failed before the fix and pass after (note: there were no tests for the display text, so for coveragy, contains also tests that passed before as well as after)
I have not reviewed the code but have only tested it.
and similarly the difference of behavior can be observed with
This is not formally documented(nor decided), so we might need to decide on a value of index for an uncontained value.
Thanks for taking an initial look :) Just keep in mind that this issue is focused on displaying of the box' value reliably.
yeah the spec is rather .. well, suboptimal at some crucial points (see also JDK-8088012). To keep the state logically consistent, we need an invariant like:
Which seems to be hinted at in a code comment in
For ChoiceBox, that will be fixed in JDK-8241999 (ready for push as soon as this is integrated).
true, that's spec'ed in ComboBox only .. so by analogy I considered it being the same here. Hmm .. might need an update of the doc?
exactly that is JDK-8241999
interesting, hadn't noticed yet - will add a test to the other fix, thanks :)
yeah, that's another issue .. selection issues are a bottomless pit ..
see the "natural" constraint above - without we get into hell, IMO.
Good points all, thanks again!
I agree that selectedIndex should be -1 for an uncontained value, so that the below condition always holds true. And this is already working well with ComboBox.
The fix for JDK-8241999 would solve most of the behavior differences of contained Vs uncontained item selection. (that I was looking at selectPrevious(), selectNext()).
Thanks for the the pointer, After reading it, I have a doubtful understanding about it as, that,
The spec is only about
And ComboBox does not follow this spec,
So I think, for ChoieBox, In addition to doc change we would need fix the behavior as well, and maintain same behavior of
But again this is a different issue and not related to this fix.
So the fix itself looks good to me along with the upcoming fix for JDK-8241999.
@kleopatra This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type
Since the source branch of this PR was last updated there have been 4 commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge
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 (@arapte, @aghaisas) but any other Committer may sponsor as well.
Your commit was automatically rebased without conflicts.
Pushed as commit ceb3fce.