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
