[Request for Comments] Behavior / InputMap

[Andy Goryachev]

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.

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.

There is no problem (anymore) with disposal or life cycle of skins and associated behaviors: there is Skin.install() and Skin.dispose().

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.

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.


     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)?

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?)

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.

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.


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.


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.

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.


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.

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?

-andy






From: openjfx-dev <openjfx-dev-retn at openjdk.org> on behalf of John Hendrikx <john.hendrikx at gmail.com>
Date: Saturday, October 7, 2023 at 10:43
To: openjfx-dev at openjdk.org <openjfx-dev at openjdk.org>
Subject: Re: [Request for Comments] Behavior / InputMap

I've read the proposal.

I think it would be good to take a step back first and look at the classes and interfaces involved: Control, Behavior, InputMap, Skin -- the time is now to take a good look at these before making more public.

They're currently very interwoven which is usually a sign that they are not well enough abstracted. They cyclically reference each other indicating they have overlapping concerns.  I think this should be addressed first before making the existing InputMap and BehaviorBase public.

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.  The Skin API suffers from this problem, and I think we can do better.

InputMap
========

Looking at InputMap first, I see no real reason why it needs the Node.  It is only used for installing and removing the event handlers.  This action should not be triggered by the InputMap, and instead should be inverted so it is triggered by the Control.  This can be achieved in multiple ways; InputMap could simply produce the required event handlers on demand:

     Map<EventType, EventHandler<?>> createEventHandlers();

The Control can then add these to the appropriate handlers, and track them for later removal (using a Subscription for example).

Or you could ask the InputMap to install them by providing the Node (the idea here being that Node is not stored, but provided as context):

     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.

The removal should be fairly trivial, and will lead to a better encapsulated and easier to reason about InputMap class that does not suffer from the chicken egg problem, and is not generically typed.

That brings us to InputMap's being mutable.  I would first spend some serious effort to see if this is strictly needed, and what benefits that would provide, as currently what I see is that InputMaps are mainly mutable in order to add mappings during construction (something a Contructor or Builder can do).  Immutable InputMaps would simplify things enormously, and would prevent mappings being modified (by a Skin or Behavior) that must be undone when the Skin or Behavior changes.  This would also mean there is no need to have multiple input maps in memory.  Cells have behaviors, and you can easily have 1000's on screen, requiring 1000's of Behavior and InputMap copies currently.

If they must be mustable however, then controls must listen for changes; this will inevitably mean that an InputMap refers each Control that listens for changes.  For a shared global InputMap this can mean that such Controls won't be GC'd, so we need to apply one of the usual strategies here to prevent this problem:

1. Use a weak listener (relying on GC)
2. Use a conditional listener (deterministic)

If InputMaps must be mutable, then the second one has by far the preference, and can be done by only listening for InputMap changes when the control is showing.  The best way to achieve this is for Controls to delay registering the InputMap's event handlers until they're about to become shown, and remove the event handlers when they're about to be unshown.  Note that this is not the same as visible/invisible and so won't lead to registration/unregistration upon toggling visibility.
Behavior
=======

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).

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.

As Behaviors respond to events however, there is I think no real need to have a direct reference to the Node -- the Event provides this already; it just needs to be extracted, or perhaps in some cases, this information needs to be added first as it is pertinent to the event.

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.  This makes sense, as that's probably the primary way a Behavior does its thing, it install event handlers and responds to them.

So I think that with a bit of refactoring, Behaviors can be made to not require the Node.  This will then allow Behaviors to be installed with a trivial setter, and would not need the install/dispose mechanism that Skins use.

TLDR;

I think that we should really be seeing if it is possible to remove the dependency on Controls before making these API's public.  This would remove the generic parameter from the signatures of both InputMap and Behavior(Base), and remove the chicken-egg dependency they currently have with Control.  It will simplify the design, and probably make a lot of classes and code a lot easier to reason about.

--John

On 30/09/2023 00:44, Andy Goryachev wrote:
Dear fellow JavaFX developers:

For some time now, we’ve been working to identify missing features in JavaFX that hinder application development.  We’ve been working on adding some of the missing features (for which we’ll have a separate announcement), but I feel that engaging wider community is a rather important part of the process.

I would like to share with you one such missing feature - ability to extend behavior of the existing components (and make the task of creating new components easier) by adding a public InputMap and BehaviorBase.

Please find the actual proposal here
https://gist.github.com/andy-goryachev-oracle/294d8e4b3094fe16f8d55f6dd8b21c09

We are very much interested in your feedback.  Thank you in advance.

-andy

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