Proof of concept pull request for Behavior API (PR 1265)

[John Hendrikx]

Hi Andy,

On 23/10/2023 20:56, Andy Goryachev wrote:
>
> Dear John:
>
> Thank you for providing the alternative proposal.  One thing I liked 
> is the clarification of the purpose of behavior to translate input 
> events into platform-independent actions.
>
> I noticed you tried to validate the design using Button - a rather 
> simple Control.  I think a better choice would be to pick one of the 
> more complex controls such as TableView or TextArea.
>
Yeah, I had limited time, but may have some time next weekend to look at 
a more complicated control.
>
> Another missing piece is probably a set of behavior tests similar to 
> https://bugs.openjdk.org/browse/JDK-8314906 to verify that the 
> behavior does not change from the user perspective.
>
For now it's a (bare bones) proof of concept, I validated manually that 
the Buttons still work as before (including things like holding keys 
down, then messing with it with the mouse) -- there's no reason to 
assume it wouldn't work, all that was added was a little indirection.
>
> One important problem I tried to solve is managing the key bindings.  
> I think it’s an important functionality that needs to be provided, 
> evident from the plethora of use cases described earlier in the 
> discussion. I cannot see from the PR how you propose to address the 
> issue, and the examples you mentioned earlier look to me more 
> complicated than necessary.
>
The idea would be that you can customize the keybindings by creating a 
new Behavior.

You'd create a new class, `MyBehavior`, in `install` you'd call 
`ButtonBehavior.getInstance().install(context)` to install the default 
keybindings and handlers from whatever base behavior you'd like.  Then 
you use the `context` to change the bindings you don't like.

The context in this simple proof of concept does not have methods yet to 
make this easy, but the methods that you provided in `BehaviorBase` 
could live in the `BehaviorContext` interface in the sample I provided; 
in other words, a method like `registerKey` could live there; in effect, 
a method like `registerKey` is the same as installing a KEY_PRESSED 
handler, but which already does the `== getCode()` check for you (and 
allows grouping multiple keys in one handler for efficiency).  If 
`BehaviorContext` becomes too big with all these methods, it could offer 
a getter to get something where all those methods could live (`InputMap` 
seems like a decent name for that).

> I have hard time understanding where the idea of static or singleton 
> behavior comes from exactly, and what benefits it provides.  The whole 
> design overlooks the fact that behaviors are tightly coupled to skins 
> - to the point where it gets progressively difficult to provide a 
> reasonable skin-to-behavior interface when skins have a different 
> design (in appearance and the kinds and number of nodes involved).
>
I'm aware, I'm saying that the whole design will benefit immensely from 
getting rid of this coupling.  You can already see how trivial (and how 
safe) it is to install a Behavior in the proof of concept; no risk of 
anything being left behind, full isolation of behavior installed event 
handlers/listeners is possible, and with the Behavior, BehaviorContext 
and handlers each handling a particular aspect, it offers a much better 
separation of concerns.

Perhaps it is impossible to uncouple Skins, but I don't think that is 
the case, simply because if you build a Skin from scratch with this in 
mind, you'd be able to do it without the tight coupling; tightly 
coupling it to skins was just the first iteration that got the job done 
-- without a clear mental model of what is supposed to be Skin and what 
is Behavior, things got mixed up and tightly coupled; no doubt that has 
resulted in a lot of hesitation to make Behaviors public as they're 
currently more of a Skin implementation detail than a proper separated 
concern.

With a (possible) definition that Behaviors offer translation to higher 
level events, Skins can make use of those, but need not use all of 
them.  This means that a Behavior can also offer translations that may 
not be useful by current JavaFX skins, but may be useful for custom 
skins or future adjustments to FX skins. For example, a custom 
ButtonSkin may not care for "ARM" and "DISARM", but might want to 
trigger a flashing animation when "FIRE" is triggered.

> Adding more events and have them bubble up the hierarchy is, in my 
> opinion, a departure from the current design, possibly a source of 
> regression, impacts performance (however slightly), and is completely 
> unnecessary.  The application developer can easily achieve that with 
> existing mechanisms should such a function is indeed needed.
>
Perhaps they're not needed, as the functions, if public, are easily 
called directly as well; they're offered as an alternative to 
FunctionTags as it is an existing mechanism, and events are quite suited 
for interpretation and translation to a different meaning (like 
FunctionTags are offering), or to higher level events with more 
meaning.  If we can drop FunctionTags, then there'd be no need (yet) for 
these events.

