[Request for Comments] Behavior / InputMap

[John Hendrikx]

On 09/10/2023 22:24, Andy Goryachev wrote:
>
> Dear John:
>
> Thank you for a very thoughtful analysis! I wanted to initiate a 
> discussion, with the goal of making FX better.  Let me try to respond 
> to your comments.
>
> The main odd thing that jumps out immediately is the fact that 
> Behaviors and InputMaps need a reference to their linked Node. This 
> will result in chicken-egg style problems where you can't create the 
> Control without an InputMap, and you can't create an InputMap without 
> a Control.  It will also lead to problems where if a Behavior is 
> replaced that we must take care to dispose it, or use only weak 
> references to the Control.
>
> If we were to place FX parts in the standard MVC paradigm, behavior is 
> the controller, skin is the view, and the model is a part of control 
> (at least in the case of TextInputControl).  In FX world I would say 
> the control represents a façade which provides programmatic access to 
> the control functionality to the app developers. Since the behavior 
> needs access to both the view and the model, it having a pointer to 
> the control is not an issue.
>
The MVC paradigm is not really relevant here, and has been superceded by 
much better paradigms; what we need is good abstractions that are easy 
to use and reason about.  Attaching a Behavior should be a simple one 
step process:

      void setBehavior(Behavior behavior);

This has repercussions for how Behaviors should be designed; first it 
should not take the control as a contructor parameter (and nor should 
InputMap).  This isn't needed; the context the Behavior needs to do its 
job is provided through Events.

After calling the above setter, the Control will first call the old 
Behavior to remove its hooks, and then call the new Behavior to install 
the necessary hooks on the control.  The Control is in the lead here, as 
it should be given that it is what actually represents a control.  This 
also means that an interface is needed, something like:

      interface Behavior<T extends Control> {
            Subscription addBehaviorListeners(T control);
      }

