On Sat, 15 Apr 2023 18:01:28 GMT, Michael Strauß <mstra...@openjdk.org> wrote:
>> This PR adds the following methods to the `EventTarget` interface:
>> 1. `addEventHandler`
>> 2. `removeEventHandler`
>> 3. `addEventFilter`
>> 4. `removeEventFilter`
>
> Michael Strauß has updated the pull request incrementally with one additional
> commit since the last revision:
>
> revert a change in Menu
Looks good overall.
modules/javafx.base/src/main/java/javafx/event/EventTarget.java line 79:
> 77: * <p>
> 78: * It is possible to register a single {@link EventHandler} instance
> for different event types,
> 79: * so the caller needs to specify the event type from which the
> handler should be unregistered.
I would rephrase to "Since it is possible to..., the caller needs to..."
Same for filter.
modules/javafx.base/src/main/java/javafx/event/EventTarget.java line 95:
> 93: * Registers an event filter for this target.
> 94: * <p>
> 95: * The filter is called when the node receives an {@link Event} of
> the specified
node -> target
modules/javafx.controls/src/main/java/javafx/scene/control/TableColumnBase.java
line 737:
> 735: * {@inheritDoc}
> 736: * <p>
> 737: * The {@code TableColumnBase} class allows registration of
> listeners which will be notified when editing occurs.
The edit events are defined only in the subclasses. Either, if possible, move
the events to this class, or clarify that these events are defined in
subclasses. `TreeItem` talks more about this - using the cell.
modules/javafx.controls/src/main/java/javafx/scene/control/TableColumnBase.java
line 738:
> 736: * <p>
> 737: * The {@code TableColumnBase} class allows registration of
> listeners which will be notified when editing occurs.
> 738: * Note that {@code TableColumnBase} is <b>not</b> a {@link Node},
> and therefore no visual events will be fired on it.
Isn't this true for the other classes here as well, like `Tab`? Might it be
worth saying this in the other classes too then, or even on `EventTarget`
itself (something like "Note that implementing classes that are not a `Node`
don't receive visual events")?
I would also give an example or 2 of what "visual events" are since this term
is not defined anywhere that I know of.
-------------
PR Review: https://git.openjdk.org/jfx/pull/1090#pullrequestreview-1387150109
PR Review Comment: https://git.openjdk.org/jfx/pull/1090#discussion_r1168092516
PR Review Comment: https://git.openjdk.org/jfx/pull/1090#discussion_r1168092983
PR Review Comment: https://git.openjdk.org/jfx/pull/1090#discussion_r1168115120
PR Review Comment: https://git.openjdk.org/jfx/pull/1090#discussion_r1168115726
