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-r...@openjdk.org> on behalf of John Hendrikx
<john.hendr...@gmail.com>
Date: Saturday, October 7, 2023 at 10:43
To: openjfx-dev@openjdk.org <openjfx-dev@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
