Sorry, I meant an example / use case where it is important and cannot be solved by the existing machinery. Any change in this area is going to introduce a major regression risk, and before we talk about changing the foundation we need to make sure we are solving a problem that exists.
-andy From: Martin Fox <mar...@martinfox.com> Date: Monday, October 16, 2023 at 14:20 To: Andy Goryachev <andy.goryac...@oracle.com> Cc: John Hendrikx <john.hendr...@gmail.com>, openjfx-dev@openjdk.org <openjfx-dev@openjdk.org> Subject: Re: [External] : Re: [Request for Comments] Behavior / InputMap Andy, John Hendrikx has given specific instances where this is breaking down already. The JavaDoc suggest that you can handle events on a node by installing event handlers. What it does not say is that a control may have already installed some unknown number of event handlers and there’s no guarantee yours will go first. And the key to controlling the customization of event handling is to get there first. The same problem occurs when subclassing a control (and I’m assuming we want to make it easier for clients to do that). Any one of the superclasses may have installed an event handler on the Node. If my subclass installs the same handler the two handlers will be executed in some random order which may or may not work. In other words, we have an existing problem with contention in the Node’s dispatcher and it will get worse. There’s nothing here that’s *impossible* to solve using the existing machinery. In both instances there are ways of manipulating the event dispatch chain to do what needs to be done. But it is not easy to write a custom EventDispatcher. It has to be coded correctly to support the bubbling and capturing phases and there is special handling for mouse enter/exit events. So if we want to pursue dispatchers as a solution we need to provide one. (Even if we don’t export an EventHandlerManager we have our own internal reasons to use one. Any time a control drops an event handler on the Node it will interact with every other event handler there. So if new code adds a new handler we should view that as a significant behavioral change that affects external clients. I don’t think we want to be in that position.) Martin On Oct 16, 2023, at 11:31 AM, Andy Goryachev <andy.goryac...@oracle.com> wrote: Martin: What would be the use case for creating a public EventHandlerManager? In general, what is the problem we are trying to address that is impossible to solve using the existing machinery? Thank you -andy From: Martin Fox <mar...@martinfox.com> Date: Saturday, October 14, 2023 at 14:48 To: John Hendrikx <john.hendr...@gmail.com> Cc: Andy Goryachev <andy.goryac...@oracle.com>, openjfx-dev@openjdk.org <openjfx-dev@openjdk.org> Subject: Re: [External] : Re: [Request for Comments] Behavior / InputMap I’ve been digging around in the code to get some context on the existing machinery for ordering event handler execution. I haven’t had time to write up test cases so all of this is based on reading the spec and the code. The current problem is that all handlers for a given Node are thrown into the same bucket and that bucket isn't delivering the execution order clients expect. Within the existing framework the minimal solution would be to (a) segregate handlers into separate buckets and (b) control the order in which the buckets are processed. The second problem is solved. The order of execution is controlled by the EventDispatchChain and there are public API’s for manipulating it. Note that it’s possible for one Node to have multiple dispatchers in the chain and anyone can take a fully constructed Node and add an EventDispatcher to it (if you’re subclassing a node you would override buildEventDispatchChain). Internally the first problem also has a solution. EventDispatchers which manage event handlers and filters are called EventHandlerManagers (the bucket I referred to earlier is the Node’s event handler manager). Again, a Node can have multiple dispatchers and any of them could be an EventHandlerManager. For example, the Control class could have an internal EventHandlerManager with its own collection of handlers and filters that is entirely separate from the Node’s. It would be up to the class to determine whether its manager executed before or after the Node’s. (In the public API anyone can implement an EventDispatcher but if they want to support the existing calls for managing event handlers and filters they are on their own. Personally I think we should provide an EventHandlerManager for public use.) Martin On Oct 14, 2023, at 10:19 AM, John Hendrikx <john.hendr...@gmail.com> wrote: On 12/10/2023 21:56, Andy Goryachev wrote: Filters: are we talking about key bindings or event handlers? With the key bindings, the user mappings take precedence over those registered through behavior. There is no provision for adjusting priority of the event handlers – that’s the FX reality, we don’t get to rearrange event handlers within the node. That’s why I mentioned event filters added to the control. I am not sure why you talk about HBoxes – adding a filter on the control enables the user to intercept the event prior to skin/behavior. On simple Controls yes, filters can be used there for this purpose, even though that's not their purpose. It works because a Control (usually) is a leaf node. It breaks down however when you want to change behavior of the View controls which have deeper control layers. You may want to override something defined for ListView, but only if say a focused editable control isn't consuming that event for a different purpose. A filter won't be able to do this. So yes, this proposal does not address the event handlers (sorry for confusing key bindings with event handlers). Unless we add addListenerBefore() API, we’d need to use event filters – but this is out of scope for this proposal. I do agree with you that we should keep the concrete Behaviors private for now. In any case, public behaviors are outside of scope for this proposal. I see BehaviorBase moving to a public package though, and it is not package private, is that intended then? One thing you mentioned several times is a “truly good design”. Let’s hear it! Could you give us an outline, at the public API level at least, of such a design? Alright; these things take a lot of time, but I've taken a few hours to think about it today. First, a lot of things just triggered me with the current proposal; - The mutable maps, immutability would allow making these static, saving many objects; when I have some time I can take a look at how many are in memory when a decent sized FX application is running; as even Labels are Controls, and Labels are the base class for any Cell, this number might be larger than expected and potentially could allow for some significant memory savings; making it public as-is closes that road down forever. Immutability does not mean things can't be changed, it just requires a slightly different mindset (ie. you combine a standard InputMap with a custom InputMap to form a new InputMap in which a binding is changed/overriden/disabled); this was also proposed on JDK-8091189 - The Control back reference; I firmly believe this is unnecessary, and also very undesirable. Events already contain this reference, its superflous and requires a new instance for an (otherwise) similar instance; This is even done already in places, exactly to avoid having to create more instances (see #getNode in FocusTraversalInputMap for example, effectively allowing that class to be static while providing the focus traversal behavior). This was also raised on JDK-8091189 - Not designing this using interfaces (also raised on JDK-8091189). With the addition of default methods, we should favor composable designs instead of being forced to inherit from a base class in order to provide a custom implementation. Sure, you can still provide a default implementation, but public methods should be referring to the interface so it can be a completely different implementation. Interfaces prevent using package private shortcuts where privileged classes can provide a functionality that users cannot. - The public BehaviorBase and the new public behaviors for many controls; I'm not convinced behaviors (if we can't define exactly what their purpose is supposed to be vs the Control and Skin) are ready to become more than just an implementation detail. As for high level design, it of course depends on what the goal here is. The issues linked in the proposal all call out for a public behavior API, but your proposal narrows the scope rightfully down to influencing default reactions to (key?) events only. Making behaviors public as they are now, and without a clear definition of what they are, seems definitely premature. I think they first need to be re-evaluated to see if they are still a good design at all (after all they're 10+ years old), then rewritten internally (if the ideas behind them changed), and only then made public. In your proposal I think the Summary and Motivation are quite good at outlining a problem to be solved. I'd like to rephrase that as a goal: "To allow developers to remap, override or disable the standard behavior of JavaFX controls (note: behavior here is used in the general sense)". I think there is no need to mention InputMap, Behaviors or key bindings here, those are merely possible means to achieve the goal. I'd then first take a look how this could be achieved with current JavaFX, and where users are running into a wall. Most obviously, the way to make controls do something you want is by using event handlers. Even Behaviors use these internally, in which we'll later see lies a bit of the problem. # Expectations Just like when a developer sets a Control property directly to a specific value, when the developers adds an event handler or listener, he/she can rightfully expect that FX respects this and does not get in the way. For the property case, CSS will not override such a property, for listeners, all listeners receive any callbacks, and for events, the user registered handlers should take priority over internal handlers (unlike listeners, event handlers can consume and act upon events before they reach the user handler, hence order plays an important role here). CSS, Skins and Behaviors sharing the same properties, listeners and event handlers with application developers has always been a bit of a balancing act; CSS has an elaborate system to respect user property changes, and keeps track of these; Skins for the most part manage to stay out of the application developer's way, mostly because they primarily use listeners which inherently don't block listeners added by the application developer. They also rarely override properties outright that are also modifiable by the developer. With Behaviors the situation is very different. Event handlers added by behaviors will consume events, effectively acting upon them before the application developer can (you may still see such events as "consumed", but they will not bubble up further). On top of that is the fact that EventHandlers are far more complicated than plain listeners or properties. For example, a KeyEvent.KEY_PRESSED handler is called before a KeyEvent.KEY_ANY handler; attempting to override behavior in a KeyEvent.KEY_ANY handler is therefore impossible when the behavior to override is using a more specific event type. Consumption of an event only blocks capturing/bubbling of the event; other event handlers at the same level do still see such events, but they're marked "consumed"; most event handlers should therefore probably start with a `if (e.isConsumed()) return` line, but often don't (users often don't do this because they expect their handlers to be the only ones present, or to be called first, even though for Controls with Behaviors this is not actually true). # Problems Some of the problems you can expect to see when you want to act upon an event that has a "default" behavior (versus ones that don't): - Adding a more generic event handler than the one used internally will result in the events you are interested in being consumed already - Adding the exact same event handler as one used internally AFTER the control was shown (or re-adding it in response to something) will result in events you are interested in being consumed already, or more generally, an event handler works differently whether they were added before or after the control was shown... - Events for which there exist a default behavior are in some cases consumed even if the behavior could not be performed (navigation) In all the above cases, for events WITHOUT default behavior, such a user installed handler works exactly as expected. IMHO there really should be no difference for the user whether there is a default behavior or not. # Causes Almost all of these problems are caused by the fact that JavaFX's internal handlers share the same lists/mechanisms as application developers to react to events; these internal handlers are mixed up with event handlers from application developers; often the internal ones run first, but it is very unpredictable: - Is your event handler more generic than an internal handler? You always run last - Is your event handler more specific than an internal handler? You always run first - Is your event handler the exact same event type as an internal handler... then: - Did you add handlers before the control was shown / skin was created? Yours run first (subject to change no doubt, we don't guarantee this) - Did you add handlers after the control was shown? Yours run last (no guarantees) - Did you add handlers after the control was shown, but then its skin was replaced? Your event handlers that used to run last now run first... (undocumented) An innocent change like listening for KeyEvent.ANY vs KeyEvent.KEY_PRESSED can have big repercussions. # How to reach the goal? There are many ways to reach the above stated goal. Opening up some internal structures that are used by the internal event handlers is one way, but it effectively creates a 2nd mechanism to do the same thing. I can change the internal event handler's internal structures to do X, or I can create an event handler that does X. For some of these, both options work, and for others, only this new mechanism works. Not only is this mostly compensating for a flaw in the event handling system, but it also means that you need to be aware of which events need special treatment. It's even possible that some events require no special treatment now, but may in the future, or may need it if the platform changes certain defaults. In other words, this new mechanism would effectively need to be used in all cases or you risk your solution (using standard event handlers) breaking in the future (or JavaFX would have to freeze input maps and never change them again -- that's already sort of the case, but it is good to be aware of that). # Alternative solution I would look into seeing if the flaws in the event handling system can be resolved, so that this mechanism that is already available, and that users already know becomes powerful enough to cater to the stated goal. Note that this doesn't preclude opening up behaviors later, but it does take away one of the fundamental reasons to do so, perhaps allowing for quite a different way of exposing these to users as the primary driver need no longer be focused on remapping bindings. Perhaps the primary driver can then be how to design behaviors in such a way that they can be re-used and easily subclassed; perhaps behaviors are no longer needed at all, and they can remain an internal implementation detail, or perhaps they can be part of skins or controls. I see a few problems that should be addressed if we want to be able to reach the goal with better event handlers: 1) Internal event handlers should NOT use the same mechanism as user event handlers; effectively, user event handlers (of any event type, even more general ones) run first, as if no internal event handlers existed at all. This is already the case depending on the timing of when the user added the handlers; the timing is unpredictable (as stated above) and so I think we have enough leeway to change this, and enough reason to tighten up the specification here. 2) All internal event handlers run AFTER user ones (per EventTarget), regardless of when they were added. A separate list can be used, or the event type system could support this with some kind of internal flag. 3) Internal event handlers can be skipped completely if the user marks an event as such. This is different from consuming the event; effectively, the event is not consumed (and can bubble up to other event targets) but internal event handlers are not allowed to act upon it. 4) All actions triggered by behaviors should be available as public methods (nested or direct) on their respective controls, allowing the user to call these as well. The above changes should be enough to support the stated goal: "To allow developers to remap, override or disable the standard behavior of JavaFX controls (note: behavior here is used in the general sense)" To override a standard behavior: install event handler (which will run first), react to an event, call a public method triggering the DIFFERENT behavior, consume the event To disable a standard behavior: install event handler, react to an event, mark it as "not for internal use", effectively disallowing the internal handlers from acting on it To remap a standard behavior: combine the above two solutions # New API needed A flag on `Event` that can be set/cleared whenever the user wants. The flag effectively communicates that the given event is not to be processed by the "hidden" internal event handlers added by JavaFX. It could be called "noDefault" or "skipDefaultBehavior". Depending on the internal changes needed to separate user event handlers from internal ones, EventType may also need a small change. For example, if the separation is implemented by introducing more event types, a convenient `EventType#toInternal` method could be added to convert a regular event type to an internal one that is always processed after standard ones. It's possible such a method does not need to be public (but could be if users desire the old unpredictable behavior of mixing user and internal event handlers). # Alternative alternative solution Part of the problem can also be solved by disallowing internal handlers to listen on the most specific EventType (ie, don't listen to KeyEvent.KEY_PRESSED, but instead listen to KeyEvent.ANY). This way a user can be the first to handle the event in all cases by using a more specific type (KeyEvent.KEY_PRESSED) or the last in all cases by using a less specific type (InputEvent.ANY). This leaves much to be desired, and doesn't solve all of the above outlined problems, but I mention it to show that there is quite a lot possible here already by tweaking the event handling system. --John Thank you -andy From: John Hendrikx <john.hendr...@gmail.com><mailto:john.hendr...@gmail.com> Date: Thursday, October 12, 2023 at 01:33 To: Andy Goryachev <andy.goryac...@oracle.com><mailto:andy.goryac...@oracle.com>, openjfx-dev@openjdk.org<mailto:openjfx-dev@openjdk.org> <openjfx-dev@openjdk.org><mailto:openjfx-dev@openjdk.org> Subject: [External] : Re: [Request for Comments] Behavior / InputMap On 11/10/2023 19:44, Andy Goryachev wrote: Dear John: Seems like addEventFilter() was specifically designed to intercept events before any other handlers, so I see no problem there. This is a misunderstanding of what filters are for. They're called in the capturing phase and they can prevent an event from reaching its intended target, but you want it to reach the intended target FIRST, as you still want to give the target the chance to be the first to act upon the event. For example, let's say I want to attach a function to the SPACE key in some specialized HBox, it should only do something when something closer to the target doesn't need it first (like a nested HBox of the same type, or an active TextField that uses SPACE for input). Now if HBox had some FX default event handler that consumed SPACE, I have no options to override SPACE anymore; the filter is not a good spot, as its too early, and the handler is not a good spot because Behavior handlers were added before mine was. I somewhat disagree about the purpose of the key mapping system – the proposed solution solves two existing issues (the skin/behavior mappings and the user mappings) in one neat package. Every other instrument such as addEventHandler/Filter is still there. I'm saying that the need for this would be almost non-existent if user event handlers weren't considered less important than FX ones. You have to be careful that there aren't two ways of doing things here: If the event you wish to give an alternative purpose to is unused by FX, you can use an event handler; otherwise you must disable it (so you can use an event handler!) or remap it (using an alternative system). Note that if FX at some point decides to "claim" another mapping, that would be a breaking change as some user event handlers may cease to function. This is why I think the input mapping system should stay hidden; its an implementation detail of the Event handlers added by FX so they don't need to write long if/else/switch chains, and so they can more easily switch out mappings depending on state. Opening up the input map effectively is being able to influence those FX added event handlers to do something different, while there is a perfectly good way of doing that already: add your own event handler (with higher priority). And, if we look at the three bullet points - Ensure user event handlers have priority over behavior/inputmap added ones - Ensure all behavior actions are available as methods on controls - Ensure that if a key is handled by the control, that it is ONLY consumed when it actually triggers an action (navigation keys get consumed regardless, even if no focus change results, that's wrong). I absolutely agree, and in fact the first three are indeed a part of the proposal. Well, the 3rd one might unfortunately be a subject of backward compatibility limitation since one of the requirements was no behavior change w.r.t. the earlier versions. We can always change the behavior if we have a completing reason and go through the usual process, nothing in the proposal precludes that. I don't see how your proposal addresses the first point. I've been reading the comments attached to https://bugs.openjdk.org/browse/JDK-8091189 and it has a lot of good information, and raises many good points (immutable input maps, keep input maps/behaviors as implementation details, use interfaces instead of base classes, what about controls that have no Skin, even the point I made about having the Control be in charge of registering the event handlers instead of letting InputMap do it requiring a Control field...). There are several patches by Jonathan Giles, and there is even a library created by the author of ReactFX that allows for replacing key bindings with a much nicer API already (in so far that is possible without having inside FX support). The general tone of the comments seems to be that Behaviors should be kept as implementation details -- they're not well defined (what is a Behavior, what should be in the Behavior, what should be in the Skin and what should be in the Control) and until that is absolutely clear, exposing any of that as API is premature. Making the behaviors completely independent with setBehavior() and FXML, as you said, might be a future effort, perhaps we could attempt opening up certain controls at some point. On one hand, I am for increasing the extensibility of FX, on the other hand the same argument can be made against it (as in solidifying a particular way of constructing skins and behaviors), but I feel it’s a separate issue that is independent of this proposal. I'm starting to lean towards keeping all of this as implementation details, at least until the current implementation is much cleaner than it is currently (the way InputMap and Behaviors currently are set up is more pragmatic than truly a good design), and just address the main issue: JavaFX stealing events that users want to override, note that I say events, key bindings are only part of it. --John -andy From: openjfx-dev <openjfx-dev-r...@openjdk.org><mailto:openjfx-dev-r...@openjdk.org> on behalf of John Hendrikx <john.hendr...@gmail.com><mailto:john.hendr...@gmail.com> Date: Wednesday, October 11, 2023 at 01:04 To: openjfx-dev@openjdk.org<mailto:openjfx-dev@openjdk.org> <openjfx-dev@openjdk.org><mailto:openjfx-dev@openjdk.org> Subject: Re: [Request for Comments] Behavior / InputMap I'm sorry, but that providing an arbitrary key mapping system seems completely out of scope and not something that JavaFX should concern itself with. It's much too high level, when the key mappings involved should only be for actions that the control can provide on its own. I think the problem we should be solving is that JavaFX control behaviors shouldn't be first in line when it comes to consuming events (which currently is only the case due to event handlers being added at the earliest possible opportunity, and event handlers being called in order). If something as trivial as: control.addEventHandler(KeyEvent.KEY_PRESSED, e -> { if (e.getCode() == KeyCode.LEFT) { e.consume(); // stop default behavior } }); ... actually worked, then there is much less need to redefine/disable behavior key mappings, and no need for a secondary system that deals with mappings (the first system, event handlers, can simply be used for this). If user event handlers had priority over behavior ones, then everything you want can be achieved with the above, including: - Stopping default behavior - Triggering different behavior (just call something on control, of course, make sure all behavior actions are available on the control in the first place) - Remapping (a combination of the above two) - Adding an alternative key for the same behavior A system to remap keys can then be left squarely in the realm of user space, and much nicer solutions can be build by users than whatever JavaFX will provide out of the box. Changes to the Behavior system can then focus on replacing complete behaviors (including their input map) and being able to use these by default for a certain subset of controls (like -fx-skin provide in CSS), as this is something users currently can't do. So in short, what I think this should be about is: - Ensure user event handlers have priority over behavior/inputmap added ones - Ensure all behavior actions are available as methods on controls - Ensure that if a key is handled by the control, that it is ONLY consumed when it actually triggers an action (navigation keys get consumed regardless, even if no focus change results, that's wrong). Future: - Make behaviors public and allow Behaviors to be replaced with -fx-behavior type CSS syntax / control.setBehavior calls --John The focus should be on being able to modify standard behavior of controls (arrow-left, enter, ctrl-shift-right, etc.), specifically also to be able to disable these when undesired, and, on top of that, that they bubble up when NOT used even when they are configured (focus navigation keys currently are always consumed, whether they actually do something or not -- that's a big issue). The other focus should be on providing an alternative behavior (or at least mappings) for all controls of a certain type -- I don't see the need for adding a mapping to a specific control, that's already covered with event handlers; the problem is mostly that behaviors currently steal certain events before the user can get at them. Custom behaviors can then be constructed that provide more things that may need mapping. I'd expect those however to be limited in scope to what the control offers, certainly not an arbitrary key/action mapping system (that wouldn't even work, as most of these would be in the scope of several controls or be global). This kind of functionality is much better provided by event handlers at the correct level for a group of controls, and I wouldn't expect to find such an eloborate system incorporated in behaviors. In fact, thinking about all of this a bit more, On 10/10/2023 19:54, Andy Goryachev wrote: Re-sending with a smaller image (256kb limit, really?). From: Andy Goryachev <andy.goryac...@oracle.com><mailto:andy.goryac...@oracle.com> Date: Tuesday, October 10, 2023 at 10:49 To: Michael Strauß <michaelstr...@gmail.com><mailto:michaelstr...@gmail.com> Cc: openjfx-dev@openjdk.org<mailto:openjfx-dev@openjdk.org> <openjfx-dev@openjdk.org><mailto:openjfx-dev@openjdk.org> Subject: Re: [Request for Comments] Behavior / InputMap Dear Michael: Here is a use case for (re-)mapping by the user at runtime: <image002.jpg> (key mappings UI in Eclipse). I can think of several other cases (mentioned in the proposal, I think) so I think we can put the concept of immutable or global InputMap to rest. Whether the InputMap contains the reference to its control or not is a minor implementation detail, I think. -andy From: openjfx-dev <openjfx-dev-r...@openjdk.org><mailto:openjfx-dev-r...@openjdk.org> on behalf of Michael Strauß <michaelstr...@gmail.com><mailto:michaelstr...@gmail.com> Date: Tuesday, October 10, 2023 at 10:36 To: Cc: openjfx-dev@openjdk.org<mailto:openjfx-dev@openjdk.org> <openjfx-dev@openjdk.org><mailto:openjfx-dev@openjdk.org> Subject: Re: [Request for Comments] Behavior / InputMap > 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. > I struggle to see a use case for this feature. I can imagine that there might be some use cases that call for customized input mappings, but why would this translate to a _mutable_ input map? That's quite a departure from the way other parts of JavaFX work. For example, skins are also immutable. If you want to have a different skin for a control, you don't somehow modify the existing skin instance; instead, you'd create a new skin class (or -- somehow -- extend an existing skin class), and then install that new skin on your control. An input map shouldn't bind input events directly to instance methods of a particular control instance. It should define the mapping of events to methods symbolically: Instead of mapping Event => instance.method(), it should map Event => Control::method. The input map could then be stateless and immutable, and can be set on any control instance. If you want to change the mappings, just set a different input map instance. There's no need that an input map would retain a reference to any particular control, since the control reference can be passed into the input map just as easily.
