Skip to content

/ jfx

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

Jump to bottom

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

Closed
wants to merge 3 commits into from
+354 −28
Closed

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

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
017be22
8242489: ChoiceBox: initially toggle not sync'ed to selection
kleopatra Mar 19, 2020
0d3ce12
ChoiceBox: added FIXME with reference to issue
kleopatra Apr 15, 2020
01aa790
ChoiceBox/-Skin/-SelectionTest
kleopatra Apr 16, 2020
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

1 modules/javafx.controls/src/main/java/javafx/scene/control/ChoiceBox.java
View file
Open in desktop
@@ -182,6 +182,7 @@ public ChoiceBox(ObservableList<T> items) {
oldSM = sm;
if (sm != null) {
sm.selectedItemProperty().addListener(selectedItemListener);
// FIXME JDK-8242001 - must sync to model state always
if (sm.getSelectedItem() != null && ! valueProperty().isBound()) {
ChoiceBox.this.setValue(sm.getSelectedItem());
}
33 modules/javafx.controls/src/main/java/javafx/scene/control/skin/ChoiceBoxSkin.java
View file
Open in desktop
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2010, 2016, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2010, 2020, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -28,9 +28,6 @@
import com.sun.javafx.scene.control.ContextMenuContent;
import com.sun.javafx.scene.control.behavior.BehaviorBase;
import javafx.beans.WeakInvalidationListener;
import javafx.scene.Node;
import javafx.scene.control.Accordion;
import javafx.scene.control.Button;
import javafx.scene.control.Control;
import javafx.scene.control.SkinBase;
import javafx.util.StringConverter;
@@ -156,20 +153,11 @@ public ChoiceBoxSkin(ChoiceBox<T> control) {
registerChangeListener(control.selectionModelProperty(), e -> updateSelectionModel());
registerChangeListener(control.showingProperty(), e -> {
if (getSkinnable().isShowing()) {
MenuItem item = null;

SelectionModel sm = getSkinnable().getSelectionModel();
SelectionModel<T> sm = getSkinnable().getSelectionModel();
if (sm == null) return;

long currentSelectedIndex = sm.getSelectedIndex();
int itemInControlCount = choiceBoxItems.size();
boolean hasSelection = currentSelectedIndex >= 0 && currentSelectedIndex < itemInControlCount;
if (hasSelection) {
item = popup.getItems().get((int) currentSelectedIndex);
if (item != null && item instanceof RadioMenuItem) ((RadioMenuItem)item).setSelected(true);
} else {
if (itemInControlCount > 0) item = popup.getItems().get(0);
}

// This is a fix for RT-9071. Ideally this won't be necessary in
// the long-run, but for now at least this resolves the
@@ -201,15 +189,6 @@ public ChoiceBoxSkin(ChoiceBox<T> control) {
label.setText(""); // clear label text when selectedIndex is -1
}
});
registerChangeListener(control.getSelectionModel().selectedItemProperty(), e -> {
if (getSkinnable().getSelectionModel() != null) {
int index = getSkinnable().getSelectionModel().getSelectedIndex();
if (index != -1) {
MenuItem item = popup.getItems().get(index);
if (item instanceof RadioMenuItem) ((RadioMenuItem)item).setSelected(true);
}
}
});
registerChangeListener(control.converterProperty(), e -> {
updateChoiceBoxItems();
updatePopupItems();
@@ -364,6 +343,11 @@ String getChoiceBoxSelectedText() {
return label.getText();
}

// Test only purpose
ContextMenu getChoiceBoxPopup() {

This comment has been minimized.

Sign in to view
@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.

Sign in to view
@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 :)
return popup;
}

private void addPopupItem(final T o, int i) {
MenuItem popupItem = null;
if (o instanceof Separator) {
@@ -428,6 +412,9 @@ private void updateSelection() {
MenuItem selectedItem = popup.getItems().get(selectedIndex);
if (selectedItem instanceof RadioMenuItem) {
((RadioMenuItem) selectedItem).setSelected(true);
} else {
// need to unselect toggles if selectionModel allows a Separator/MenuItem
// to be selected
toggleGroup.selectToggle(null);

This comment has been minimized.

Sign in to view
@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.

Sign in to view
@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.

Sign in to view
@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.

Sign in to view
@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.

Sign in to view
@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.

Sign in to view
@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.
}
// update the label
8 modules/javafx.controls/src/shims/java/javafx/scene/control/skin/ChoiceBoxSkinNodesShim.java
View file
Open in desktop
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2011, 2015, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2011, 2020, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -25,11 +25,17 @@

package javafx.scene.control.skin;

import javafx.scene.control.ContextMenu;

public class ChoiceBoxSkinNodesShim {

// can only access the getChoiceBoxSelectedText method in ChoiceBoxSkin
// from this package.
public static String getChoiceBoxSelectedText(ChoiceBoxSkin skin) {
return skin.getChoiceBoxSelectedText();
}

public static ContextMenu getChoiceBoxPopup(ChoiceBoxSkin skin) {
return skin.getChoiceBoxPopup();
}
}
ProTip! Use n and p to navigate between commits in a pull request.