If we go the event route, the possibilities are left open ended, as the 
user can decide where and how to deal with them (which need not be at 
the Control level at all).  It's even possible such higher level events 
are picked up by a Behavior again (3 "disarms" in a row triggers 
something new).

It IMHO certainly is worth consideration before dismissing them.

Anyway, I certainly don't agree that they're a regression risk, nor do I 
think the performance should matter here -- events are already being 
sent out, a few more is not going to be a problem; events are also in 
reaction to user actions which are measured in milliseconds.

I also don't see how they're a departure from the current design (and by 
design I mean current public API's -- not private ones, nor 
non-finalized ones).

>
> Of course, I can be missing some important points altogether, and/or 
> be completely wrong about /what people want (tm)/.
>
No, I think key remapping is probably a fair assesment of what people 
want, although we do disagree a bit at what level it should be offered.  
I can imagine a couple of systems:

- Per control mappings

The customization is per control, and all changed mappings need to be 
(re)applied after constructing the control.

- Global changes of keymappings (ie. something that affects all Buttons 
with a single call)

The customization happens on a BehaviorFactory level, which if 
manipulated before any controls are created, would result in all the 
controls using the changed mappings.

- CSS based (a selector decides which controls gets different mappings)

This could take the form of being able to install a Behavior like you 
could with Skins (ie. -fx-behavior, -fx-skin), or a completely different 
form where a CSS property is able to change mappings:

        -fx-key-mappings: addKey(SPACE, fire) removeKey(ENTER)

Indirections could be done the way color indirections are done (some 
prefixes may be needed):

       .root { fire: BUTTON_FIRE }

- Behavior based (replace a Behavior, but only change the mappings)

Allow a Behavior to be set, and to be easily overriden and its mappings 
altered.  This would be per control, but differs slightly in that you 
only need to change one thing (the Behavior) instead of multiple things 
(each mapping).  It may also open up a path towards doing CSS based 
installing of behaviors (using -fx-behavior).

> Now that Kevin is back from vacation, we could have an internal 
> discussion of the two proposals to determine how we can move forward.
>
> Thanks again for a lively discussion and the PR.
>
Maybe a bit too lively; I may have come off as very direct (even more so 
when it is via a text medium) and it took me a while to also see all the 
issues (and I'm sure I'm still missing alot given limited resources to 
look into this, and then to provide good feedback/criticism); that was 
not my intention, I'm just hoping for the best API we can manage, if 
possible one that looks as if it was part of JavaFX from the start, even 
if it takes a bit more effort.


> Cheers,
>
> -andy
>
--John


> *From: *openjfx-dev <openjfx-dev-retn at openjdk.org> on behalf of John 
> Hendrikx <john.hendrikx at gmail.com>
> *Date: *Friday, October 20, 2023 at 14:40
> *To: *openjfx-dev at openjdk.org <openjfx-dev at openjdk.org>
> *Subject: *Proof of concept pull request for Behavior API (PR 1265)
>
> Hi list,
>
> I've made a proof of concept for possibly exposing Behaviors as a first
> class API in JavaFX.  It's by no means complete, and there will be some
> significant hurdles still no doubt, but this proof of concept does
> replace the internal ButtonBehavior completely with a newly implemented
> public ButtonBehavior (and it seems to be all working still).
>
> The proof of concept shows the basics behind this proposal, and
> introduces a Behavior interface, a BehaviorContext interface, a public
> ButtonBehavior and a class with ButtonEvents.
>
> There is also an Alternate ButtonBehavior which (somewhat clumsily)
> changes the SPACE key for selecting a button to the "A" key.  I suspect
> it shouldn't be too hard to make this a bit more streamlined.
>
> One thing of note that I haven't quite figured out yet; even though the
> new ButtonBehavior does not install the Navigation keys, Navigation
> still works; this is not because the old behavior is still active behind
> the scenes, but just seems to work out of the box.
>
> I'm hoping this will make the idea behind this proposal a bit easier to
> grasp.  Note that I'm also mainly trying to make it possible to be able
> to remap keys, but I want to make sure that this is done in such a way
> that it doesn't block a future open sourcing of a Behavior API.  With
> this proposal it's possible however that Behaviors must become public
> first though before being able to access the necessary classes to change
> input mappings.
>
> The PR can be found here: https://github.com/openjdk/jfx/pull/1265
>
> Feel free to comment here or on the PR.  The PR is in draft, so comments
> there will not show up on the mailing list.
>
> Note: the test failure is because tests are using reflection to access a
> behavior field that is part of skins; this field is no longer present
> for ButtonSkin.
>
> --John
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/openjfx-dev/attachments/20231023/7455db4e/attachment-0001.htm>