On Wed, 29 Apr 2020 07:06:49 GMT, Ambarish Rapte <[email protected]> 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 :)
>
>> 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 :)
>
> Thanks for the suggestion 👍 , I shall try this approach and update the PR. I
> am not sure if we already do this for any
> other control. Do you know any, if we do ? Not actively working on this
> issue, Will soon get back on this :)
the nearest to different input maps based on control state might be in
listViewBehavior itself: it has differrent child
maps for vertical/horizontal orientation. Could be possible to widen that a bit
with another child map for vertical and
in combo popup (provided it has a means to decide being in such a state for the
sake of an interceptor, without api
change that might be a simple entry in its properties)
-------------
PR: https://git.openjdk.java.net/jfx/pull/172
