On Tue, 17 Dec 2024 20:01:14 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/InputMap.java
>> line 292:
>>
>>> 290: */
>>> 291: public void unregister(KeyBinding k) {
>>> 292: map.put(k, NULL);
>>
>> Why does this add a `NULL` rather than remove the mapping? I'm trying to
>> understand the difference between this method and `resetKeyBindings` (maybe
>> this is related to my earlier question above?). One thing to consider is
>> that allowing `NULL` values means that there is no way to tell the
>> difference between a key with a `NULL` value and a key that isn't in the map
>> without calling `map.contains`, which you don't expose.
>
> see my previous comment for the use case.
>
> this method disables the key binding, regardless of whether it's done
> programmatically by the application (as in constructor of the custom RTA), or
> the skin.
>
> if we are talking about implementation, the NULL basically blocks subsequent
> lookup in the skin input map.
I see. In that case, unregister is not a good name, since it is _not_ the
inverse of register. What it is really doing is registering a no-op binding. In
which case... why is it needed at all? The application can just as easily do
this:
register(keyBinding, () -> {});
If there really is a compelling reason to keep this method, then we need to
think of a better name. Maybe "disable" or "disableKeyBinding"?
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1889167616
