On Sat, 29 Aug 2020 09:11:26 GMT, Ambarish Rapte <ara...@openjdk.org> wrote:
>> I haven't tested it, but it looks like it should work. I left a couple of
>> minor suggestions below.
>>
>> Would it be possible to add some tests to verify the behavior of HOME and
>> END for editable and non-editable ComboBox
>> controls?
>
>> Would it be possible to add some tests to verify the behavior of HOME and
>> END for editable and non-editable ComboBox
>> controls?
>
> @kevinrushforth @kleopatra
> Please check the updated changes. _ComboBoxTest_ and _ListViewTest_ both have
> minor modifications in how they access
> _KeyMappings_. and added test for verifying **HOME** and **END** key with
> both editable and non editable ComboBox.
hmm .. this is getting unwieldy, isn't it ;)
The pain points:
- cascade of listeners (editable -> comboSkin -> properties -> behavior)
- dynamic change (add/remove) of mappings
- multiple key/value pairs for basically the same - though variant - state
My suggestion would be to take a step back (in solution path): near the
beginning was the evaluation of using different
inputMaps for different state contexts. Which was not further evaluated because
it looked like we could get away with
simply configuring the mappings - based on certain condition - once at
instantiation time. Which has the advantage of
not touching too much code but unfortunely turned out to be not enough.
Meanwhile, I'm convinced that in the long run there is no way around different
inputMaps based on context: the
differences in behavior (stand-alone vs. editable combo-popup vs. not-editable
combo-popup) are many - f.i. focus-only
navigation doesn't make sense in the popup (should be selection navigation
always), left/right in a not-editable should
trigger selection navigation .. and certainly more. So we not only have to
enable/disable certain mappings, but also
re-map the triggered behavior.
That's too broad for this issue, but we could take a step into that direction:
use the InputMap/Mapping API to help -
it was designed for exactly such a differentiation :) The step would be to use
interceptors (instead of dynamic
modification of the mappings list), they are available on both inputMap and
mapping level. As a first step, we could
use the latter: keep the addition of mappings as-is (before the fix) and add
interceptors to mappings for
inclusion/exclusion based on context. No listeners, no dynamic modification,
just one marker in the properties ..
hopefully :)
Raw code snippets:
// let combo skin put a Supplier for editable as value
getProperties().put("comboContext", (Supplier<Boolean>) () ->
getSkinnable().isEditable());
// let listView behavior use the supplier to build interceptors
Supplier<Boolean> comboEditable = (Supplier<Boolean>)
control.getProperties().get("comboContext");
Predicate<KeyEvent> interceptIfInCombo = e -> comboEditable != null;
Predicate<KeyEvent> interceptIfInEditableCombo = e -> comboEditable != null
&& comboEditable.get();
if (comboEditable == null) {
// add focus traversal mappings if not in combo popup
addDefaultMapping(listViewInputMap,
FocusTraversalInputMap.getFocusTraversalMappings());
}
// add mappings with appropriate interceptors
addDefaultMapping(listViewInputMap,
// missing api in KeyMapping: no constructor taking KeyCode and
interceptor
new KeyMapping(new KeyBinding(HOME), e -> selectFirstRow(),
interceptIfInEditableCombo),
new KeyMapping(new KeyBinding(END), e -> selectLastRow(),
interceptIfInEditableCombo),
new KeyMapping(new KeyBinding(HOME).shift(), e ->
selectAllToFirstRow(), interceptIfInCombo),
new KeyMapping(new KeyBinding(END).shift(), e -> selectAllToLastRow(),
interceptIfInCombo),
...
With this, the tests for key navigation are passing, the low-level mapping
tests will have to be re-formulated to test
for not/intercepted vs. existence.
What do you think?
-------------
PR: https://git.openjdk.java.net/jfx/pull/172
