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

8242489: ChoiceBox: initially toggle not sync'ed to selection #177

Closed
wants to merge 3 commits into from

Conversation

@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Apr 13, 2020

Macroscopic issue is that initially, the toggle is not sync'ed to the selection state. Root reason is an missing else block when updating toggle selection state (see report for details).

Fixed by introducing the else block and removing all follow-up errors that tried to amend the consequences of the incorrect selection state

  • removed listener to selected item
  • removed toggle selection update in showing listener

The former also fixed the memory leak when replacing the selectionModel plus an unreported NPE when the selectionModel is null initially.

Added tests that failed before the fix and passed after. As there had been no tests around toggle state, so added some to verify that the change doesn't break. Enhanced shim/skin to allow access to popup for testing. Removed the informally ignored test part for memory leak.


Progress

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

Issue

  • JDK-8242489: ChoiceBox: initially toggle not sync'ed to selection

Reviewers

  • Ajit Ghaisas (aghaisas - Reviewer)
  • Ambarish Rapte (arapte - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jfx pull/177/head:pull/177
$ git checkout pull/177

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 13, 2020

👋 Welcome back fastegal! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request.

@openjdk openjdk bot added the rfr label Apr 13, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 13, 2020

Webrevs

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Apr 13, 2020

@aghaisas can you review this?

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Apr 14, 2020

/reviewers 2

@openjdk
Copy link

@openjdk openjdk bot commented Apr 14, 2020

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

Copy link
Collaborator

@aghaisas aghaisas left a comment

Code changes and test look OK to me.
I have a minor comment that I have listed separately.

@@ -182,6 +182,7 @@ public ChoiceBox(ObservableList<T> items) {
oldSM = sm;
if (sm != null) {
sm.selectedItemProperty().addListener(selectedItemListener);
// unfixed part of JDK-8090015 - why exclude null?

This comment has been minimized.

@aghaisas

aghaisas Apr 15, 2020
Collaborator

Add a TODO: or FIXME: if you intend to work on it. Also, it will be better to create a JBS issue.

If not - please remove the comment.

This comment has been minimized.

@kleopatra

kleopatra Apr 15, 2020
Author Collaborator

Actually forgot that comment, already filed and took the issue JDK-8242001
thanks, eagle eye :)

@kevinrushforth kevinrushforth self-requested a review Apr 15, 2020
Copy link
Member

@arapte arapte left a comment

The change looks good to me, however I have minor concern about selecting Separator, which should be taken in a follow on issue. It may need a change in Separator selection test here.

@@ -428,6 +412,7 @@ private void updateSelection() {
MenuItem selectedItem = popup.getItems().get(selectedIndex);
if (selectedItem instanceof RadioMenuItem) {
((RadioMenuItem) selectedItem).setSelected(true);
} else {
toggleGroup.selectToggle(null);

This comment has been minimized.

@arapte

arapte Apr 15, 2020
Member

The else part here means that user programmatically has selected a Separator or SeparatorMenuItem. The behavior in such scenario is not defined in doc but the methods, select(), selectNext(), selectPrevious() of ChoiceBox.ChoiceBoxSelectionModel imply that if index points to a Separator then a valid item should be selected. However these method do handle this correctly.
If these methods are implemented such that no Separator is allowed to be selected then this else part would not be needed and we might be able to remove the instanceof checks.

The fix in this PR looks good to me.
But we should also decide the behavior in above scenario and may be file a JBS.
If we decide that when a Separator is chosen for selection then the current selection should not be changed or a valid item should be selected, then the test related to Separator selection need to be changed. Or all of it can be handled in a follow on issue.

This comment has been minimized.

@kleopatra

kleopatra Apr 16, 2020
Author Collaborator

yeah, you are right:

a) the implementation of ChoiceBoxSelectionModel is broken when it comes to handling of unselectable items (such as Separator): next/previous try to move on, the others simply select. The implementation changed in fix of JDK-8088261 - before select(index) tried to handle it, after this was moved into next/previous. Arguably, the model can do what it wants without specification ;)

b) the skin is responsible to sync the selection state of its toggles with the state of model: if the selectedIndex/Item does not have a corresponding toggle (f.i. if it's a separator), all toggles must be unselected.

c) my test related to the Separator is broken - as you correctly noted, it will fail if a future implementation decides to select a really selectable item :)

