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

8209788: Left/Right/Ctrl+A keys not working in editor of ComboBox if popup showing #172

Closed
wants to merge 13 commits into from
@@ -88,7 +88,7 @@ protected void addDefaultMapping(InputMap<N> inputMap, Mapping<?>... newMapping)

for (Mapping<?> mapping : newMapping) {
// check if a mapping already exists, and if so, do not add this mapping
// TODO this is insufficient as we need to check entire InputMap hierarchy
// TODO: JDK-8250807: this is insufficient as we need to check entire InputMap hierarchy
// for (Mapping<?> existingMapping : existingMappings) {
// if (existingMapping != null && existingMapping.equals(mapping)) {
// return;
@@ -116,6 +116,7 @@ protected void addDefaultMapping(InputMap<N> inputMap, Mapping<?>... newMapping)
}

protected void removeMapping(Object key) {
// TODO: JDK-8250807: Traverse the child maps of getInputMap() and remove the mapping from them.
InputMap<?> inputMap = getInputMap();
inputMap.lookupMapping(key).ifPresent(mapping -> {
inputMap.getMappings().remove(mapping);
@@ -26,55 +26,9 @@
package com.sun.javafx.scene.control.behavior;

import javafx.scene.control.ComboBox;
import javafx.scene.control.ComboBoxBase;
import javafx.scene.control.SelectionModel;
import com.sun.javafx.scene.control.inputmap.InputMap;

import static javafx.scene.input.KeyCode.DOWN;
import static javafx.scene.input.KeyCode.UP;

public class ComboBoxListViewBehavior<T> extends ComboBoxBaseBehavior<T> {

/***************************************************************************
* *
* Constructors *
* *
**************************************************************************/

/**
*
*/
public ComboBoxListViewBehavior(final ComboBox<T> comboBox) {
super(comboBox);

// Add these bindings as a child input map, so they take precedence
InputMap<ComboBoxBase<T>> comboBoxListViewInputMap = new InputMap<>(comboBox);
comboBoxListViewInputMap.getMappings().addAll(
new InputMap.KeyMapping(UP, e -> selectPrevious()),
new InputMap.KeyMapping(DOWN, e -> selectNext())
);
addDefaultChildMap(getInputMap(), comboBoxListViewInputMap);
}

/***************************************************************************
* *
* Key event handling *
* *
**************************************************************************/

private ComboBox<T> getComboBox() {
return (ComboBox<T>) getNode();
}

private void selectPrevious() {
SelectionModel<T> sm = getComboBox().getSelectionModel();
if (sm == null) return;
sm.selectPrevious();
}

private void selectNext() {
SelectionModel<T> sm = getComboBox().getSelectionModel();
if (sm == null) return;
sm.selectNext();
}
}
@@ -46,6 +46,8 @@

import java.util.ArrayList;
import java.util.List;
import java.util.function.Predicate;
import java.util.function.Supplier;

import static com.sun.javafx.scene.control.inputmap.InputMap.*;
import static javafx.scene.input.KeyCode.*;
@@ -78,12 +80,19 @@ public ListViewBehavior(ListView<T> control) {
listViewInputMap = createInputMap();

// add focus traversal mappings
addDefaultMapping(listViewInputMap, FocusTraversalInputMap.getFocusTraversalMappings());
Supplier<Boolean> isListViewOfComboBox =
(Supplier<Boolean>) control.getProperties().get("editableComboBox");
Predicate<KeyEvent> isInComboBox = e -> isListViewOfComboBox != null;
Predicate<KeyEvent> isInEditableComboBox =
e -> isListViewOfComboBox != null && isListViewOfComboBox.get();
This conversation was marked as resolved by arapte

This comment has been minimized.

@kleopatra

kleopatra Sep 9, 2020
Collaborator

nit-pick about naming: I think we don't use (crippled) hungarian notation, or do we? If we don't, the leading "p" should be removed, isIn/Editable/Combo is expressive enough (and no need to differentiate by type of functional interface, IMO)

This comment has been minimized.

@arapte

arapte Sep 11, 2020
Author Member

Even I was bit skeptical about prefix p, before it those names sounded like a boolean. So I wanted to give it a different name. But as you said, isIn/Editable/Combo looks enough expressive. Changed names.

if (isListViewOfComboBox == null) {
addDefaultMapping(listViewInputMap, FocusTraversalInputMap.getFocusTraversalMappings());
}
addDefaultMapping(listViewInputMap,
new KeyMapping(HOME, e -> selectFirstRow()),
new KeyMapping(END, e -> selectLastRow()),
new KeyMapping(new KeyBinding(HOME).shift(), e -> selectAllToFirstRow()),
new KeyMapping(new KeyBinding(END).shift(), e -> selectAllToLastRow()),
new KeyMapping(new KeyBinding(HOME), e -> selectFirstRow(), isInEditableComboBox),
new KeyMapping(new KeyBinding(END), e -> selectLastRow(), isInEditableComboBox),
new KeyMapping(new KeyBinding(HOME).shift(), e -> selectAllToFirstRow(), isInComboBox),
new KeyMapping(new KeyBinding(END).shift(), e -> selectAllToLastRow(), isInComboBox),
new KeyMapping(new KeyBinding(PAGE_UP).shift(), e -> selectAllPageUp()),
new KeyMapping(new KeyBinding(PAGE_DOWN).shift(), e -> selectAllPageDown()),

@@ -98,9 +107,9 @@ public ListViewBehavior(ListView<T> control) {
new KeyMapping(F2, e -> activate()),
new KeyMapping(ESCAPE, e -> cancelEdit()),

new KeyMapping(new KeyBinding(A).shortcut(), e -> selectAll()),
new KeyMapping(new KeyBinding(HOME).shortcut(), e -> focusFirstRow()),
new KeyMapping(new KeyBinding(END).shortcut(), e -> focusLastRow()),
new KeyMapping(new KeyBinding(A).shortcut(), e -> selectAll(), isInComboBox),
new KeyMapping(new KeyBinding(HOME).shortcut(), e -> focusFirstRow(), isInComboBox),
new KeyMapping(new KeyBinding(END).shortcut(), e -> focusLastRow(), isInComboBox),
new KeyMapping(new KeyBinding(PAGE_UP).shortcut(), e -> focusPageUp()),
new KeyMapping(new KeyBinding(PAGE_DOWN).shortcut(), e -> focusPageDown()),

@@ -145,10 +154,9 @@ public ListViewBehavior(ListView<T> control) {
new KeyMapping(new KeyBinding(DOWN).shortcut().shift(), e -> discontinuousSelectNextRow()),
new KeyMapping(new KeyBinding(PAGE_UP).shortcut().shift(), e -> discontinuousSelectPageUp()),
new KeyMapping(new KeyBinding(PAGE_DOWN).shortcut().shift(), e -> discontinuousSelectPageDown()),
new KeyMapping(new KeyBinding(HOME).shortcut().shift(), e -> discontinuousSelectAllToFirstRow()),
new KeyMapping(new KeyBinding(END).shortcut().shift(), e -> discontinuousSelectAllToLastRow())
new KeyMapping(new KeyBinding(HOME).shortcut().shift(), e -> discontinuousSelectAllToFirstRow(), isInComboBox),
new KeyMapping(new KeyBinding(END).shortcut().shift(), e -> discontinuousSelectAllToLastRow(), isInComboBox)
);

addDefaultChildMap(listViewInputMap, verticalListInputMap);

// --- horizontal listview
@@ -198,7 +206,6 @@ public ListViewBehavior(ListView<T> control) {
}



/***************************************************************************
* *
* Implementation of BehaviorBase API *
@@ -29,6 +29,7 @@
import com.sun.javafx.scene.control.behavior.ComboBoxListViewBehavior;

import java.util.List;
import java.util.function.Supplier;

import javafx.beans.InvalidationListener;
import javafx.beans.WeakInvalidationListener;
@@ -504,6 +505,9 @@ private void updateCellFactory() {

{
getProperties().put("selectFirstRowByDefault", false);
// editableComboBox property is used to intercept few Key inputs from this ListView,
// so that those inputs get forwarded to editor of ComboBox .
getProperties().put("editableComboBox", (Supplier<Boolean>) () -> getSkinnable().isEditable());
}

@Override protected double computeMinHeight(double width) {
ProTip! Use n and p to navigate between commits in a pull request.