On Thu, 12 Dec 2024 16:40:27 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
>> Andy Goryachev has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 83 commits: >> >> - Merge remote-tracking branch 'origin/master' into 8301121.RichTextArea >> - legal >> - unicode license >> - add handler >> - review comments >> - Merge remote-tracking branch 'origin/master' into 8301121.RichTextArea >> - clamp >> - Merge remote-tracking branch 'origin/master' into 8301121.RichTextArea >> - review comments >> - review comments >> - ... and 73 more: https://git.openjdk.org/jfx/compare/b76c05b9...e5814b21 > > modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/InputMap.java > line 363: > >> 361: */ >> 362: // TODO this should not affect the skin input map, but perhaps >> place NULL for each found KeyBinding >> 363: public void unbind(FunctionTag tag) { > > This needs a different name than the other "unbind" method since the > semantics are quite different. Since this removes all key bindings for a > given function tag, I recommend renaming it to `removeKeyBindingsFor`, which > goes nicely with `getKeyBindingsFor`. > > And shouldn't there be an `unregisterFunction(FunctionTag)` method that is > the inverse of `registerFunction(FunctionTag, Runnable)`? Or is that what > `restoreDefaultFunction(FunctionTag)` does already? you are right - no need for a separate `unregister(FunctionTag)`, the `restoreDefaultFunction(FunctionTag)` does the job. > modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/RichTextArea.java > line 168: > >> 166: * {@link >> InputMap#register(jfx.incubator.scene.control.input.KeyBinding, >> FunctionHandler)}. >> 167: */ >> 168: public static class Tags { > > Should this be `Tag` instead of `Tags`? Each instance is a single > `FunctionTag`. Isn't it a collection of tags? But you are right, it looks better in the behavior code. ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1882864930 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1882866282