My plan:

  1. do nothing for a (don't feel like filing yet another bug around selection ;) and b (the skin behaves correctly, I think)
  2. fix the test to be resistant against implementation changes of selectionModel

Thanks for the extensive review, very much appreciated :)

This comment has been minimized.

@kleopatra

kleopatra Apr 16, 2020
Author Collaborator

btw: just noticed that there are methods in ChoiceBoxSkin testing the fix for next/prev

@Test public void test_jdk_8988261_selectNext() {
@Test public void test_jdk_8988261_selectPrevious() {

the name look like they want to point to the corresponding issue .. but the id is incorrect: that id doesn't exist, should be 8088261 (spelling error, I think) - is it okay to change them to the right id?

This comment has been minimized.

@arapte

arapte Apr 16, 2020
Member

1. do nothing for a (don't feel like filing yet another bug around selection ;) and b (the skin behaves correctly, I think)

I am good with this. Though I will file a JBS for the correction in ChoiceBoxSelectionModel.
seletPrevious(), selectNext() need one more check value instanceof SeparatorMenuItem.
and similarly selectFirst() and selectLast() should be overridden correctly.
and I can't think of why select() was changed so may be rethink about it :).
We can discuss it again whenever we start fixing it.

2\. fix the test to be resistant against implementation changes of selectionModel

Thanks for link to JDK-8088261, As mentioned in this bug description, "Culprit is an incorrect override of select(int): it may reject the new index if that would select a separator, but it must not select an arbitrary index instead", So It is not sure to me what should select() do in such scenario.
So I think the test can also go as is, in case we change the behavior then test can be changed with it.

This comment has been minimized.

@arapte

arapte Apr 16, 2020
Member

should be 8088261 (spelling error, I think) - is it okay to change them to the right id?

That will be good to change, but not sure if as part of this bug. It will be unrelated to fix.

This comment has been minimized.

@kleopatra

kleopatra Apr 16, 2020
Author Collaborator

At the end and following your latest comments I did nothing Except adding code comments to the separator test and the else (toggle nulling) branch.

@@ -364,6 +343,11 @@ String getChoiceBoxSelectedText() {
return label.getText();
}

// Test only purpose
ContextMenu getChoiceBoxPopup() {

This comment has been minimized.

@arapte

arapte Apr 15, 2020
Member

I would recommend to prefix the method name with test_. It is not followed across, only TabPaneSkin has test_ prefixed method.

This comment has been minimized.

@kleopatra

kleopatra Apr 16, 2020
Author Collaborator

well, as TabPaneSkin is a singularity in controls (in graphics such a pattern seems to be wider-spread), I would prefer not to do it here - also because ChoiceBoxSkin has another test-only accessor (for label text) which doesn't. Might be a candidate for a general cleanup task - if we decide to really introduce such a naming convention (which I personally wouldn't like :)

@kevinrushforth kevinrushforth removed their request for review Apr 15, 2020
- added code comments to clarify test/implemenation intention
@arapte
arapte approved these changes Apr 16, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Apr 17, 2020

@kleopatra This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type /integrate in a new comment to proceed. After integration, the commit message will be:

8242489: ChoiceBox: initially toggle not sync'ed to selection

Reviewed-by: aghaisas, arapte
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /solves command.

Since the source branch of this PR was last updated there have been 19 commits pushed to 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 automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate 69e426659de3db9b189a2e9218c17546955d26f2.

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 (@aghaisas, @arapte) 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 17, 2020
@kleopatra
Copy link
Collaborator Author

@kleopatra kleopatra commented Apr 17, 2020

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Apr 17, 2020

@kleopatra
Your change (at version 01aa790) is now ready to be sponsored by a Committer.

@openjdk openjdk bot added the sponsor label Apr 17, 2020
@arapte
Copy link
Member

@arapte arapte commented Apr 20, 2020

/sponsor

@openjdk openjdk bot closed this Apr 20, 2020
@openjdk openjdk bot added integrated and removed sponsor ready labels Apr 20, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Apr 20, 2020

@arapte @kleopatra The following commits have been pushed to master since your change was applied:

Your commit was automatically rebased without conflicts.

Pushed as commit 69e4266.

@openjdk openjdk bot removed the rfr label Apr 20, 2020
@kleopatra kleopatra deleted the kleopatra:bug-fix-JDK-8242489 branch May 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants