[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Re: RFR: 8209788: Left/Right/Ctrl+A keys not working in editor of ComboBox if popup showing
From:       Jeanette Winzenburg <fastegal () openjdk ! java ! net>
Date:       2020-04-29 9:26:42
Message-ID: BfKbIwV3Ht_ZHkjcXkx68uTzzN0J8_yO1ks2HlyQBBw=.d9ce2381-f432-4fe9-ac8e-1a5e78e8679d () github ! com
[Download RAW message or body]

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


[prev in list] [next in list] [prev in thread] [next in thread]