As you can see, the control is passed here, allowing the Behavior to 
either: keep track of the control with the listeners it registers (but 
don't store it in a field), or only use the control to install the 
listeners and rely on the Event#getSource or Observable passed to 
listeners if it needs the control.  This latter option allows all the 
installed listeners to be shared amongst all controls -- as Behaviors 
often install quite a few listeners, let's not underestimate how much 
resources this could potentially save.  Duplicating all these just to 
keep track of a different control is not the high level of optimization 
I've come to expect from JavaFX.

The above also means that a Behavior is stateless; all its state is 
captured in the listeners it registers (if any is needed). Stateless 
objects can be shared, so I can do:

       TextField f1, f2;
       Behavior behavior = new MyBehavior();

       f1.setBehavior(behavior);
       f2.setBehavior(behavior);

... or for cell factories, a single behavior can be set on all cells 
created.

> With the InputMap having a pointer – it’s a convenience.  I mean, we 
> could hide the input map altogether and add a bunch of methods to the 
> Control for manipulating the key bindings – would that be better?  I 
> personally don’t think so, but it’s definitely an option.
>
A Control always has an InputMap.  This means any methods InputMap 
exposes are available if you have a Control.  Effectively this means the 
methods could have been put on Control as well (they'd just be 
shortcuts), but to avoid clutter it is nicer to put them on a new 
concept, the InputMap.  Such a new concept should be easy to use for 
anybody now, not just for Controls when InputMap was just an 
implementation detail.  An InputMap, when it was an implementation 
detail, can have a "convenient" pointer back to the control, as nobody 
could use InputMaps before and nobody could construct them incorrectly.  
That changes when you make it public, and so its design should be 
reconsidered.

Mirroring the above, an InputMap can be constructed without providing a 
Control, mappings can be added and removed (or just provided in a 
constructor immedaitely), and then such an InputMap can be set with a 
single setter on a control.  The only state it needs is its mappings, 
and as long as you want the same mappings, the same InputMap can be 
shared amongst however many controls you'd want.  Sample:

       InputMap map = new InputMap(Map.of(KEY_A, FunctionKey.of("type A"));

       control.setInputMap(map);

       // control is in the lead, and first removes any mappings from 
the previous InputMap,
       // then calls methods on InputMap to get the new mappings installed


> There is no problem (anymore) with disposal or life cycle of skins and 
> associated behaviors: there is Skin.install() and Skin.dispose().
>
Skins use this solution because there was no other option anymore as 
their API was already public.  It is however not a good solution, but 
the best possible given the restrictions imposed by API's that were 
already public.  I'm not criticizing that solution.

Skins could however have been much easier to use if they had foregone 
their pointer to the control, and if the API was not public yet (or if 
we were willing to create a SkinV2 imlementation).  A simple 
"setSkin(STATIC_SKIN)" would have sufficed.  Skins would then be called 
to install their hooks with a reference to the control.  The hooks 
themselves are passed the control on every callback (through Observable) 
but could in theory also track this information by capturing the control 
variable:

       class MySkin implements Skin<T> {
           // called by the control
           Subscription applySkin(T control) {
                   return Subscription.combine(
                          control.backgroundProperty().subscribe(v -> {
                                // do something with captured control 
here, or use a listener's Observable
                          })
                   };
           }
      }

> The Skin API suffers from this problem, and I think we can do better.
>
> Let’s keep in mind that a major redesign which will create a 
> compatibility issues for all the controls and their skins is most 
> likely not an option, but I would be very interested to hear how we 
> can do it better within the constraints of the existing design.
>
You misunderstand, I'm not criticizing the solution for Skins here, the 
current solution is the best that could be done given the API's that 
were already public and in place.

Behaviors and InputMap however are not public yet, and so I think their 
design should be carefully considered.

> Looking at InputMap first, I see no real reason why it needs the Node
>
> Mainly for convenience, and to prevent the user from supplying 
> completely unrelated control reference. Yes, it’s a waste of a 
> pointer, but the alternative is to move the APIs to Control.
>
If InputMaps are not control specific, then this becomes irrelevant.  
I'm not sure why you'd think that the only alternative then would be to 
put these API's on Control.
>
> Subscription installEventHandlers(Node node);
>
> Note that the above returns a Subscription, which can be used to 
> remove the event handlers again without requiring an interaction with 
> the InputMap.
>
> What would prevent the user from supplying a totally unrelated Node 
> (or rather, Control)?
>
Nothing, it does not need to be prevented.  The user already has full 
access to the Control and can do anything to that Control, including 
installing their own event handlers to create a whole different InputMap 
system.  Calling it the "wrong" way just installs some (more) handlers 
without the Control being in the lead; to uninstall these, the user 
needs to call `unsubcribe` themselves.  If you however want to make 
things easy for yourself and you wish to fully replace the standard 
InputMap, then you'd call `Control.setInputMap`.  If you just want to 
add a few bindings, you can either create a full fledged subclass of an 
existing InputMap you like, or provide a partial InputMap that you 
install directly yourself.  Or of course you can keep the option to add 
separate mappings.

The same applies to Skins really.  A control can be skinned without 
using the Skin system, all it takes is making a subclass to get access 
to (mutable) getChildren and layoutChildren.  This is done often enough 
to create new (adhoc) Controls with a new look without having to bother 
with creating a Skin as well.

> And, as it currently stands, there is no need to use Subscription, as 
> all the mappings created by the behavior will be automatically removed 
> with BaseBehavior.uninstall().  (And Subscription would waste more 
> bytes for each instance that one InputMap.control, wouldn’t you agree?)
>
A single Subscription can track as many resources as you'd like, so I 
don't think that's correct.  What would however save a lot of memory is 
if Behaviors were stateless, as all the listeners can then be re-used 
accross controls, only wrapping the uninstall logic in a Subscription.  
Effectively a Behavior would go from one behavior instance + X event 
handler instances per control to a single Behavior global instance, X 
stateless event handlers + 1 Subscription per control.  The Subscription 
effectively carries any state that Behaviors may need per Control instance.
>
> That brings us to InputMap's being mutable.  I would first spend some 
> serious effort to see if this is strictly needed
>
> Yes, one of the features the new design provides is ability to modify 
> key mappings by the user at runtime.  So yes, not only it needs to be 
> mutable, but it also adds some APIs for exactly that.
>
Having Immutable input maps does not preclude you from changing 
mappings. It would just mean that if there's is need for an InputMap 
with some mappings changed, that you'd create a new one to install on 
the control.

Something I've been wondering: what are the use cases currently? I would 
imagine that if I want to change a mapping for a certain type of control 
(let's say I want Buttons to react to <ENTER>), do I need to make a 
ButtonFactory that will call "addMapping" on every button I create?  Can 
I override the standard Button InputMap somewhere globally?  Will it be 
possible with CSS (so I can do it globally)?  Currently, I already can 
do this to affect all Buttons:

privatestaticvoidbuttonsRespondToEnter(Scene scene) {

scene.addEventHandler(KeyEvent.KEY_PRESSED, e -> {

if(!e.isConsumed() && e.getCode() == KeyCode.ENTER) {

Node node = scene.getFocusOwner();

if(node instanceofButton button) {

button.fire();

e.consume();

}

}

});

}

Will the new API be able to this as easily?

I struggle to think of a good use case for an addMapping method that 
only works for a single instance of a control.  In almost all cases I'd 
want to modify **all** controls of a certain type, or perhaps with a 
certain style associated with them.

> A shared InputMap (or, strictly speaking a shared InputMap and a 
> per-control InputMap) is an alternative that might have some benefit 
> in terms of memory consumption, at the expense of some complication.  
> It will, however, change the design as right now FX does not have such 
> a global map.  I can see how we can explore this avenue at a later 
> date, since it will not require public API changes as far as I can tell.
>
InputMap as it is currently requires a control in its constructor, that 
would preclude making them shareable later. Same for BehaviorBase.

Also they're not interfaces, they really should be if you want to design 
them now correctly to allow for user provided Behaviors and InputMaps later.

> First, I think there should be a real Behavior interface.  A required 
> abtract base class will just lead to the tempatation to use JavaFX 
> internals when things get inconvenient for internal Controls (using 
> the accessor pattern), and will result in users not being able to 
> replicate all the behaviors that FX controls can exhibit (as they 
> won't have access to the same internals).
>
> Can you explain what you mean?  I see no need for an interface, the 
> base class provides all the necessary methods to deal with the 
> control’s InputMap.  We are not (at this time) talking about making 
> the actual behaviors public as it is a much larger task, but nothing 
> precludes us from doing so in the future.  We do need the base class 
> public though.
>
You're making BehaviorBase public (it's not package private), and 
locking any Behavior classes into providing several protected methods 
(like getControl and getInputMap). This would preclude major design 
shifts later.  For example, I'm not so sure that a behavior should have 
a reference to the InputMap or the Control, at least not if you are 
introducing the FunctionTag mechanic.

> BehaviorBase immediately shows the same problem as InputMap. Even 
> though you'd expect that a Behavior should be sharable between 
> controls (it is after all the case that for most controls of a certain 
> type all their behaviors will be the same) it has elected to 
> explicitely couple itself to a specific control instance.  The base 
> class itself only does this because InputMap needs the Node, however, 
> subclasses are using this judiciously everywhere to access the Node 
> linked to a Behavior.
>
> I would _not_ expect a shared behavior.  This isn’t how FX works right 
> now, it isn’t how Swing works, and it is certainly not a requirement.  
> It _might_ be considered as an optimization in some cases, but the key 
> bindings and event handlers are certainly added on a per-control basis.
>
 From a user point of view, a Behavior is the same accross many 
controls.  How Swing does things does not figure into this.  The 
handlers are indeed added on a control basis, but they can be the same 
shared handlers.

> Looking at various uses of `getNode` (used in about 200-300 places) I 
> haven't found one yet that isn't responding to an event.
>
> Behavior, being a “controller” needs a pointer to the view and the 
> model, so having control instance (renamed from getNode()) is a 
> non-issue.  And nobody is going to rewrite 200-300 places, 
> realistically speaking.
>
Its responding to an event, which has this pointer, so this is basically 
disproven.

The "rewrite" would be a minor refactoring where to get the control 
pointer, certainly worth considering when introducing and committing to 
new API's forever.  I don't think all need to be adjusted immediately 
either.

>
> To summarize, I think that having control pointer in the InputMap or 
> BehaviorBase is a non-issue, there are no cycling dependencies or 
> chicken-egg problem because the life cycle is well determined.
>
It currently is well determined and guarded as they're private. This 
will not be the case when these classes/interfaces become public.  
Repeating the solution for Skins is sub-optimal and was necessity 
driven, so saying its a non-issue given the roundabout ways Skins 
currently need to be installed, and how we struggled to find a 
satisfactory solution there seems to me somewhat unfair.

The design should look much further ahead if it is truly a goal to keep 
evolving JavaFX into a better framework. Once these classes are public, 
they can't be changed, so we better make absolutely sure that if we 
indeed want to do things like sharing behaviors or inputmaps, or 
allowing user made custom behaviors, that that is indeed going to be 
possible with what is being made public now.

> Please don’t regard my responses as dismissals but rather as 
> clarifications, and most certainly I welcome further discussion.  I 
> think this proposal provides a good benefit-to-cost ratio, unless 
> somebody can find a scenario where things would definitely not work.
>
> What do you think?
>
I welcome the work, I just have this feeling we're not designing far 
enough ahead, and letting existing private code dictate the design. When 
it comes to API's, the costs of not thinking far enough ahead are 
massive, so I changing a bit of existing non-public code should never 
figure into a design choice.

--John
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/openjfx-dev/attachments/20231010/a0d34bd6/attachment-0001.htm>