RFR: 8209788: Left/Right/Ctrl+A keys not working in editor of ComboBox if popup showing

[Jeanette Winzenburg]

On Wed, 29 Apr 2020 07:06:49 GMT, Ambarish Rapte <arapte at 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 :)
>
>> 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