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.hendr...@gmail.com>
Date: Tuesday, October 10, 2023 at 08:00
To: Andy Goryachev <andy.goryac...@oracle.com>, openjfx-dev@openjdk.org
<openjfx-dev@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
