Update RichTextFX to use InputMap API from WellBehavedFX's experimental package

[JordanMartinez]

I know you already want this done. So, I'm starting this issue to help facilitate the discussion of issues I come across as I implement this.

[JordanMartinez]

One issue I've run into is the EventPattern#anyOf(EventPattern, EventPattern). It seems to imply that it will take a variable amount of arguments anyOf(EventPattern...) rather than just two. Specific to RichTextFX, StyledTextAreaBehavior currently has 3 handlers for the same action (cut). This could be reduced into the following code and thus increase readability.

consume(
    anyOf(
        keyPressed(CUT), 
        keyPressed(X, SHORTCUT_DOWN), 
        keyPressed(DELETE, SHIFT_DOWN)
    ), e -> view.cut())

So, either this method should be turned into a variable argument method or there should be additional methods that provide the same function for 2-5 arguments.

[TomasMikula]

Good point, it should take variable number of arguments.

[JordanMartinez]

Another minor issue of inconvenience:
I can create an input map via sequence(), but can't dictate when all of its components should work (e.g. when the view is editable). Currently, I have to do

InputMap<?> baseEdits = sequence( /* the edit stuff */ );
InputMap<?> edits = InputMap.when(view::isEditable, baseEdits);

instead of

InputMap<?> edits = sequence( /* edit handlers */ ).onlyWhen(view::isEditable);

[JordanMartinez]

Just a question, but is the charPressConsumer still needed in this new API? I'm not sure why it's being consumed in the first place.

[TomasMikula]

There sure could be a convenience method on InputMap#onlyWhen. But maybe

when(condition, action)

is a little bit clearer than

action.onlyWhen(condition)

which is kind of reverse.

Regarding charPressConsumer: When a character key is pressed, it produces both KEY_PRESSED and KEY_TYPED events. We handle the KEY_TYPED events for character input. charPressConsumer is there to prevent the corresponding KEY_PRESSED events from bubbling up to parent nodes.

[JordanMartinez]

maybe when(condition, action) is a little bit clearer than action.onlyWhen(condition).

Good point.

charPressConsumer is there to prevent the corresponding KEY_PRESSED events from bubbling up to parent nodes.

Ok.

[JordanMartinez]

Another issue: otherNavigation clears the target offset if the event has been consumed, but I'm not sure how to do the equivalent with the new package.

[JordanMartinez]

Another issue: 1 (static) template is used to work across multiple areas (saves memory). How would that be implemented with InputMap if there's no reference to StyledTextAreaBehavior? Can that be done?

[JordanMartinez]

Another issue: 1 (static) template is used to work across multiple areas (saves memory). How would that be implemented with InputMap if there's no reference to StyledTextAreaBehavior? Can that be done?

I've been looking at InputMap, but I'm guessing that's the role of InputMapTemplate.

[JordanMartinez]

Another issue: the corresponding sequence for InputMapTemplate is package-private. Additionally, I can't create the TemplateChain because it's package-private. I'm not sure if this is intentional or not. If it isn't, that won't be too hard to fix.

[TomasMikula]

Yeah, I think sequence is package private by accident.

There's no built-in equivalent to ifConsumed. Such functionality can be implemented in user-space, but maybe should be part of the library. It is what is discussed under Common post-consumption processing in the use cases. The implementation is part of tests.

[TomasMikula]

The implementation would have to be adapted for InputMapTemplate.

[JordanMartinez]

Yeah, I think sequence is package private by accident.

It's also package-private in InputMap. However, that is an interface whereas InputMapTemplate is a class. That's probably why it was missed.

[TomasMikula]

Everything in an interface is public, even if public modifier is omitted. Yeah, that's probably how it got lost.

[JordanMartinez]

Now that that's implemented, I have a few other questions:

• How should the InputMapTemplate be installed? I can compose one InputMapTemplate from KEY_PRESSED_TEMPLATE, KEY_TYPED_TEMPLATE, etc. and then install that on the view or I can install each [EVENT_TYPE]_TEMPLATE on the view. I'm not sure if there are benefits or disadvantages to each approach.
• Some of the EventPattern#EventType() method names conflict with StyledTextAreaBehavior's corresponding method name. How should STAB's names be renamed? doEventType()? (e.g. doMousePressed())

[TomasMikula]

I'm not sure if there are benefits or disadvantages to each approach.

They are very similar in effect. Composing one template will have more sharing among different instances of StyledTextAreas. Each InputMapTemplate that is installed is first instantiated to an InputMap and then the InputMap is installed. Less InputMapTemplate installs means less InputMaps instantiated for each area.

doEventType()? (e.g. doMousePressed())

handleMousePressed()?

[JordanMartinez]

They are very similar in effect. Composing one template will have more sharing among different instances of StyledTextAreas. Each InputMapTemplate that is installed is first instantiated to an InputMap and then the InputMap is installed. Less InputMapTemplate installs means less InputMaps instantiated for each area.

So, there should be only one.

So, I looked into the name conflict more: the IDE (and gradle) seem to think that mousePressed() refers to StyledTextAreaBehavior#mousePressed(MouseEvent), not EventPattern#mousePressed() (gradle will fail when I try to build it). I'm not sure why it doesn't realize that I want the other one, but I can get around that without needing to change any names by just using EventPattern#eventType, which actually looks better overall since EventPattern does not have a corresponding method for MOUSE_DRAGGED or DRAG_DETECTED

So, this:

MOUSE_EVENT_TEMPLATE = InputMapTemplate.sequence(
    consume(eventType(MouseEvent.MOUSE_PRESSED),  StyledTextAreaBehavior::mousePressed),
    consume(eventType(MouseEvent.MOUSE_DRAGGED),  StyledTextAreaBehavior::mouseDragged),
    consume(eventType(MouseEvent.DRAG_DETECTED),  StyledTextAreaBehavior::dragDetected),
    consume(eventType(MouseEvent.MOUSE_RELEASED), StyledTextAreaBehavior::mouseReleased)
);

as opposed to this:

MOUSE_EVENT_TEMPLATE = InputMapTemplate.sequence(
    consume(mousePressed(),  StyledTextAreaBehavior::mousePressed),
    consume(eventType(MouseEvent.MOUSE_DRAGGED), StyledTextAreaBehavior::mouseDragged),
    consume(eventType(MouseEvent.DRAG_DETECTED), StyledTextAreaBehavior::dragDetected),
    consume(mouseReleased(), StyledTextAreaBehavior::mouseReleased)
);

[TomasMikula]

I just released WellBehavedFX 0.3, where event.experimental replaced event.

[JordanMartinez]

Ok. I updated the build to 0.3 but gradle didn't find it. Even searching on Maven Central shows no 0.3 release. Now how can that be?

[TomasMikula]

My bad. It should appear on Maven Central in a bit.