8389551: Update class docs of MouseEvent - #2238
Conversation
|
👋 Welcome back nlisker! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
Notes to reviewers:
import javafx.application.Application;
import javafx.scene.Node;
import javafx.scene.Scene;
import javafx.scene.input.ClipboardContent;
import javafx.scene.input.Dragboard;
import javafx.scene.input.InputEvent;
import javafx.scene.input.MouseEvent;
import javafx.scene.input.TransferMode;
import javafx.scene.layout.HBox;
import javafx.scene.layout.VBox;
import javafx.scene.paint.Color;
import javafx.scene.shape.Rectangle;
import javafx.stage.Stage;
public class MouseTest extends Application {
public static void main(String[] args) {
launch(args);
}
@Override
public void start(@SuppressWarnings("exports") Stage stage) throws Exception {
var scene = createScene(createSourcePDR(), createSourceMouseFullPDR(), createSourceDnD(), createTarget());
stage.setScene(scene);
stage.show();
}
private Rectangle createSourcePDR() {
var source = new Rectangle(50, 50);
source.setId("PDR");
source.setFill(Color.YELLOW);
source.setOnDragDetected(e -> {
print(source, e);
e.consume();
});
addCommonListeners(source);
return source;
}
private Rectangle createSourceMouseFullPDR() {
var source = new Rectangle(50, 50);
source.setId("FullPDR");
source.setFill(Color.RED);
source.setOnDragDetected(e -> {
source.startFullDrag();
print(source, e);
e.consume();
});
addCommonListeners(source);
return source;
}
private Rectangle createSourceDnD() {
var source = new Rectangle(50, 50);
source.setId("DnD");
source.setFill(Color.GREEN);
source.setOnDragDetected(e -> {
Dragboard db = source.startDragAndDrop(TransferMode.ANY);
var content = new ClipboardContent();
content.putString("");
db.setContent(content);
print(source, e);
e.consume();
});
addCommonListeners(source);
return source;
}
private Rectangle createTarget() {
var target = new Rectangle(100, 50);
target.setId("target");
target.setFill(Color.BLUE);
target.setOnDragDetected(MouseEvent::consume);
addCommonListeners(target);
return target;
}
private Scene createScene(Rectangle sourcePDR, Rectangle sourceFullPDR, Rectangle sourceDrag, Rectangle target) {
var scene = new Scene(new VBox(10, new HBox(sourcePDR, sourceFullPDR, sourceDrag), target), 300, 200);
scene.setOnMouseEntered(e -> System.out.println(e.getEventType() + " scene"));
scene.setOnMouseExited(e -> System.out.println(e.getEventType() + " scene"));
scene.setOnDragDetected(e -> {
scene.startFullDrag();
System.out.println(e.getEventType() + " scene");
});
scene.setOnDragDone(e -> {
e.acceptTransferModes(TransferMode.ANY);
System.out.println(e.getEventType() + " scene");
});
scene.setOnMouseDragReleased(e -> System.out.println(e.getEventType() + " scene"));
scene.setOnMouseDragDone(e -> System.out.println(e.getEventType() + " scene"));
return scene;
}
private static void addCommonListeners(Node node) {
// MouseEvent
node.setOnMouseMoved(e -> print(node, e));
node.setOnMouseDragged(e -> print(node, e));
node.setOnMouseEntered(e -> print(node, e));
node.setOnMouseExited(e -> print(node, e));
node.setOnMousePressed(e -> print(node, e));
node.setOnMouseReleased(e -> print(node, e));
node.setOnMouseClicked(e -> print(node, e));
node.addEventHandler(MouseEvent.MOUSE_ENTERED_TARGET, e -> {
System.out.print("TARGET_ ");
print(node, e);
});
// MouseDragEvent
node.setOnMouseDragOver(e -> print(node, e));
node.setOnMouseDragEntered(e -> print(node, e));
node.setOnMouseDragExited(e -> print(node, e));
node.setOnMouseDragReleased(e -> print(node, e));
node.setOnMouseDragDone(e -> System.out.println(e.getEventType() + " " + node.getId()));
// DragEvent
node.setOnDragOver(e -> {
e.acceptTransferModes(TransferMode.ANY);
print(node, e);
});
node.setOnDragEntered(e -> {
e.acceptTransferModes(TransferMode.ANY);
print(node, e);
});
node.setOnDragExited(e -> {
print(node, e);
});
node.setOnDragDropped(e -> {
e.acceptTransferModes(TransferMode.ANY);
e.setDropCompleted(true);
print(node, e);
});
node.setOnDragDone(e -> {
e.acceptTransferModes(TransferMode.ANY);
print(node, e);
});
}
private static void print(Node node, InputEvent e) {
System.out.println(e.getEventType() + " " + node.getId());
}
} |
|
/reviewers 2 @andy-goryachev-oracle @kevinrushforth please review. This is a docs-only change that can go into 27. |
Webrevs
|
andy-goryachev-oracle
left a comment
There was a problem hiding this comment.
The new diagram is a nice addition (I am not sure if "DRAG_DETECTED" node should be a rhombus since it's a decision node (according to https://en.wikipedia.org/wiki/Flowchart )
The textual changes I like much less, mainly because the old one is more useful, in my opinion, since it describes what actually happens.
| /// [mouseTransparent][javafx.scene.Node#mouseTransparentProperty()] set to `true` do not receive mouse events. | ||
| /// | ||
| /// ## Button events | ||
| /// A mouse button can be [pressed][#MOUSE_PRESSED] and [released][#MOUSE_RELEASED]. A button [click][#MOUSE_CLICKED] |
There was a problem hiding this comment.
it sounds like press-drag-release gesture might produce a MOUSE_CLICKED event. can you clarify?
also, we probably should mention that the MOUSE_CLICKED arrives after the MOUSE_RELEASED, either here or in the MOUSE_CLICKED javadoc.
There was a problem hiding this comment.
it sounds like press-drag-release gesture might produce a
MOUSE_CLICKEDevent. can you clarify?
Both simple PDR and full-PDR can produce MOUSE_CLICKED. Do you want me to explicitly state it? In the section about drag events, it's shown that both end with a release event, so it is implied that it produces a click, but I can add that info.
There was a problem hiding this comment.
-
click: "A button click occurs after these happen" -> "may follow MOUSE_RELEASED" to be specific. I recall at some point I thought it was press->click->release.
-
there is no
MOUSE_CLICKEDin your diagram, plus I can't figure out how a click can be generated during the drag operation.
There was a problem hiding this comment.
I can't figure out how a click can be generated during the drag operation.
Using the test program above, just start a PDR or a full-PDR and release the mouse over the same node. For PDR I get:
MOUSE_PRESSED PDR
MOUSE_DRAGGED PDR
MOUSE_DRAGGED PDR
MOUSE_DRAGGED PDR
MOUSE_DRAGGED PDR
MOUSE_DRAGGED PDR
MOUSE_DRAGGED PDR
MOUSE_DRAGGED PDR
DRAG_DETECTED PDR
MOUSE_DRAGGED PDR
MOUSE_DRAGGED PDR
MOUSE_DRAGGED PDR
MOUSE_RELEASED PDR
MOUSE_CLICKED PDR
For full-PDR I get:
MOUSE_PRESSED FullPDR
MOUSE_DRAGGED FullPDR
MOUSE_DRAGGED FullPDR
MOUSE_DRAGGED FullPDR
MOUSE_DRAGGED FullPDR
MOUSE_DRAGGED FullPDR
DRAG_DETECTED FullPDR
MOUSE_DRAGGED FullPDR
MOUSE-DRAG_ENTERED FullPDR
MOUSE-DRAG_OVER FullPDR
MOUSE_DRAGGED FullPDR
MOUSE-DRAG_OVER FullPDR
MOUSE_DRAGGED FullPDR
MOUSE-DRAG_OVER FullPDR
MOUSE-DRAG_RELEASED FullPDR
MOUSE-DRAG_RELEASED scene
MOUSE-DRAG_DONE FullPDR
MOUSE-DRAG_DONE scene
MOUSE_RELEASED FullPDR
MOUSE_CLICKED FullPDR
MOUSE-DRAG_EXITED FullPDR
There was a problem hiding this comment.
1. "may follow MOUSE_RELEASED"
When does it not?
2. there is no
MOUSE_CLICKEDin your diagram
I can add it, but the diagram is for drag-related events. After the mouse release event, the important part is if there's an event for ending the drag. A click event is not part of it.
| /// | ||
| /// There are 3 types of drag gestures that can be chosen from within the `DRAG_DETECTED` handler: | ||
| /// | ||
| /// 1. Simple press-drag-release (PDR), in which the source (picked) node is the only node involved. It receives all the |
There was a problem hiding this comment.
"node involved" is much more nebulous than "The whole press-drag-release gesture is delivered to one node ... If a mouse clicked event is generated from these events, it is still delivered to the same node." which actually describes what is happening.
I really prefer the old description of the three types, as it is very clear, unlike the new one.
Can we revert?
There was a problem hiding this comment.
I fixed the "involved" wording be removing it - the next sentence is specific ("receives all the events in the PDR gesture").
The sentence "If a mouse clicked event is generated from these events, it is still delivered to the same node." is in the next sentence "...including button events even when they occur over other nodes."
The old description is even lacking as it doesn't say that MOUSE_RELEASED events are also delivered to the node. Interestingly, (They are delivered)MOUSE_PRESSED events are not generated, I wonder if this is a bug or intended.
There was a problem hiding this comment.
I think the main issue I have with the new version is that it removed some information that I feel is important.
for example:
- The whole press-drag-release gesture is delivered to one node (removed)
- and all subsequent mouse events are delivered to that same node until the button is released.
- If these nodes need to be involved in the gesture, full press-drag-release gesture has to be activated. (instructs what the app dev is supposed to do)
Also, in the new version:
- DRAG_DETECTED handler: what handler? are you referring to some internal entity?
I think the old Dnd section is much more informative and easier to understand.
Please keep in mind that this is just my opinion, let's see what other people think.
There was a problem hiding this comment.
I think the main issue I have with the new version is that it removed some information that I feel is important.
for example:
- The whole press-drag-release gesture is delivered to one node (removed)
- and all subsequent mouse events are delivered to that same node until the button is released.
I can add these, but how is it different than the current "Simple press-drag-release (PDR), in which the source (picked) node receives all the events in the PDR gesture, including click events even when they occur over other nodes."?
Perhaps I should change "all the events in the PDR gesture" to "all the events during the PDR gesture".
"until the button is released" is true for all drag gestures and is specified in the paragraph above: "Dragging ends when a mouse button is released."
- If these nodes need to be involved in the gesture, full press-drag-release gesture has to be activated. (instructs what the app dev is supposed to do)
Doesn't "This gesture starts when the startFullDrag method of a node (or a scene) is invoked." tell what the developer what to do even in more detail (how to activate it)?
I can phrase it in a more conditional way, something like: "To start this gesture, the startFullDrag method of a node (or a scene) must be/has to be invoked."
Also, in the new version:
- DRAG_DETECTED handler: what handler? are you referring to some internal entity?
The only mention of this in the current docs is "When you start dragging, eventually the DRAG_DETECTED event arrives. In its handler, you can either start a...", which didn't explain what handler, so why is this a new problem?
The new version says: "...at which point a [#DRAG_DETECTED] event is dispatched to the source node", which is both more precise and gives a link to the event.
I can give a link to Node#onDragDetectedProperty(), but all the docs assume that an X event is handled by an onX handler, and this one is no exception.
I can also write "DRAG_DETECTED event handler" if it adds anything.
I think the old Dnd section is much more informative and easier to understand.
Please keep in mind that this is just my opinion, let's see what other people think.
Yes, but you suggested a full revert of the textual section rather than corrections, so I take/took it that something in the explanation is done very badly. One area I need to clarify, which is was hinted to in the old docs but is phrased incorrectly in the new docs, is that the simple PDR starts before the DRAG_DETECTED event is dispatched and continues alongside a full-PDR (but not a DnD) gesture.
There was a problem hiding this comment.
I just find the new version much more difficult to read and understand. Maybe it's just me.
There was a problem hiding this comment.
I see now that the sentence "When a mouse button is pressed, the top-most node is picked and all subsequent mouse events are delivered to that same node until the button is released." is incorrect. Nodes that are under the top-most node also receive these events (if they are not consumed). I'll clarify this too.
There was a problem hiding this comment.
"node involved" is much more nebulous than "The whole press-drag-release gesture is delivered to one node ... If a mouse clicked event is generated from these events, it is still delivered to the same node." which actually describes what is happening.
The "involved" terminology was used in the original docs:
During a simple press-drag-release gesture, the other nodes are not involved and don't get any events. If these nodes need to be involved in the gesture, full press-drag-release gesture has to be activated.
I'm removed this usage.
I didn't follow any convention. If it matters, I can adjust the shapes. I assumed it would be clear after reading that section.
I found the old ones to be confusing at best. Some specifics:
Which lines that describe "what actually happens" are missing? It's probably easier to just add them than just revert. |
| /// `MOUSE_ENTERED`. | ||
| /// 3. During the bubbling phase, its type is switched back to `MOUSE_ENTERED_TARGET`. | ||
| /// | ||
| /// If the event is filtered or consumed, it affects both event types. |
There was a problem hiding this comment.
how? what happens?
this gives very little information.
There was a problem hiding this comment.
this gives very little information.
This was the original: "It's still one event just switching types, so if it's filtered or consumed, it affects both event variants."
It gives the same information, only less accurately ("variants" is not an established term).
how? what happens?
For "how?", I added a link to consume(). As for "what happens?", it depends on what change was made.
| /// This means that a node can receive these events when the mouse enters a node in its scenegraph hierarchy. To | ||
| /// distinguish between these two cases, the event target can be tested on equality with the node. | ||
| /// | ||
| /// Since `MOUSE_ENTERED`/`MOUSE_EXITED` are subtypes of `MOUSE_ENTERED_TARGET`/`MOUSE_EXITED_TARGET`, they are also |
There was a problem hiding this comment.
the "subtype" thing is present in the original doc and is quite confusing: aren't those distinct types?
maybe instead we should just rephrase the whole thing saying the *TARGET events are delivered to parents while the ENTERED/EXITED ones to the target node only?
the original phrase "It's still one event just switching types" actually explains the whole thing better.
There was a problem hiding this comment.
the "subtype" thing is present in the original doc and is quite confusing: aren't those distinct types?
All the events are distinct from a proper Java language perspective (no extends), but they hold information about their "event supertype":
EventType<MouseEvent> MOUSE_ENTERED_TARGET = new EventType<>(MouseEvent.ANY, "MOUSE_ENTERED_TARGET");
EventType<MouseEvent> MOUSE_ENTERED = new EventType<>(MouseEvent.MOUSE_ENTERED_TARGET, "MOUSE_ENTERED");The first argument is superType, so MOUSE_ENTERED is a subtype of MOUSE_ENTERED_TARGET in the event system. Don't get me started on what I think about this design :)
There was a problem hiding this comment.
now I see it, thanks!
I wonder if we could link to the EventType(EventType) constructor when we talk about subtypes, something like this:
are {@link EventType#EventType(EventType) subtypes} of
|
Updated both sections with clarifications and corrections. I noticed that the |
andy-goryachev-oracle
left a comment
There was a problem hiding this comment.
Nir, I am sorry. I think we need a fresh pair of eyes to give constructive criticism. I am afraid I prefer the old version - perhaps with some clarifications - but the new version does not bring happiness. It could be just me, so perhaps someone else could chime in.
| @@ -25,106 +25,137 @@ | |||
|
|
|||
| package javafx.scene.input; | |||
There was a problem hiding this comment.
this comment applies to mouse_events.svg (I don't know how to annotate the image in github). sorry to say that, but it has many issues.
- MOUSE_DRAGGED: remove side line (it's not a fork)
- "exit hist. area?" - decision point, so it should have two outcomes (yes and no), 2 lines
- DRAG_DETECTED: again, only one line down
- rhombus with some kind of check, and 3 outgoing lines: DragAndDrop path, FullDrag path, and one that leads to MOUSE_DRAGGED. I'd think the rhombus should name the criteria and all three outcomes need proper labels.
|
I can take a look. I also request @arapte to take a look. Given that there is likely to be a little further discussion, I recommend targeting the JBS issue to |
Adds a section for button events and clarifies the existing sections.
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/2238/head:pull/2238$ git checkout pull/2238Update a local copy of the PR:
$ git checkout pull/2238$ git pull https://git.openjdk.org/jfx.git pull/2238/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 2238View PR using the GUI difftool:
$ git pr show -t 2238Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/2238.diff
Using Webrev
Link to Webrev Comment