On Tue, 4 Aug 2020 10:07:14 GMT, Jeanette Winzenburg <faste...@openjdk.org>
wrote:
>> good that we agree on where to put it :)
>>
>> Will have to think about about the remove installed mappings vs. change
>> existing code to not installing. Right now,
>> feels a bit smelly .. but I see the constraints (and might agree :) Will
>> come back!
>
> btw, using a key-value pair in properties is just fine with me (might have
> been unclear, sry) - nothing else to do
> short of changing api (which feels too heavy weight)
> One nit-pit for checking for its existence, an alternative slightly
> simplified pattern might be
>
> if (Boolean.TRUE.equals(control.getProperties().get(....)) {
> removeMappings(...);
> }
okay, I still favor the not-adding-if-not-needed approach - and it's not _that_
much of a change to existing code.
Basically, in the constructor of ListViewBehavior
- in a first block, do not add the mappings that (currently) are removed again
if the combo flag is set
-in a second block, add the mappings for a stand-alone listView to all
inputMaps as needed
code snippets:
// not needed if in combo popup
// addDefaultMapping(listViewInputMap,
FocusTraversalInputMap.getFocusTraversalMappings());
addDefaultMapping(listViewInputMap,
// not needed if in combo popup
// new KeyMapping(HOME, e -> selectFirstRow()),
...
// add those that are always needed
}
// same for child input maps
addDefaultMappings(verticalInputMap, ...
// add those for stand-alone listView
// handle mappings that are needed only in a stand-alone listView
if
(!Boolean.TRUE.equals(control.getProperties().get("removeKeyMappingsForComboBoxEditor")))
{
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(A).shortcut(), e ->
selectAll()),
new KeyMapping(new KeyBinding(HOME).shortcut(), e ->
focusFirstRow()),
new KeyMapping(new KeyBinding(END).shortcut(), e ->
focusLastRow())
);
addDefaultMapping(verticalListInputMap,
new KeyMapping(new KeyBinding(HOME).shortcut().shift(), e
-> discontinuousSelectAllToFirstRow()),
new KeyMapping(new KeyBinding(END).shortcut().shift(), e ->
discontinuousSelectAllToLastRow())
);
}
Tests are passing as expected, and the [open issue
JDK-8250807](https://bugs.openjdk.java.net/browse/JDK-8250807)
doesn't stand in the way :)
As to testing, independent of which approach is chosen at the end: I would
suggest to not only test the macroscopic
behavior (look if key events on combo/listView have the expected effect) but
also test the base layer, that is whether
or not the mappings are not/added to the inputMaps.
One open (maybe not-an) issue might be the handling of the horizontal map which
currently is not touched (but probably
will, once the deep removal of
[JDK-8250807](https://bugs.openjdk.java.net/browse/JDK-8250807) is fixed) :
while not
very probable, client code might change the listView in the popup to be
horizontal. What would be the expected UI in
that case? Anyway, could be left open for the scope of this issue, IMO.
-------------
PR: https://git.openjdk.java.net/jfx/pull/172
