On Wed, 22 Apr 2020 06:43:31 GMT, Abhinay Agarwal
<github.com+3197675+abhinayagar...@openjdk.org> wrote:
> Don't you think that all the changes in `ListViewSkin` can be moved to
> `ListViewBehavior`? All that we do in the skin
> class is to call `ListViewBehavior#updateSelectionModeKeyMapping`, which
> smells like feature envy.
> Moreover, `ListViewBehavior` already has change listener attached to
> `selectionModelProperty`, waiting for us to re-use
> it 😉
good point :) Though - I don't like listeners to control properties in
behaviors, and much less listeners to path
properties (they tend to not getting cleaned on dispose).
In the particular case of behaviors of controls with selectionModels they do
(must?) because the selectionModel is not
api complete (having no notion of anchor), so they jump in to cover up.
Hopefully that design flaw will be fixed at
some time in future, which would remove the existing listener, anyway. With
just another responsibility - difference
based on selectionMode - such cleanup would be harder.
Here the basic approach is to add/remove a keyMapping for multiple/single
selection. Compared to current state, there's
a subtle side-effect (the event bubbles up if the mapping if removed). We can
achieve the same behavior (with the same
side-effect) by making the mapping consume depending on whether it is handled
(to select all) or not.
In code that would be pattern like:
// in constructor
KeyMapping selectAllMapping;
addDefaultMapping(listViewInputMap,
...
selectAll = new KeyMapping(new KeyBinding(A).shortcut(), this::
selectAll),
...
};
selectAllMapping.setAutoConsume(false);
// selectAll with modified signature
/**
* Calls selectAll on selectionModel and consumes the event, if
* the model is available and in multimode,
* does nothing otherwise.
*/
private void selectAll(KeyEvent key) {
MultipleSelectionModel<T> sm = getNode().getSelectionModel();
// do nothing, let it bubble up
if (sm == null || sm.getSelectionMode() == SelectionMode.SINGLE) return;
// handle and consume
sm.selectAll();
key.consume();
}
BTW, there are other keys that don't work as expected (from the perspective of
the editor in the combo): f.i.
shift-home/end is mapped to scrollToFirst/LastRow - that's hampering ux if f.i.
the user has typed some input, uses
them and sees her input lost because first/last row is selected. Sry to not
have noticed earlier in my bug report :(
So whatever approach we choose (mappings being removed/added or their handlers
not/consuming doesn't matter), we would
have to do it for several keys. Plus we have the side-effect mentioned above.
The mass of change _for all listviews_
has a certain risk of breaking existing code. Think f.i. global accelerators
that might (or not) get triggered
depending on selection mode.
On the other hand, different mappings are needed only when the list resides in
the combo's popup (and probably only if
the combo is editable, didn't dig though). An alternative might be a different
inputMap (or containing different
mappings) when used in combo's popup (which is similar to what Swing/X does ..
no wonder I would prefer it :)
-------------
PR: https://git.openjdk.java.net/jfx/pull/172
