[External] : Re: [Request for Comments] Behavior / InputMap

[Andy Goryachev]

Dear John:

Thanks again for a detailed response!

I noticed a theme that perhaps we should discuss first.  It looks like there is a desire to have:


  1.  stateless, shared behavior
  2.  behavior separated from the skin
  3.  input map as a separate entity (or entity that can be used by any Node)
  4.  input maps are not control-specific
  5.  immutable input maps
  6.  whether changes to behavior span one control or all controls

so, in the order listed:

1. I understand the desire to have a stateless/shared behaviors comes mainly from trying to save a few thousand bytes (a noble goal, no question).  When we look at the current behaviors we can see that they are not stateless (TextInputControlBehavior.contextMenu, AccordionBehavior.focusModel, etc.)  Perhaps these can be redesigned to arrive at a “better FX framework”, but the way I see it we have one major constraint – nobody is going to do that in the foreseeable future.  Having announced this limitation, I’d say stateless/shared behaviors is not a requirement.

2. Behaviors are currently tightly coupled with the skin.  Separating the two would require additional APIs in Skin, and possibly in Behavior(Base).  I think we might explore this, likely for a subset of controls, in some distant future, but it is currently out of scope.  I don’t think this proposal makes it impossible – please correct me if I am wrong.

3. I would strongly argue against using InputMaps for anything other that Controls.  Making it a property of Control makes possible to customize key mappings on per-control basis regardless of the current skin/behavior, which is one of the requirements outlined in the proposal.

4. Input maps are (by design) control-specific, to allow for per-control customization.  For example, a RichTextArea might offer two instances working off the same data model with different editable areas or navigation rules.

5. I don’t know where this is coming from.  Runtime customization is one of the requirements (one possible use case was described in a previous message referring to Eclipse’s key settings UI).  Input map is mutable, mutable at runtime, and also supporting reverting back to the original mappings).

6. This is a different use case, that should be addressed by extensibility of the skin/behavior.  It is currently out of scope since this proposal does not require public behaviors, but nothing precludes us from making certain behaviors public.  A possible workaround might involve subclassing the control and modifying its input map.

Perhaps there is something I missed, please correct me.

-andy




From: John Hendrikx <john.hendrikx at gmail.com>
Date: Tuesday, October 10, 2023 at 08:00
To: Andy Goryachev <andy.goryachev at oracle.com>, openjfx-dev at openjdk.org <openjfx-dev at openjdk.org>
Subject: [External] : Re: [Request for Comments] Behavior / InputMap


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:

private static void buttonsRespondToEnter(Scene scene) {

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

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

Node node = scene.getFocusOwner();

if(node instanceof Button 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/6ed4e1e4/attachment-0001.htm>