Bug ID: JDK-8091189 Move BehaviorBase into public API

Other
tbdUnresolved

I'd like to suggest the InputMap API: Proposal: https://github.com/andy-goryachev-oracle/Test/blob/main/doc/InputMap/InputMapV2.md Discussion: https://github.com/andy-goryachev-oracle/Test/blob/main/doc/InputMap/InputMapV2-Discussion.md API Specification (Javadoc): https://cr.openjdk.org/~angorya/InputMapV2/javadoc/ JavaFX Model-View-Controller (MVC): https://wiki.openjdk.org/display/OpenJFX/UI+Controls+Architecture Control Class Hierarchy: https://github.com/andy-goryachev-oracle/Test/blob/main/doc/Controls/ControlsClassHierarchy.md JBS: JDK-8314968 Public InputMap
25-06-2024
A better approach that I've previously mentioned is the 'interceptors' API I've been exploring (although time has been short recently). For example, you would write something like the following: button1.getInputMap().getInterceptors().add(event -> { if (! (event instanceof KeyEvent)) return true; return KeyCode.SPACE != ((KeyEvent)event).getCode(); }); Or, in short form, the following: button1.getInputMap().getInterceptors().add(new KeyMappingInterceptor(new KeyBinding(SPACE))); In this approach, the event is not consumed and does bubble up to parent containers for handling.
21-04-2015
Hi John, I will be repeating myself, but the problem with your code overriding the SPACE key is that it consumes the event, while I would like it to bubble up and be handled by some parent node. The null KeyBinding really is an implementation detail. If you make it part of the documentation, it means you as a developer cannot change it in the future without breaking your users. It gets even worse. The mappings are (typically) registered by the skin. Even if the control documents the mappings, and the user of the control follows that, it would still break with a different skin that defines the mappings differently than the default skin provided by the author of the control. > Keep it is as clean and as simple as possible Absolutely. I just don't associate arbitrary mutability and digging into implementation details with cleanliness and simplicity.
20-04-2015
Hi All, I am a relative newcomer to JavaFX in general and in particular to custom controls, how ever I have recently been designing some custom controls and so this issue is of interest to me. Having spent a couple of hours reading this issue and digesting most of it, I would like to make the following comments I would agree with Jonathan that the API needs to be as lightweight and as simple as possible. I would like to see something along the following lines, A standard control, button etc installs its own default mappings and the user or developer of the control has the ability to override these as in Jonathon's example or remove them altogether. button1.getInputMap().getMappings().remove(mapping); button1.getInputMap().getMappings().add(mapping); button1.getInputMap().getMappings().addAll(mapping...); button1.getInputMap().getMappings().setAll(mapping...); button1.getInputMap().getMappings().clear(); To pick up on a point that Tomas made "because the user of the control has to know that a null KeyBinding has been registered by the skin" I believe that might be a mute point in terms of this discussion as it is my responsibility as the control developer to inform the user of the control that such a binding has been installed. If a user really wants to ignore a particular mapping what would be wrong with the following ? // The SPACE key should no longer fire the button (and instead do nothing) button1.getInputMap().getMappings().add(new KeyMapping(SPACE, e -> { // do nothing })); I can see where this might get cumbersome if the user wanted to ignore a large number of mappings, but in that case I would expect to the user to remove all mappings and provide a new set. From a new comers perspective, it appears to me that there exists a danger of trying to make this an "all things to all people" implementation Keep it is as clean and as simple as possible, I can see nothing wrong with the above API so long as it consistent across all controls or nodes.
20-04-2015
One option to resolve both issues mentioned in your previous comment is to promote my 'interceptors' concept (for lack of a better name) up to InputMap. That way, you can do the following: button1.getInputMap().getInterceptors().add(event -> { if (! (event instanceof KeyEvent)) return true; return KeyCode.X != ((KeyEvent)event).getCode(); });
10-04-2015
Both good points - I'll give them some thought. Thanks.
10-04-2015
The issue I was pointing at was that the user of the control needs to know that the skin installed a "catch all" mapping at all. The user should not care whether the skin installed and individual mapping for Tab, a catch-all mapping, or something else. The user just wants to say "Whatever handler might have been installed for 'Tab pressed', ignore it". Plus I just realized a catch in both our proposals: there has to be a way to ignore a mapping even if that mapping has not been installed yet (remember, skins are only instantiated later). We mentioned this before, but see, it got lost. I have an idea how to solve this in my proposal that would require a return value from the handle method: enum Result { CONSUME, SKIP, CONTINUE } Result handle(Event event);
10-04-2015
You think this issue is long - try getting up to speed on RT-12643 (which has fortunately been resolved)! :-) I will try to extract out my proposed API next week. With the 'ignoring the tab key' issue - yes, it isn't great, but there are ways to improve that. The simplest is to have a KeyBinding constructor that just takes an EventType<KeyEvent> and is documented to be specifically for the 'catch all' scenario. That at least helps with the readability.
10-04-2015
This has become a very long discussion. Things said earlier can easily get lost in this linear flow of text. I am also afraid that this amount of previous text to read can discourage potential new contributors to the discussion. As you said, we should really write down the proposed new API, how it handles a bunch of use cases, and the implementation, side by side for both of our approaches. That would help us, as well as newcomers to the discussion. I wish I had a few days to dedicate to this, which I currently don't. I really don't like the way how your new prototype handles "ignoring the Tab key", because the user of the control has to know that a null KeyBinding has been registered by the skin, which should be an implementation detail of the skin.
10-04-2015
A bit of an update: I've done some more exploring with my prototype and have made a few small tweaks. None of them will be popular to Tomas, but its all for exploration and discussion purposes, so rip it to shreds if you don't like it :-) Firstly, the changes: 1) I've added a List<Predicate<T>> getInterceptors() method (where T extends Event) into the Mapping class. I'm sure there is a better name out there, but for now it is interceptors :-) What this does is provide a means of people creating mappings that are conditionally run, based on zero or more predicates being specified. 2) I've added a 'disabled' property on to Mapping also, to allow for a mapping to be disabled via a binding. This was based on Hendriks comment earlier. 3) I've added a KeyBinding class to make it slightly nicer to specify key bindings. 4) I added inputMap#lookupMappings(Object). The Object no longer is reflected upon - it simply looks into a Map<Object, Mapping<?>>, where the key is a unique identifier returned by the mapping based on its type (so, the KeyBinding for KeyMapping, the MouseEvent for MouseMapping, etc). With these changes I've (mostly) recreated the StyledTextAreaBehavior class (well, at least, the input mapping definitions). I did this to test if there were any edge cases I hadn't considered. Of course, I can't test the changes, and there is one thing I didn't port (the '.ifConsumed((b, e) -> b.clearTargetCaretOffset())' part). Overall though I think the translation was relatively painless and the readability remains high. I can post it if you are interested. I also extended my tests in HelloButton, to look up and modify various built in mappings. Here are some snippets: // add new mappings // The J key should fire the button button1.getInputMap().getMappings().add(new KeyMapping(J, e -> button1.fire())); // The SPACE key should no longer fire the button (and instead just print text to console) button1.getInputMap().getMappings().add(new KeyMapping(SPACE, e -> { System.out.println("This should replace the default action event!"); })); // The degenerate case where we accept all input - a null KeyBinding matches anything // and results in firing the button button1.getInputMap().getMappings().add(new KeyMapping(new KeyBinding(null, KEY_TYPED), e -> { System.out.println("catch-all mapping caught event: " + e); button1.fire(); })); // test one: lookup the J mapping and disable it button1.getInputMap().lookupMapping(new KeyBinding(J)).ifPresent(mapping -> { System.out.println("disabling J key mapping"); mapping.setDisabled(true); }); // test two: the degenerate case - we need to iterate through the mappings // to find the null keyCombination, so that we may install an interceptor // to block the tab key. This means all keys except tab will work. button1.getInputMap().lookupMapping(new KeyBinding(null, KEY_TYPED)).ifPresent(mapping -> { System.out.println("adding interceptor for Tab key mapping"); mapping.getInterceptors().add(event -> { if (! (event instanceof KeyEvent)) return true; return KeyCode.TAB != ((KeyEvent)event).getCode(); }); }); // test three: remove a mapping button1.getInputMap().lookupMapping(new KeyBinding(J)).ifPresent(mapping -> { System.out.println("removing J key mapping"); button1.getInputMap().getMappings().remove(mapping); }); Regarding Node vs Control, it is not inconceivable that this InputMap API ends up on Node - it is worth discussing.
09-04-2015
Regarding scope, I think a lot of this makes sense for Node in general, not just Control. Maybe we should be also careful about not preventing future promotion of InputMap-related API to Node. I may be an author of an input mapping API, but only because I am a user of such API in the first place.
09-04-2015
In terms of minimal API - we have to consider new classes, interfaces, and methods (such as all the keyPressed, etc methods). I can appreciate you're pushing for an interface-based approach with minimal implementation. This is why I said earlier we should round up all the API (including unimplemented methods) you're proposing onto InputMap so that we can better understand the surface area of your proposal. My problem is reconciling what API travels out of WellBehavedFX in such a scenario. I worry that it is too much (in terms of scope). To clarify - it isn't a political issue - it is a scope issue. We have to keep the scope of this work as minimal as possible, whilst exposing as much functionality as possible (within reason). We do not have the man power to push for any more than we absolutely need. Also, we have to be careful to properly define the users of this API - when you say "users like me" I think that may be a misnomer in some sense, as you're by far a vastly different type of user than the generally intended audience (I don't believe anyone else has developed their own input mapping API!). So whilst we of course need an API that caters towards the (at most) handful of power users such as yourself, we can't forget that the actual target audience is slightly different. I'm exploring alternate KeyMapping API at present, as I'm not entirely happy with the current one. Will report back if I find anything.
09-04-2015
> An important consideration to this API is to keep the API as minimal as possible. My proposal so far is certainly lighter on the number of new types introduced: just InputMap and EventPattern. I'm not trying to push hard for EventHandlerTemplate - I'm trying to push hard for making EventHandlerTemplate possible in a library. On the other hand, it seems somewhat heavier on helper methods. > I would be stepping outside the bounds of my ownership I would like not to get into a politics talk. I care about finding a good solution for the users like me. It's just too bad if lack of coordination within the JavaFX team is a limiting factor. > by proposing to include additional event related API in this change. Interfaces like EventPattern are unlikely to be something we can consider About the only thing about EventPattern that is more event related than InputMap or Mapping is the word "event" in its name. The use seems very similar to me: new KeyMapping(UP, e -> traverseUp(e)) versus on(keyPressed(UP)).act(e -> traverseUp(e)) and I am actually fine with either, as long as this stays separate from the InputMap interface. > For example, wouldn't the user have to do something like the following No, it's still just ignore(keyPressed("\t")), except now keyPressed returns just Predicate<Event> instead of EventPattern.
09-04-2015
An important consideration to this API is to keep the API as minimal as possible. I would be stepping outside the bounds of my ownership by proposing to include additional event related API in this change. Interfaces like EventPattern are unlikely to be something we can consider (the same goes for the builder pattern, etc, discussed earlier). If we stick to the minimal interface required, ignore(Predicate<Event>) I feel like we end up going down a very similar path to what I was proposing for the lookupMapping method I suggested earlier (except the job of doing the comparison is put back on to the developer, rather than the InputMap developer). For example, wouldn't the user have to do something like the following (jira-typed pseudocode warning!): inputMap.ignore((Event event) -> { if (! (event instanceof KeyEvent)) { return false; } KeyEvent keyEvent = (KeyEvent)event; // compare keyEvent event type, character, etc to expected value return true / false; });
09-04-2015
Also, in the light of InputMap being able to tell the minimal set of EventTypes it can possibly handle, EventPattern may need another method: getEventType().
08-04-2015
Predicate<Event> is the minimal interface required by ignore(). keyTyped("\t") from my former example was a reference to an actual method in WellBehavedFX, returning an EventPattern. EventPattern<T extends Event, U extends T> is slightly more powerful than Predicate<Event>: instead of test(Event) method returning boolean, it has a match(T) method returning Optional<U>. That is, it has the ability to down-cast the event to a more specific type. With retrospect, the first type parameter now seems mostly useless, so consider EventPattern to be interface EventPattern<U extends Event> extends Predicate<Event> { Optional<U> match(Event event); // default implementation of Predicate<Event> default boolean test(Event event) { return match(event).isPresent(); } } and the signature of keyTyped("\t") to be EventPattern<KeyEvent> keyTyped(String character, KeyCombination.Modifier... modifiers); Then keyTyped("\t").match(event) returns either Optional.empty(), or event cast to KeyEvent, thus freeing the users from type-casting the event themselves. Anyway, for the purposes of the ignore method above, Predicate<Event> is sufficient.
08-04-2015
Tomas, I'm trying to understand your ignore code. In particular, you pass in a Predicate<Event>, and use this as a test against the actual event. This seems fine, but earlier your example code is ignore(keyTyped("\\t")). Is there something that needs reconciling here? I didn't expect keyTyped("\\t") to return a Predicate<Event>, or even an Event for that matter.
08-04-2015
Ok - I see - thanks for clarifying. Perhaps it is time for you to roll up your proposed InputMap interface so we can see how it feels with all proposed API? It doesn't need to be fully implemented of course.
08-04-2015
Yes, I consider instanceof to be a form of reflection. Implementation of the ignore method can be something along these lines: interface InputMap extends EventHandler<Event> { void handle(Event event); Set<EventType<?>> getEventTypes(); default InputMap ignore(Predicate<Event> pattern) { return new InputMap() { public void handle(Event event) { if(!pattern.test(event)) { InputMap.this.handle(event); } } public Set<EventType<?>> getEventTypes() { return InputMap.this.getEventTypes(); } }; } static void ignore(Control control, Predicate<Event> pattern) { InputMap im = control.getInputMap(); if(im == null) { im = InputMap.EMPTY; } control.setInputMap(im.ignore(pattern)); } }
08-04-2015
Some good use cases have been fleshed out here - thanks - I'll give it all some thought and will get back when time permits. In terms of the 'reflection and all' comment - I wasn't thinking there would be any reflection necessary (unless you mean 'instanceof' calls). In the meantime, how do you envision implementing your ignore method?
08-04-2015
A lookup method like that is going to cause you headaches (reflection and all). And it is still just best effort, thus unreliable, thus going to cause headaches to users, too. An ignore method really is trivial, both for the implementor and for the users. > The use case you're proposing is most likely an uncommon case (that is, the general use case is to have one specific input to one specific mapping) This is how TextArea handles input. It cannot add a specific mapping for each character. It may not be the most common use case, but it is an important one. > I'd just add an event filter to the control to consume the tab key input before it is ever received into the input map. Consuming the event by an event filter does not work, because I want the event to bubble up and be handled by some parent node.
08-04-2015
This is all speculation on my part at the moment, but one option is to have a InputMap.lookupMapping(Object) method, where subclasses would try their best to handle the input. For example, KeyMapping would firstly check if the Object was a KeyCombination, and if so use that directly to compare against what is stored in the KeyMapping. If the object is a String, it may try to call KeyCombination.valueOf((String)input) and compare with that. Similar code could be written for each of the Mapping subclasses. Users could then write code along the lines of: inputMap.lookupMapping("shift+F10").ifPresent(mapping -> inputMap.getMappings().remove(mapping) A similar code pattern could also be used for disabling mappings temporarily, as Hendrik suggested, (assuming API is added), along the following lines: inputMap.lookupMapping("shift+F10").ifPresent(mapping -> mapping.disabledProperty().bind(.....)); The use case you're proposing is most likely an uncommon case (that is, the general use case is to have one specific input to one specific mapping). To handle this case, I would likely not use inputMap at all - I'd just add an event filter to the control to consume the tab key input before it is ever received into the input map.
08-04-2015
What is going to be the argument to the lookup method? I suppose something like EventPattern? Anyway, implementation of such a method is only possible if you have a closed set of Mapping implementations and a closed set of EventPattern implementations (which again, is very restrictive). Even then, implementing the lookup method is a hassle. On the other hand, implementing a method like `ignore(EventPattern)` proposed above is trivial. How would you go about this use case: The developer of the control adds a (single) mapping that handles any typed character. The user of the control then wants to remove the mapping for the Tab character. This is trivial with the proposed ignore method: InputMap.ignore(control, keyTyped('\t'))
08-04-2015
In many regards, having a list of event handlers would have been a useful API decision! It would give an explicit ordering to events, which is something I really wish we had (so that you could prioritise event handlers). For removing key mappings, there are two answers. Of course, if the user has provided their own mapping, they can just remove that from the list (assuming they keep a reference to it). In terms of removing existing mappings, or more generally any mapping for a given input on a control (which is arguably more important) - I haven't crossed that bridge yet in terms of API, but my thinking was that we may end up creating a public API for the behavior of the control. This behavior essentially creates the InputMap, but perhaps also has a means of looking up a Mapping for a given input. Alternatively, this functionality could just go directly onto InputMap as a lookup method. I'm not yet clear on how much of the current Behavior classes we end up making public API (in terms of moving to a javafx.scene.control.behavior package). I have a feeling that this is an inevitability, but I don't want to end up with an explosion of new classes either.
08-04-2015
Depends which part of JavaFX you are looking at. Input mappings are a lot like event handlers (they actually are event handlers). JavaFX does not expose a list of event handlers and let you manipulate it directly. By the way, what is the approach in your prototype to remove a key mapping?
08-04-2015
My main concern with the template-based approach is the requirement to add potentially quite a few methods to Control or InputMap, for things like adding, removing, ignoring, chaining templates, etc. This feels inconsistent with 'The JavaFX Way', which is to expose a list of objects and let the user mutate that list as appropriate. With my prototype, the InputMap just exposes the mappings list and lets the users (both controls developers and end-users of the controls) mutate the list as desired. I think this is the primary concern that needs to be addressed with the template approach.
08-04-2015
Regarding first paragraph: yes, all I was saying was that two templates would be combined into one (e.g. template1.orElse(template2)) and would be registered at once. > Even still, you'll need to differentiate the two different event types somehow? I don't see where you see the problem. Each individual mapping knows what event type it handles (whether it is your KeyMapping, MouseMapping, etc, or my mappings built from "event pattern" and "action"). If you combine template1 that only handles "key pressed" and template2 that only handles "key typed", then handle(Event event) method of the resulting template (i.e. template1.orElse(template2)) can be smart enough to skip template1 and route the event directly to template2, if the event is a "key typed". > Regarding the ability to add only the bare minimum listeners based on the provided mappings, I tend to prefer embedding that within the InputMap to save the burden of Control having to observe a set of mappings that can change at runtime. But I don't have a strong conviction for this. With my approach, it is really just observing that one property - inputMapProperty(). As proposed above, the InputMap has a getEventTypes() method that returns a set of event types, but that set of event types does not change at runtime for a given InputMap, since InputMaps in my proposal are immutable. Therefore, the Control only needs to act when the InputMap is replaced. Also, it is the responsibility of the Node to observe changes of onMouseClicked, etc, so it would only be consistent with that. > The problem I have with the interface approach is it becomes too minimal, or too lowest common denominator, The main idea here is separation of concerns, InputMap having only what Control needs it to have. How InputMaps are constructed, that is a different concern. > to the point where it becomes potentially painful to work with. For example, my current prototype allows for runtime modification of the mappings, using API such as inputMap.getMappings().add(newMapping). That's still too much boilerplate ;) And it is actually control.getInputMap().getMappings().add(newMapping) right? How about control.addMapping(newMapping) or InputMap.add(control, newMapping)? You can do this with both our approaches, so this is not a differentiating factor. > Up to this point, I don't feel convinced that a template-based approach offers any resounding value over the per-control approach I'm not arguing to pick one over the other. I'm arguing for making both possible, while you lean towards closing the doors for a template-based approach. > and in fact, my concern is that it makes it harder to achieve some things (like removing mappings) Is control.ignoreMapping(keyPressed(SPACE)) or, if we don't like adding too many new methods to control, InputMap.ignore(control, keyPressed(SPACE)) harder than your prototype? > I am always open to suggestions, but I think there are hurdles with the template approach that need resolving. I think I have addressed all concerns you have raised over the course of this discussion. If you feel I haven't, or if you have further concerns, I will be happy to try to address them. Of course I am biased, but my conclusion so far is that the interface approach * is more flexible (not closing doors for alternative implementations), * better separates concerns (construction (by the user) vs. consumption (by the control)), * is at least equally usable as the mutable class prototype. I actually believe it will be easier to both implement and use, because of the immutability property. Mutable state is just notoriously hard and error prone.
08-04-2015
Unfortunately I'm still not following your thinking from the first paragraph. I don't understand how having an InputMap property on Control would lessen the number of templates needed in your approach. I'm not saying there is anything wrong, but it is clear that I mustn't be fully understanding some important concept of your approach. Are you just saying that you'll put the mappings into the InputMap, and that this would install into the correct event handlers? Even still, you'll need to differentiate the two different event types somehow? Regarding the ability to add only the bare minimum listeners based on the provided mappings, I tend to prefer embedding that within the InputMap to save the burden of Control having to observe a set of mappings that can change at runtime. But I don't have a strong conviction for this. The problem I have with the interface approach is it becomes too minimal, or too lowest common denominator, to the point where it becomes potentially painful to work with. For example, my current prototype allows for runtime modification of the mappings, using API such as inputMap.getMappings().add(newMapping). In general I feel like the discussion is now at the point where we are discussing two separate approaches to the same problem, and we'll likely make little progress convincing each other :-) Up to this point, I don't feel convinced that a template-based approach offers any resounding value over the per-control approach (and in fact, my concern is that it makes it harder to achieve some things (like removing mappings)). My thinking is to continue seeking feedback but to also continue developing my prototype to keep exploring the domain. I am always open to suggestions, but I think there are hurdles with the template approach that need resolving.
08-04-2015
There are two separate (instead of just one) top-level templates constructed in StyledTextAreaBehavior, because they are registered with two different `on<event>Property()`s. With a single inputMapProperty() for all event types, there would be just one top-level template created. Same for the helper methods to override a mapping - they would all work with the single inputMapProperty(). Yes, by "more engineering effort" I meant something like you do in your prototype - automatically register respective event handlers. I would, however, not make it the responsibility of the InputMap to register itself as event handlers. I would move this functionality to the Control. This would require to extend my previously proposed API of just one "void handle(Event e)" method to two methods: interface InputMap { void handle(Event event); Set<EventType<?>> getEventTypes(); } The control would observe its own inputMapProperty() and when it changes, unregister the old InputMap from all its event types and register the new InputMap with all its event types. Unfortunately, I cannot do this in a library (I can and am willing to prototype it somehow, but the usage would be cumbersome without support from Control). No reference from InputMap to the Control necessary. InputMap interface still has only methods that the Control needs it to have; nothing about how InputMap should be constructed. > 4) My thinking is that InputMap (in my current implementation, at least) makes sense as a class. I don't think we've discussed why we would want an alternative implementation of InputMap. Did you have one in mind? We have discussed it for a while now: template-based InputMaps (shared across all instances of a Control) is an alternative implementation of InputMap. > Is it not feasible that InputMap exist as it does now, and alternate implementations (such as WellBehavedFX) continue to work as they do now? That would mean WellBehavedFX, and alternative implementations in general, could not take any advantage of the new inputMapProperty() API on Control, which would be a bummer. Basically leaving them with the current state of things. > 3) That is surprising to me. I would have expected ifConsumed to only apply to otherNavigation, as I would have assumed if the event was consumed in edits that nothing further would happen. I'm not criticising, just pointing out a readability concern. If you rewrite the expression template = edits.orElse(otherNavigation).ifConsumed(action) to template0 = edits.orElse(otherNavigation) template = template0.ifConsumed(action) it is clear that ifConsumed applies to template0, which is (edits + otherNavigation). I think what confused you was that you thought this was still the builder pattern; it is not. Builder ended with a call to "create()" and these are operations on fully constructed templates. template0 above is a black box to the ifConsumed operator. Anyway, criticism is OK with me and welcome; I never take offense from comments about my code ;) Regarding 'auto-consuming', I agree that "It might be worth exploring."
02-04-2015
A few responses to your comments: 1a) Your last statement is unclear to me: "this is done for individual handler properties (like onKeyPressedProperty() above), but this would change to be done on Control#inputMapProperty()." Can you clarify what your thinking is here? 2a part 1) Just to clarify - I wasn't concerned about a template existing for a single action - just noting that it was interesting. Your follow-up comment is also interesting: "Once we have a single inputMapProperty() on the control, there would be just one template. Or I could have added more engineering effort and be able to obtain all event types that a template could possibly handle and register all the respective handlers (and no more)." For the first sentence, can you clarify (it's probably related to 1a above)? For your second sentence, if I understand correctly, this is what the InputMap does in my prototype (and is the reason why the InputMap now has a reference back to the control - as mappings are added and removed, the InputMap adds / removes its event handler for the relevant event type. Does that align with what you were saying? 2a part 2) I do think the issue around 'auto-consuming' events is an interesting topic. It is something that has occasionally reared its head in bug reports. Both our implementations (as well as the existing behaviors implementation) have this issue. It's almost that you want the input mappings to be possible with two separate lambda expressions, one which returns void (as they do now), or alternatively, one which returns a boolean or enum. It might be worth exploring... 3) That is surprising to me. I would have expected ifConsumed to only apply to otherNavigation, as I would have assumed if the event was consumed in edits that nothing further would happen. I'm not criticising, just pointing out a readability concern. 4) My thinking is that InputMap (in my current implementation, at least) makes sense as a class. I don't think we've discussed why we would want an alternative implementation of InputMap. Did you have one in mind? Is it not feasible that InputMap exist as it does now, and alternate implementations (such as WellBehavedFX) continue to work as they do now? ===== I don't think it is worth responding to your sum up quite yet, as it is clear I am not fully understanding your vision of how an InputMap would work. Hopefully the questions above will help clarify things.
01-04-2015
Thanks, these are good comments. 1a) Adding/overriding a mapping is done by creating a new handler and combining it with the current one. Something like orElse on templates, but since I cannot extend the EventHandler interface, there is a static helper method for that: EventHandlerHelper#chain(handler1, handler2). In fact, there is also a helper that checks whether a handler is installed or not and creates the chained handler if yes, so all it takes is EventHandlerHelper.install(control.onKeyPressedProperty(), newHandler); Here it is in action, overriding the Tab behavior in RichTextFX: https://github.com/TomasMikula/RichTextFX/issues/86 You can even remove previously "installed" handler, leaving original mappings in place: EventHandlerHelper.remove(control.onKeyPressedProperty(), newHandler); There is currently no support to disable a mapping without consuming the event. That would need a new combinator, something like template.ignore(EventPattern) for templates and EventHandlerHelper#ignore(EventHandler, EventPattern) for handlers. As you can see, this is done for individual handler properties (like onKeyPressedProperty() above), but this would change to be done on Control#inputMapProperty(). 1b) StyledTextAreaBehavior is not intended as public API and is public only because it is in a different package than StyledTextArea. 1c) Yes, new API for EventPattern. 2a) It seems like you are concerned about the fact that a template is constructed for just one action. The main reason why it is a separate template is that it operates on a different event type (KEY_TYPED), and the handlers for KEY_TYPED and KEY_PRESSED events are registered separately. Otherwise, it would fit nicely within the `edits` template (they also share the same predicate: area.isEditable). Once we have a single inputMapProperty() on the control, there would be just one template. Or I could have added more engineering effort and be able to obtain all event types that a template could possibly handle and register all the respective handlers (and no more). Regarding consumption based on return value: I would like if event handling worked this way in the first place, i.e. if we did not have to consume the event in the handler, but the handler would return true/false (or better yet, an enum) indicating whether it handled the event. That way you could not forget to consume the event, because you are forced to think what you return. In this particular case, though, the act(...) part in the builder is meant to mean that when it executes, it consumes, so that users don't have to add the boilerplate of "event.consume()" or "return true". I'm not insisting on this part, though. 3) The "ifConsumed" applies to (edits + otherNavigation), where otherNavigation is non-vertical navigation. ifConsumed feels like an ad-hoc addition, but it enables me to execute an action after any of a bunch of mappings consumed an event, thus I don't have to repeat that action in all of the individual mappings. This is not a crucial part of my approach, just very handy. 4) Saving the cost of the predicate property is one part, but what is more important to me is not polluting the InputMap interface with predicateProperty. If InputMap is an interface (my preferred way!), then predicateProperty makes it harder to implement. If it is a class, well, classes are difficult to override by themselves. About the runtime modification of the predicate: I'm not sure this can be useful. If end-users were to override the predicate, they would have to know the anatomy of the InputMap, which means knowing the implementation details. So the only entity that can reasonably override the predicate is the author of the InputMap, i.e. the author of the predicate. But in that case, whatever logic used for overriding the predicate can be incorporated in the predicate itself, eliminating the need to override the predicate. ======= About the differences in our approaches, I would sum them up differently: 1) Should input mappings be modifiable or immutable? (I say immutable.) 2) Is there just one meaningful way how to define input mappings? (I say no, there are more meaningful ways, and therefore no one way should be imposed on users by making InputMap a class.) My answers to 1) and 2) then enable you to define mappings as templates shared across controls, but you are still free to construct them per-control (and WellBehavedFX supports this as well: notice for example the EventHandlerHelper#chain(EventHandler...) method above, which is an analog of the template-level orElse() method). Then there is the unresolved problem of how to pass the reference to the visuals to the behavior. This does not have to be resolved now (or ever), but it would have to be resolved if Behavior or BehaviorSkinBase was going to be public API.
01-04-2015
I've taken a look at the code you linked to, and have a few comments / questions: ======= 1) In your approach, I can't see API for people to do a few things such as the things listed below. Did I miss them or do you have thoughts on them? a) Allowing developers to add or remove (or disable) a single input mapping from an instance of the control (we can of course assume that API can be added to Control for this, like in my prototype with the inputMap property - but I am interested to hear what you think needs to be done here for your implementation). To me this is one of the main benefits from bringing our behaviors code out of com.sun.* packages, so I'm keen to preserve this (and is one of the reasons why the InputMap class exists). b) In your mind, does the StyledTextAreaBehavior become public API (either accessible via the skin or the control), or is it hidden away as an implementation class that no one should use or subclass? It is mostly private methods, so I would presume you would expect that no one should ever use it. I ask because, in terms of JDK 9 modularity, we have a hard distinction between public and private (i.e. com.sun.* packaged code) - anything in com.sun.* is invisible to the world - it is not part of the scope during compilation or runtime. In the case that you expect this to be public API, how would people subclass this class to add in additional mappings if they were either creating a new RichTextArea control, extending your one, or just wanting entirely different behaviors / input mappings? c) Support for other event types (like mouse / touch / context menu / etc). I presume you would add additional API to the EventPattern interface? 2) On lines 130-142 of StyledTextAreaBehavior you define a KEY_TYPED_TEMPLATE that only has one action, but the template has a relatively complex predicate applied to it that operates on both the event and the behavior (and indirectly the control). This is interesting as the EventTemplate appears to largely be designed to only work with a single action in this case (or, only actions that meet all criteria of the predicates). This leads to two questions: a) I can understand why you put the 'where' statements in this builder, as opposed to testing the conditions in the keyTyped method (as if you did this, the action would be consumed even if the predicate was false and the method returned out of with no action being performed). Have you given any thought to this scenario, where we only conditionally consume the event, based on a return result or otherwise from the method? One option is to leave the consumption of events up to each individual mapping, but this is bound to be error prone (although, conceptually correct). b) My prototype does not currently offer any arguments into the predicate, but having seen this I think it makes sense to change the type of my InputMap predicate property from a BooleanSupplier to Predicate<Event>....so, I've made that change :-) 3) The KEY_PRESSED_TEMPLATE has an ifConsumed statement. This presumably only applies to the otherNavigation? The problem with builders is that sometimes the fluency doesn't help to make things clearer. 4) Referring to your point #2 - I'm not sure I see the value proposition of your point? I presume it is that by using the onlyWhen combinator you're saving the cost of the predicate property. My counter argument is that the cost would be negligible, and also allows for runtime modification of the predicate (by end-users of the control, as well). Did I misinterpret your point? 5) Regarding builders, yes, I would imagine that these would be a no go, given the direction JavaFX has gone on builders in the past (both in terms of the automatically generated builders API, as well as the reworking of the proposed dialogs API to remove builders). I don't have the stomach to propose builders again :-) ======= That's all I can think to ask right now. I think in general the discussion between our approaches boils down to a few questions (correct me if you disagree): 1) How much API do we give to end-users to modify the input mappings of individuals control instances? 2) How should mappings be defined? As a template where all controls share a single set of mappings, or should they be instantiated per-control? My opinion is that with the approach taken in my prototype, yes, we end up with slightly more API in InputMap, but we end up with a bit more flexibility being given to end-users. I'm keen to hear your thoughts! :-)
31-03-2015
Hi Jonathan, thanks for the patch. It would be much easier to have a branch on bitbucket ;). For example, what repository should the patch be applied against? Anyway, I will have a very busy upcoming two weeks, so I will probably not be able to prototype this against OpenJFX. However, most of what I am proposing is in some way implemented in WellBehavedFX [1], so I encourage you to look there (package org.fxmisc.wellbehaved.event). Have a look at how it is used to implement behavior in RichTextFX [2]: of interest are the first 143 lines of code. Notice a few things: 1) It uses the "template" approach I mentioned above, so that the mapping hierarchies are shared among all controls (notice that KEY_PRESSED_TEMPLATE and KEY_TYPED_TEMPLATE are declared static). On lines 199-200, these templates are instantiated to actual EventHandlers. 2) Notice how the "edits" template, which is a "child input map" of KEY_PRESSED_TEMPLATE, uses the predicate "isEditable" (line 66). Predicate is added via the "onlyWhen" combinator, instead of imposing a "predicateProperty" on all InputMaps. 3) On lines 126-128, see how KEY_PRESSED_TEMPLATE is composed from child templates using the "orElse" combinator. 4) On lines 202-203 see how the handlers are installed. "installAfter" is a helper method that checks if an event handler is already installed, and if so, replaces it with a composition of the original one and the new one, the original taking precedence (so that the skin does not override user-defined mappings). You will also notice that it uses the builder pattern to construct the templates, but that is not essential. It is also constructing separate handlers for separate event types, but that's not essential either. [1] https://github.com/TomasMikula/WellBehavedFX [2] https://github.com/TomasMikula/RichTextFX/blob/master/richtextfx/src/main/java/org/fxmisc/richtext/skin/StyledTextAreaBehavior.java
31-03-2015
Attaching the latest code from my InputMap exploration repo. I should note that this is entirely prototype exploration and the code is actually the same as it was a few weeks ago. It is no where near final! I just figured that if Tomas was going to work on a prototype he might like to see all the changes I've made. Enjoy! :-)
30-03-2015
@Hendrik: Regarding having predicate on an InputMap, my thinking was that this was a way to define branching input maps that are enabled / disabled on some higher-level concept (such as the control state (e.g. ListView orientation) or operating system). I didn't like the idea of having a predicate on each individual mapping, as it seemed like too much of a mental overhead (i.e. it might not be all that pleasurable writing out the input mappings if a large bunch of the mappings need a predicate to toggle their state). If we take the predicate off of InputMap, all of a sudden I'm not sure there is any sense in allowing InputMap hierarchies - it would seem you could just put all mappings into a single InputMap. Regarding having some way to toggle a mapping off (and then back on), I (briefly) thought about this, but figured that this could be handled in at least two other ways that it might not be worth having the extra API. The two ways are: 1) having the method that the mapping calls check state and only optionally perform the action, or 2) remove the mapping from the InputMap when it should be disabled. I admit that both of these have their downsides, so I'm not against having API on the mapping class to disable a mapping. I think it comes down to the question of how often this is necessary. @Robert: The intent of this jira issue is to discuss and define an improvement over the current Behavior private API. The primary benefit of the kind of API being discussed here is that the input mappings can easily be manipulated by developers at runtime. The existing BehaviorBase private API is not ideal as the mappings are not easily modified. @Tomas: I look forward to your prototype - it'll be very informative to better understand your thinking. Hopefully it can bring a different approach entirely to the discussion (whilst still offering roughly the same functionality as exists in the current prototype).
30-03-2015
Hopefully, I will be able to get to this next week. If only we had the bitbucket mirror to start with ;)
27-03-2015
Typing on my phone, so I'll be brief. I just wanted to say that I'll be returning to this topic next week, and Tomas, if you have the bandwidth available can I encourage you to clone the openjfx repo and try prototyping your approach against the actual code base? That may inform further discussion.
27-03-2015
Hendrik brought up an important use case, which reinforces me in thinking that InputMap interface should be minimalist (and flexible) rather than trying to anticipate and cover all possible use cases (and be rigid). Should there now be predicate on InputMap and enabledProperty on Mapping? That is getting complicated and introduces unnecessary overhead (most mappings will not use the enabledProperty). Thus I would again like to argue for minimalist interface and smart helper methods for composition of InputMaps. Note that the case of disabled button is nicely handled by the `onlyIf` helper method sketched above. Also note that when stripped down to the essence, there is not difference between an InputMap and Mapping - they can both be abstracted by the same interface: handle(Event e).
27-03-2015
I like Jonathans approach :) Using a simple predicate for OS specific mappings sounds like a good idea. Why is the predicate defined in the InputMap instead of the Mapping class? Is there a way to easily active and deactivate a mapping? If there is a defined mapping for a mouse click I maybe want to deactivate this if the button is disabled. By doing so I don't need to check the button state in the action and can simply bind the enabled state of the mapping to the disabled state of the button. By doing so you can use the predicate for general definition that normally won't change at runtime (OS specific actions) and the enabled property can be used for runtime dependent changes.
27-03-2015
I believe that BehaviorSkinBase should also be promoted to the public API together with BehaviorBase, as it is an extremely useful class when it comes to creating custom controls.
27-03-2015
The API you are proposing combines two aspects: the event handler aspect (handle(Event) method) and how that event handler is going to be constructed (list of mappings, list of child maps, predicate). These two concerns can be separated, which alone seems like a good argument to separate them. The use case I have in mind is the one I mentioned earlier - have unmodifiable input maps (I called them InputMapTemplates) so that they can share the key mappings, instead of instantiating all the mappings for each instance of the control. I haven't suggested, nor will I ever suggest to use type casting. I previously suggested composition of InputMaps as a way to add/override mappings. About the other methods: What is the use for getControl()? How can you ensure dispose() being called when users can freely replace the InputMap? When is parentInputMap property going to be used? With regards to behavior referencing back to the visuals, Control.getSkin() would be a terrible recommendation for precisely the reason you gave.
20-03-2015
With regards to behavior referencing back to the visuals, the other API I haven't mentioned is of course the Control.getSkin() method, but relying on getting a certain skin implementation here may cause grief if people decide to replace control skins.
20-03-2015
The current public API on InputMap is the following: public C getControl() public ObservableList<Mapping<?>> getMappings() public ObservableList<InputMap<C>> getChildInputMaps() public void dispose() public void handle(Event e) In addition, there are properties for parentInputMap and predicate. The handle(Event) method would work as a private method too, so that can go either way. However, I would imagine having Control return an InputMap interface with only handle(Event) on it as being too restrictive for most users - they would inevitably be forced to look into javadoc or do a google search to cast the InputMap into an implementation, with which they can modify the mappings list, for example. Unless there is a good argument for having an interface, my inclination is to have InputMap be a class, to make users lives easier.
20-03-2015
Can you clarify what use cases you see for having different InputMap implementations?
20-03-2015
I agree that "unholy back-reference" or callbacks don't need to (and should not) be in the API. But I also think that they cannot be the recommended approach. "Unholy back-reference" means that a not fully constructed `this` escapes the constructor. This cannot be the recommendation. The callback approach means that the Behavior is not "fully constructed" until callbacks are set, which is error prone. You need to do TextAreaBehavior behavior = new TextAreaBehavior(textArea); // behavior not yet valid behavior.setSomeCallback(callback); Note that this is very different from, e.g. ScrollPane scrollPane = new ScrollPane(); // scrollPane fully constructed scrollPane.setContent(content); where scrollPane without content is a valid Node and can be used in the scene graph. The callback above, however, is essential for proper function of TextAreaBehavior. It also gets ugly very quickly if you need several callbacks. This bugs me because the visuals don't need a reference to behavior, so there should be an easy way for the behavior to have a reference to visuals, right? I would argue for InputMap being an interface instead of a class. From my experience, the inherited behavior often gets in the way when trying to provide custom implementation of something. I also imagined it simpler than what I can infer from the code snippets. At its bare bones it could be just interface InputMap { void handle(Event); } which is actually isomorphic to Consumer<Event>. Not every InputMap is going to have a predicate, so predicate should not be part of the InputMap API. Implementation of an InputMap with a predicate would look like this: void handle(Event event) { if(predicate.getAsBoolean()) { // handle event } } There could be a helper (default?) method to "set" the predicate: interface InputMap { void handle(Event event); default InputMap onlyIf(BooleanSupplier predicate) { return event -> { if(predicate.getAsBoolean()) { handle(event); } }; } } The main point is that the InputMap interface stays simple and easy to implement.
19-03-2015
At present I have not crossed the bridge of how (or if) the behavior / input map should reference the skin. There is a hard reference to the control however. There are a few options, so I'm keen to hear your thoughts. I can see that in TextAreaBehavior there is an 'unholy back-reference'. This is one option - given that the behavior class is instantiated by the skin, it is possible for the skin to pass itself into the behavior if necessary. In other Behavior classes we avoid doing this by having a bunch of setters that receive Callback, Runnable, etc, which are set by the skin. This way the behavior calls the Callback / Runnable without actually referencing the skin. I'm quite pragmatic about this however and don't think anything needs to be done in terms of API. Thoughts? Here's an example of the use of predicates in the NewListViewBehavior class. It adds Mac OS X specific behavior, and all other platforms get the 'normal' behavior: // create OS-specific child mappings // --- mac OS InputMap<ListView<T>> macInputMap = new InputMap<>(control); macInputMap.setPredicate(() -> PlatformUtil.isMac()); addDefaultMapping(macInputMap, new KeyMapping("shortcut+ctrl+space", e -> toggleFocusOwnerSelection())); listViewInputMap.getChildInputMaps().add(macInputMap); // --- all other platforms InputMap<ListView<T>> otherOsInputMap = new InputMap<>(control); otherOsInputMap.setPredicate(() -> !PlatformUtil.isMac()); addDefaultMapping(otherOsInputMap, new KeyMapping("ctrl+space", e -> toggleFocusOwnerSelection())); listViewInputMap.getChildInputMaps().add(otherOsInputMap); This is the degenerate case where we only have one mapping per input map, so it does appear verbose. ListViewBehavior also adds two other InputMaps, one for vertical ListView and the other for horizontal ListView, in exactly the same way as above, but the code-to-mapping ratio is a little better :-) I'm on the fence about the NewBehavior class. It might be useful but at present it isn't really, so I tend to agree that it doesn't need to be public API. FocusTraversalInputMap is still a work in progress. It already has changed from extending InputMap to just being a class with a private constructor and a single getTraversalInputMapping() method. Therefore, the way to use it has changed to the following: // create a map for button-specific mappings (this reuses the default // InputMap installed on the control, if it is non-null, allowing us to pick up any user-specified mappings) buttonInputMap = createInputMap(); // add focus traversal mappings addDefaultMapping(buttonInputMap, FocusTraversalInputMap.getFocusTraversalMappings()); This updated approach also installs the mappings only if there is no mapping set already.
19-03-2015
Hi Jonathan, thanks again for sharing. I would be interested to see an example where the behavior actually depends on the visuals. Not sure if there is such a case in ListView, but there certainly is in TextArea: the behavior for Up and Down arrow keys needs to consult the visual rendering in order to find out the text position at which to place the caret. Also, I would like to see the use of predicates. It seems to me Behavior or BehaviorBase don't need to be public API; they are not adding much value? Neither does FocusTraversalInputMap have to be a public class. The constructor you used in the code could be replaced by a static factory method returning InputMap. From the code you posted, it seems to me that the FocusTraversalInputMap would override any user- or skin-defined mappings, for example inserting Tab in TextArea. This may not be the case, since I don't know the details of FocusTraversalInputMap.
19-03-2015
I've spent some time working on a new prototype, and have transferred Button and ListView behaviors over to use the new API as proof of concept. At present it sticks to my thinking of having an InputMap whose mappings can be modified. I don't want this post to get bogged down in technicalities (yet), so I'll just call out a few small chunks of code for discussion. Firstly, here is some code out of a test application that adds support for the 'J' button to fire events, and also replaces the default mapping for the space key: button1.getInputMap().getMappings().add(new KeyMapping(J, e -> button1.fire())); button1.getInputMap().getMappings().add(new KeyMapping(SPACE, e -> { System.out.println("This should replace the default action event!"); })); Secondly, here are the first three lines from the ButtonSkin constructor: // super(button, new ButtonBehavior<Button>(button)); super(button, new BehaviorBase<Button>(button, Collections.<KeyBinding>emptyList())); button.setInputMap(new NewButtonBehavior<>(button).getInputMap()); The first line is commented out as it was installing the old behavior. The second line is a placeholder line and retains the old API but installs an empty Behavior (using the old private API). The third line is the relevant line - it creates a NewButtonBehavior class with a reference to the Button, and sets the InputMap onto the button. Here's some of the code from the NewButtonBehavior class: public NewButtonBehavior(C control) { super(control); // create a map for button-specific mappings (this reuses the default InputMap installed on the control, if it is non-null, allowing us to pick up any user-specified mappings) InputMap buttonInputMap = createInputMap(); addDefaultMapping(buttonInputMap, new KeyMapping(SPACE, KeyEvent.KEY_PRESSED, this::keyPressed)); addDefaultMapping(buttonInputMap, new KeyMapping(SPACE, KeyEvent.KEY_RELEASED, this::keyReleased)); addDefaultMapping(buttonInputMap, new MouseMapping(MouseEvent.MOUSE_PRESSED, this::mousePressed)); addDefaultMapping(buttonInputMap, new MouseMapping(MouseEvent.MOUSE_RELEASED, this::mouseReleased)); addDefaultMapping(buttonInputMap, new MouseMapping(MouseEvent.MOUSE_ENTERED, this::mouseEntered)); addDefaultMapping(buttonInputMap, new MouseMapping(MouseEvent.MOUSE_EXITED, this::mouseExited)); // add mappings to root map (which includes the focus traversal mappings) rootInputMap = new FocusTraversalInputMap(control, buttonInputMap); // Button also cares about focus control.focusedProperty().addListener(this::focusChanged); } The addDefaultMapping method is part of the NewBehaviorBase class (which may or may not be public API) that simply checks to see if a mapping with the given parameters already exists, and if so doesn't set the new mapping. This is how we can install the 'J' and 'space' mappings before the skin is constructed. That's about it. The NewListViewBehavior is more complex, but all it really does differently is create four five InputMaps (rather than just the two above), and set a few predicates to ensure only certain InputMaps are considered based on system and control state. At a very high level, there is very little changed conceptually, except now the behaviors are visible and accessible via the Control in a public way. Thoughts?
19-03-2015
Thanks for clarifying your thinking. When time permits I'll work on some prototypes and will publish them. My main goal is to go with the simplest and lowest-overhead API possible, that can cover all use cases that need to be covered.
13-03-2015
I had some assumptions about the InputMap which I see we don't quite share. That's probably where the misunderstanding is coming from. My assumptions are that 1) an InputMap is unmodifiable, and 2) actual mappings are only in the leafs of the InputMap hierarchy. You override a key mapping not by modifying it in the InputMap, but by creating a new InputMap with two children: the InputMap with the new mapping and the old InputMap. When this new InputMap is asked to handle an event, the mapping from the first child will take precedence over the mapping from the second child, thus effectively overriding the old mapping. About sharing the InputMap hierarchy among all instances of a control: What I have in mind would actually require two types, InputMap and InputMapTemplate. InputMapTemplate would have a method instantiate(ButtonSkin) which returns InputMap that you can add to the control. Let's sketch some code: interface InputMap { void handleEvent(Event event); } interface InputMapTemplate<S> { // S is the type of the Skin, e.g. ListViewSkin void handleEvent(Event event, S skin); default InputMap instantiate(S skin) { return event -> this.handleEvent(event, skin); // or, if InputMap is not a @FunctionalInterface: return new InputMap() { @Override public void handleEvent(Event event) { InputMapTemplate.this.handleEvent(event, skin); } } } } ListViewSkin can then have one InputMapTemplate hierarchy as a private static field, and then "instantiate" it to an InputMap for each ListView instance. The main idea here is that for each instance of the control, only a thin wrapper around the hierarchy of mappings needs to be instantiated, instead of recreating the whole hierarchy for each control. The hierarchy is shared across controls. I by no means meant to expose a method like getSkin() in the InputMap API. We seem to be in agreement that the InputMap may need to enclose a reference to the skin (captured either by your BooleanSupplier, or during instantiation of the template).
12-03-2015
Great feedback - thanks! A) My thinking is to shift towards not having any 'getBehavior()' method on Skin, and to instead have a 'getInputMap() / setInputMap() / inputMapProperty()' API on Control. This would be the behavior, and it would be installed by the skin. To jump forward to your point in F), one way to resolve this is to ensure that InputMap has some means of querying whether a certain event mapping exists, and if so, to not install the mapping. This would allow for people to set their mappings before the skin is created, and the skin would then use this API to only install the mappings that are not overridden by the user. D) Violent agreement! :-) E) I think this needs to be explored further: i) With regards to the predicate, my thinking was that this was only used to determine if the input map (and its children) should even be considered. ii) It seems you want there to be a single InputMap defined for, e.g., Button, but you also want the InputMap to have a reference to the skin? I must be misunderstanding something critical. For example, how does this work when the skin would do something like buttonInstance.setInputMap(ButtonBehavior.getSingleInstance())? This would result in all buttons sharing a single input map, and if the user modifies one button input map, they will modify all? It seems ok to me to have the behavior / input map created per instance (and therefore, a BooleanSupplier is fine and removes the requirement that InputMap know about its Control / Skin). I'm sure I'm misunderstanding your point, so I'm not set in stone on this at all - just trying to understand. iii) To put forward my thinking (at a very early stage): I would probably have the skin create a new instance of the ButtonBehavior class. This ButtonBehavior would have a reference to the skin (and through that, the control). The ButtonBehavior would create an InputMap hierarchy (with the focus traversal mappings at the root, and then additional mappings for Button-specific behavior). The skin would then install this behavior into the control. For controls with control state specific behavior (e.g. listview orientation), the input map hierarchy would have children input maps that use the reference to the control as a predicate. Because the control is already available, a BooleanSupplier is adequate, and also means that the InputMap does not have to contain any reference to the Control, Skin, or Behavior classes.
12-03-2015
> Ugh, I hate jira discussions when they devolve to a number of bullet points - they slowly become less and less able to be comprehended! :-) Is replacing numbers with letters supposed to help? :D A) Yes, it is not clear to me who would be the caller of getBehavior(). Maybe you mean getBehavior() to be protected and called by SkinBase constructor itself? If that is the case, then I don't see a big difference between overriding getBehavior() and overriding the constructor directly. I was also saying that a reference to the View (whatever it is) should be passed to the Behavior constructor (or behavior factory or whatever constructs the Behavior), so that Behavior can register listeners on the View. D) If by "and then add in additional mappings on top of this" you mean "use this default mapping as part of their own InputMap hierarchy", then we are in agreement. E) There is the following problem with BooleanSupplier: it will have to be a closure over the control/visuals. This means that the whole InputMap hierarchy will have to be recreated for each control, in order for each individual predicate to "close over" the right instance of the control. If the predicate is parameterized by the control/visuals, then the InputMap hierarchy can be built once for all controls of that type, and only the final Behavior will close over the specific instance of the control. Also refer back to A) where I argue that Behavior should have a reference to the visuals. F) That is indeed an alternative. It is however not going to be as simple as inputMap.replaceMapping(KeyCode.TAB, e -> { ...}), because this code will typically be run before the instantiation of the skin, thus the skin would still override this user-defined mapping. Using WellBehavedFX, I call something like this from Skin: installAfter(control.onKeyPressedProperty(), behaviorKeyPressedHandler) which with inputMapProperty would become installAfter(control.inputMapProperty(), behaviorInputMap) It means that the input map provided by the skin (behaviorInputMap) is installed with lower precedence than whatever current InputMap there already is. To compose and decompose EventHandlers, WellBehavedFX uses an approach as described in https://javafx-jira.kenai.com/browse/RT-38898. I suppose something similar will be supported by the InputMap.
12-03-2015
Ugh, I hate jira discussions when they devolve to a number of bullet points - they slowly become less and less able to be comprehended! :-) A) What would you propose? It seems as if you're suggesting that Skin / SkinBase should _not_ have an overridable getBehavior() method that, by default, returns a no-op behavior, but I'm not fully understanding what your thinking is. The fact is, creating a Behavior (probably in the form of an InputMap) and registering it on the control can be an optional thing done by skins. One approach would be to essentially keep the current control structure where we have, for example, ButtonBehavior. This is instantiated by the skin and installs the relevant listeners on the control. B) "This can be handled automatically and transparently for the custom control developer." I was just meaning that the developer ideally would not have to concern themselves with calling addEventHandler(KeyEvent.ANY, ...), etc - we should abstract away the exact listeners we install so that they may only concern themselves with providing the appropriate input mappings. C) Yes - "bindings" is lazy of me :-) I have tried to avoid using any concrete names for now, but the one I've used in previous code is 'mapping' and 'input map', so perhaps we use that for now. D) Composition vs inheritance: What I was thinking was that we would have an InputMap class that contained a list of mappings. We could build a hierarchy of these for OS-specific / property-specific requirements. Additionally, we could have some form of FocusTraversalInputMap (better name pending!) that we could use by default - most behaviors would use this and then add in additional mappings on top of this. E) BiPredicate<C,E> is a little tough (for now), because the InputMap has no reference back to the control or its visuals. Another option is to use BooleanSupplier, and to leave the job of determining the boolean up to the person defining the method (at which point they should have reference to the control, skin, etc). F) User-overridable event handlers. This is an important use case. The problem as it stands is that we're discussing the input map as if it hidden behind the skin. Perhaps the alternative approach is to have Control have an inputMap property. This allows for skins to populate this map as appropriate, and also allows for there to be public API to modify the input map. Then, the case of replacing the tab key method is simple (in pseudocode: inputMap.replaceMapping(KeyCode.TAB, e -> { ...}). This approach also should resolve your complication.
12-03-2015
Hi Jonathan, thank you for sharing your thoughts with us. Here are some of my early comments. > The Behavior of a Control should be a property of its Skin. This is because the inputs that are possible on a Control is dictated by the visuals provided by its Skin. I agree Behavior should be part of the Skin. At the same time, I am convinced it should _not_ be part of the View (or visual) part of the Skin. (I see Skin as an encapsulation for both View and Controller.) I believe one should be able to define (i.e. construct) visuals (View) without any reference to Behavior. On the other hand, to construct Behavior, one is definitely going to need a reference to the View (except for very simple controls). > Therefore, we should update the Skin / SkinBase API such that it returns some form of Behavior implementation when requested. It is not clear to me that it will be necessary to expose Behavior in the Skin API. Who will need to have access to the instance of Behavior? Only use case I can think of is overriding default event handlers, but doing it this way does not play well with Skin-less Controls > 1) Support for all event types - mouse, keyboard, touch, contextMenu, etc > This means that the behavior (or skin) must add the relevant event handlers onto the control at construction time. This can be handled automatically and transparently for the custom control developer. Can you elaborate what you mean by the last sentence? > 2) Hierarchical bindings > In other words, we should be able to build a tree of behavior bindings. This is important for some of the other requirements below. Agree. Can we just come up with a different word for "bindings", because that word is already used in JavaFX for something else (namely javafx.beans.binding). > 3) Support for focus traversal > Focus traversal is already part of the Behavior implementation code, so this should continue to be the case. Focus traversal behavior could therefore be defined in the BehaviorBase class. I would like to appeal for composition instead of inheritance. If there is going to be a need for Behavior type, make it an interface, not a class. Make the default focus traversal behavior available for composition with developer defined behavior, in the sense of 2). > 4) Support for state-dependent behavior (e.g. horizontal vs vertical listview) > In classes like ListViewBehavior, there is code that states that certain behaviors are valid only for horizontal ListViews, and other behaviors are only for vertical ListViews. I was thinking one way of implementing this would relate back to the hierarchical bindings, such that the ListView Behavior would have a root set of bindings (i.e. those provided by BehaviorBase for focus traversal, etc), and then two children bindings. The binding class would allow for a predicate to be stated, that would be checked when events occur. In the case of ListViewBehavior, the predicates would be isVertical() and isHorizontal(). In this way, only the bindings that meet the predicate would be considered. Yes, I imagine something like BiPredicate<C, E>, where C is the type of the Control (e.g. ListView), and E is the type of the event (e.g. KeyEvent). Alternatively, C could be the type of the Visual (the View part of the Skin). > 5) Support for OS-specific behavior > In much the same vein as requirement 4 above, we will need to support OS-specific behavior, and perhaps some convenience could be provided to pre-populate the predicate, for example. Agree. > I do not consider any degree of actions, or method annotations, to be in scope for this investigation. I am glad for that ;) I would add 6) Support for user-overridable event handlers. For example, in TextArea, override Tab key to insert 4 spaces instead of a Tab. Note that there's a little complication here, because ability to override keyboard shortcuts would be desirable even for Skin-less controls, but there is not going to be any Behavior if there is no Skin.
12-03-2015
Here's a bit of a brain dump on my thoughts on this topic. They are not overly refined and everything is open for discussion. The Behavior of a Control should be a property of its Skin. This is because the inputs that are possible on a Control is dictated by the visuals provided by its Skin. Therefore, we should update the Skin / SkinBase API such that it returns some form of Behavior implementation when requested. The requirements for a Behavior include the following: 1) Support for all event types - mouse, keyboard, touch, contextMenu, etc This means that the behavior (or skin) must add the relevant event handlers onto the control at construction time. This can be handled automatically and transparently for the custom control developer. 2) Hierarchical bindings In other words, we should be able to build a tree of behavior bindings. This is important for some of the other requirements below. 3) Support for focus traversal Focus traversal is already part of the Behavior implementation code, so this should continue to be the case. Focus traversal behavior could therefore be defined in the BehaviorBase class. 4) Support for state-dependent behavior (e.g. horizontal vs vertical listview) In classes like ListViewBehavior, there is code that states that certain behaviors are valid only for horizontal ListViews, and other behaviors are only for vertical ListViews. I was thinking one way of implementing this would relate back to the hierarchical bindings, such that the ListView Behavior would have a root set of bindings (i.e. those provided by BehaviorBase for focus traversal, etc), and then two children bindings. The binding class would allow for a predicate to be stated, that would be checked when events occur. In the case of ListViewBehavior, the predicates would be isVertical() and isHorizontal(). In this way, only the bindings that meet the predicate would be considered. 5) Support for OS-specific behavior In much the same vein as requirement 4 above, we will need to support OS-specific behavior, and perhaps some convenience could be provided to pre-populate the predicate, for example. I'm sure there are other topics to discuss, but this might be a good starting point. I should conclude that some of my earlier comments and explorations into actions is almost without doubt taking this exploration too far and in the wrong way. I do not consider any degree of actions, or method annotations, to be in scope for this investigation.
11-03-2015
YEAH :)
11-03-2015
Just a heads-up that I will be returning to this topic in the coming days/weeks/months as exploration for JDK 9. I'm by no means ready for any technical discussion (because I have nothing to add), but perhaps this note might inspire people to go out and freshen up their prototypes and starting thinking about how behaviors should progress towards becoming public API.
11-03-2015
Hi Jonathan, I finally started such a project: https://github.com/TomasMikula/WellBehavedFX
25-09-2014
Tomas, as I said I can't recall the details of the patch and right now don't have the time to bring myself back up to speed, so I'll need to hold back from speculating on why the approach was the preferred one when we last looked at it. Regarding the patch being independent from JavaFX internals, I'm not sure it is unless you're specifically calling out the InputMap code, which seems pretty simple. Most of the patch is applying that API to the existing behavior code to test out its efficacy. This patch is not part of my current focus (and even then, it was only for exploration and prototyping, not intended as a final product), so I won't be able to spend time taking it apart and creating a mini open source project out of it. You're more than welcome to borrow the concepts and take it further - I'm sure there is a lot of interesting stuff in there that could simplify the behavior code and also make it easier for people to modify the key/mouse behavior of UI controls.
18-06-2014
Hi Jonathan, isn't the remapping solved by the InputMap hierarchy? If I want to override a mapping, I create a new InputMap with the original one as its parent, and override the key combination in the new one. Also, can mapping multiple key combinations onto a single action be easier than this? inputMap.addMapping(new KeyMapping(evt -> redo(), "ctrl+y")); inputMap.addMapping(new KeyMapping(evt -> redo(), "ctrl+shift+z")); The current approach is inputMap.addMapping(new KeyMapping("Redo", "ctrl+y")); inputMap.addMapping(new KeyMapping("Redo", "ctrl+shift+z")); getActionMap().put("Redo", evt -> redo()); Regarding patches... all this InputMap et al. seems to me pretty independent from any JavaFX internals. It would be nice if, for the time being, it was a separate mini-project, or just a gist on github, that people could play with, fork and eventually send pull requests. It would be much easier to work with than a patch against a 2 years old snapshot.
18-06-2014
@Hendrik: The ActionMethod annotation is not part of the most recent patch - and is not something I ended up wanting to use (from November 2012). @Tomas: Initially I did do it the way you propose, but this makes it more difficult for users of controls to easily remap input events to actions. In the approach I have in my patch, it is possible for a developer to easily remove all keymappings related to a particular key combination, or to map multiple key combinations onto a single action. It's not a big sell, but I did find the separation useful when I was actually in the code (although admittedly I am a bit rusty on this code, as it is quite a long time since I last touched it). In any case, there is no 'definite right approach' for this yet - I only went a small way down this path, but definitely something needs to be done before behavior can become public API. I'm always open to suggestions and patches! :-)
17-06-2014
I like the concept of InputMap. The way it is used in your latest patch is that a Mapping (e.g. key mapping) is looked up in InputMap, and then its name is used to look up an action in an action map. I would like to suggest an alternative where the Mapping already holds the action (i.e. the EventHandler). The InputMap.lookup() method would return the event handler instead of action ID. This would eliminate the need for a separate action map. To populate the input map, instead of inputMap.addMapping(new KeyMapping("TraverseUp", KeyCode.UP)); getActionMap().put("TraverseUp", evt -> traverseUp()); one would write inputMap.addMapping(new KeyMapping(evt -> traverseUp(), KeyCode.UP)); To handle an event, instead of String actionName = getInputMap().lookup(e); if (actionName != null) { EventHandler evtHandler = getActionMap().get(actionName); if (evtHandler != null) { evtHandler.handle(e); } e.consume(); } one would write EventHandler evtHandler = getInputMap().lookup(e); if (evtHandler != null) { evtHandler.handle(e); e.consume(); } or, if InputMap returned Optional<EventHandler> instead of nullable EventHandler, one would write getInputMap().lookup(e).ifPresent(evtHandler -> { evtHandler.handle(e); e.consume(); }); This approach also eliminates the need for any annotations (and thus reflection). IMO, method annotations establish the mapping between events and actions rather indirectly.
16-06-2014
I checked the patch and really like the @ActionMethod annotation. We currently use a similar approach in DataFX (http://www.guigarage.com/2014/05/datafx-tutorial-1/). A tooltip / description value is currently missing and I would change the name() field to value(). Another problem is the graphic() value: by using a String you can simply define an image that can be used as the icon for the action. But I think a SVG or Font icon (like Awesome Font) is better today. So maybe an additional enum to define the graphic type is needed here: Usecase 1: using an enum in the annotation to define a SVG icon: @ActionMethod(graphicsType= SVG_PATH, graphics= "M150 0 L75 200 L225 200 Z") Usecase 2: using an addition annotation to define a graphic factory method in the class: @ActionMethod(graphicsType= FACTORY_METHOD graphics="createReloadGraphic") in class: @GraphicFactoryMethod("createReloadGraphic") private Node createTheGraphic() { .... } Usecase 3: Use an image: @ActionMethod(graphicsType= IMAGE_RESSOURCE, graphics= "path/to/image") About the KeyEventMethod Annotation: I would use a enum for the different operation Systems and then define an array in the Annotation: supportedOs = {MAC, WIN, ...}
16-06-2014
Just recently I updated SkinBase to remove all references to Behavior. I introduced a new private class BehaviorSkinBase that retains the behavior. This allows for this jira issue to be untargeted from JavaFX 8.0.
05-12-2012
Attaching patch that included an early InputMap prototype. This patch was sitting in my main repo and gathering dust, so I thought I would push it here so it doesn't get lost or forgotten. This is by no means final!
09-11-2012
Attaching a new patch that builds on top of the concept in the initial patch (although it is far more fleshed out). This time the patch introduces the concept of input and action maps on Control (and subclasses). This means that Behavior will eventually become an implementation detail for JavaFX UI controls, and will never be public API. Behaviors now simply populate the input and action maps. To make populating the input and action maps, this patch also introduces two annotations and an annotation processor that can find annotated methods and populate the relevant maps. There are however problems, most notable is that there needs to be support for key press and / or key released events, but KeyCombination does not support this notion. This is not a big issue, but definitely something that needs to be considered. Finally, this patch is by no means complete: it is working where it needs to work, but it is stubbed out in the peripheral areas (such as resource management and i18n support).
17-09-2012
Attaching a patch file containing a very quick exploration into using annotations to register behavior methods against their relevant keystrokes. This is nothing more than a proof of concept of one possible direction we may go. In fact, the patch continues to retain the concept of a 'action name', rather than use something smarter such as method handles.
21-05-2012

Blocks :	JDK-8096725 - RT-34620: [ComboBox, DatePicker] Buttons set to default/cancel are not reacting to ComboBox enter/esc keys
Duplicate :	JDK-8100681 - Make SkinBase and BehaviorBase as public API
Relates :	JDK-8092194 - ☂ Improve support for building custom UI controls
Relates :	JDK-8091131 - [Actions] Implement Actions (many toolkits have them, such as Qt)
Relates :	JDK-8091976 - Expose TextInputControl undo(), redo(), canUndo(), canRedo() methods
Relates :	JDK-8314968 - Public InputMap (v3)
Relates :	JDK-8300569 - ☂ Missing APIs related to rich text control
Relates :	JDK-8092468 - Move SkinBase into public API
Relates :	JDK-8092211 - Promote all skin and behavior classes of the default controls to the public API
Relates :	JDK-8092401 - Change BehaviorSkinBase registerChangeListener to use Callback functional interface rather than strings
Relates :	JDK-8343646 - Public InputMap (Incubator)