Allow customisation of caret visibility criteria and blink rate.

[shoaniki]

I have run into issue #144 (to make the caret visible on non-editable controls), and also I wish to be able to configure the rate of blinking. (Some users must disable blinking cursors for accessibility reasons, so to hardcode the blink rate is problematic.)

So I have attempted to modify StyledTextArea to make this behavior customisable. This seems to be working well in my application, but I would rather not maintain my own branch forever :)

I don't know whether you will agree with the implementation, as this is the first time I tried to write code using ReactFX, so I would appreciate your comments if there is a better way to do this! Also I have not tried to expose these options in CSS, although perhaps that should be done?

[JordanMartinez]

...this is the first time I tried to write code using ReactFX, so I would appreciate your comments if there is a better way to do this!

I don't personally see any issues with the code you wrote. I also don't see any harm in implementing this. If Tomas approves of it, the CSS options should be included IMO.

[TomasMikula]

Thanks for the PR! The code itself looks good. The question is, will these 4 modes (ALWAYS, WHEN_EDITABLE, WHEN_ENABLED, NEVER) cover everyone's needs?

[JordanMartinez]

The question is, will these 4 modes (ALWAYS, WHEN_EDITABLE, WHEN_ENABLED, NEVER) cover everyone's needs?

Why don't we just allow customization of showCaret?

ObjectProperty<BooleanBinding> showCaret = // creation code;

EventStream<Boolean> blinkCaret = EventStreams.valuesOf(showCaretProperty())
                                              .map(BooleanBinding::get)

Then one adjust it for whatever their use case is:

Node someOtherNode = // creation code;
StyledTextArea<PS, S> area = // creation code;
area.setShowCaret(area.focusedProperty()
                 .and(area.editableProperty()
                 .and(area.disabledProperty().not())
                 .and(someOtherNode.editableProperty())
                 .and(someOtherNode.disabledProperty().not())
)

[TomasMikula]

Why don't we just allow customization of showCaret?

That's a good idea. Though the implementation you sketched would not work.

[JordanMartinez]

Though the implementation you sketched would not work.

😄 I don't know why it wouldn't off the top of my head. However, I wouldn't be surprised since the implementation was thrown together real quick

[TomasMikula]

EventStream<Boolean> blinkCaret = EventStreams.valuesOf(showCaretProperty())
                                              .map(BooleanBinding::get)

With map, you will only be calling get when showCaretProperty() changes. Which will be once in most cases ;).

[JordanMartinez]

With map, you will only be calling get when showCaretProperty() changes. Which will be once in most cases ;).

Dah! I should have known! facepalm...

[JordanMartinez]

How about this?

EventStream<Boolean> blinkCaret = EventStreams.valuesOf(showCaretProperty())
        .flatMap(showCaret -> Val.map(showCaret, Function.identity()).values());

EventStream<Duration> blinkRates = EventStreams.valuesOf(blinkRateProperty());

// whether or not to animate the caret
caretVisible = EventStreams.combine(blinkCaret, blinkRates)
        .flatMap(tuple -> {
            boolean showCaret = tuple.get1();
            Duration rate = tuple.get2();
            if (showCaret) {
                return rate.isZero()
                        ? Val.constant(true).values()
                        : booleanPulse(rate, caretDirty);
            } else {
                return Val.constant(false).values();
            }
        })
        .toBinding(false);
manageBinding(caretVisible);

One other minor improvement could be using a private static EventStream for the false and true eventstreams. Otherwise, each additional area is creating an unneeded EventStream.

Note: when I tested the above code out with the following code:

@Override
public void start(Stage primaryStage) {

    InlineCssTextArea area = new InlineCssTextArea("Some text");
    VBox box = new VBox(area, new TextArea());

    Scene scene = new Scene(box, 500, 500);
    primaryStage.setScene(scene);
    primaryStage.show();

    FxTimer.runLater(Duration.ofMillis(2000), () -> {
        System.out.println("Caret should show when area does not have focus");
        area.setShowCaret(area.disabledProperty().not());
    });

    FxTimer.runLater(Duration.ofMillis(6000), () -> {
        System.out.println("Caret should show when not have focus and is editable");
        area.setShowCaret(area.disabledProperty().not().and(area.editableProperty()));
    });

    FxTimer.runLater(Duration.ofMillis(10000),() -> {
        System.out.println("Make area no longer editable; caret should no longer show");
        area.setEditable(false);
    });
}

and clicked in the Rich text area and soon thereafter clicked in the second text area, the two carets were blinking at different rates. Not sure if that'll be annoying or not...

[TomasMikula]

Val.map(showCaret, Function.identity())

this map by identity is redundant and equivalent to just showCaret.

Anyway, ObjectProperty<BooleanBinding> is kind of cumbersome, especially when Property<Boolean> would do just as well (the user can just bind it).

Still, I would like to preserve the default behavior, so at least a three-valued type would be necessary, such as enum { ON, OFF, AUTO } or Optional<Boolean>.

[JordanMartinez]

this map by identity is redundant and equivalent to just showCaret.

Yeah. I tried Val.wrap(showCaret) but there was a type incompatibility or something like that. map was more of a bad way to get around that because I wasn't sure how else to get around it.

Anyway, ObjectProperty is kind of cumbersome, especially when Property would do just as well (the user can just bind it).

Still, I would like to preserve the default behavior, so at least a three-valued type would be necessary, such as enum { ON, OFF, AUTO } or Optional.

So would ON be the current conditions (focused, editable, not disabled), OFF would not show it at all, and AUTO would be the value that Property<Boolean> returns when the developer binds it to something else?

[TomasMikula]

I meant AUTO to be the current behavior, ON to always show, OFF to never show. No additional Property<Boolean>, just Property<CaretVisibility>. If someone wanted to bind it to an ObservableValue<Boolean>, they could do it like this

ObservableValue<Boolean> p = ???;

area.showCaretProperty().bind(Val.map(p, b -> b ? ON : OFF));

[JordanMartinez]

So, blinkrate would be the only CSS to implement in this PR, right?

[TomasMikula]

CSS would be nice, but I don't mind if that's not included in this PR.

[JordanMartinez]

@shoaniki can you update your PR to take into account the comments we've made above?

[JordanMartinez]

@TomasMikula If we did implement the CSS for the blink rate, the StyleConverter.getDurationInstance() returns back a javafx.util.Duration, not ReactFX's expected java.time.Duration. This leads to a conversion step in booleanPulse.

[TomasMikula]

The conversion needs to be done just once per CSS change, so I don't find it a problem. Alternatively, one could always implement a converter that returns java.time.Duration.

[JordanMartinez]

Alternatively, one could always implement a converter that returns java.time.Duration.

I thought about doing that, but it felt like copyright since I'd only be replacing the Duration class and using a long instead of a double