On Tue, 1 Sep 2020 13:59:06 GMT, Ambarish Rapte <[email protected]> wrote:
>> 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?
>
>> What do you think?
>
> Suggestion looks promising, I shall try it and update.
@kleopatra @kevinrushforth
Change looks pretty with interceptor.
Please take a look.
-------------
PR: https://git.openjdk.java.net/jfx/pull/172
