Home » Eclipse Projects » GEF » [GEF4] AbstractFXTool & Policies
[GEF4] AbstractFXTool & Policies [message #1721180] |
Mon, 25 January 2016 14:13 |
Camille Letavernier Messages: 952 Registered: February 2011 |
Senior Member |
|
|
Hi,
Since Bug 484690 [1], the AbstractFXTool iterates on the entire hierarchy of the selected element when receiving an input (e.g. Mouse Drag/Drop)
I have some difficulties to understand how to filter the elements which should actually react to an input in this case. Let's say I have the following (simplified) hierarchy of ContentParts:
- Root
-- Package
--- Class
When I do a Drag/Drop on the Class, the Drag policy of Class, Package and Root will be invoked. Moreover, the only information these policies receive is the JavaFX mouse event. The problem I have is that, if I have a "Move on Drag" policy installed on both the Package and Class, both will move when dragging the Class. And since the Policy only receives the low-level JavaFX MouseEvent, it's difficult to filter which events actually affect which part (Before, it was the responsibility of GEF4 to do this filtering, and only the policies of the leaf part were invoked)
Note: looking a little bit deeper, I found that some policies check that their host is currently in the selection model to implement this filter. Then I looked at the FocusAndSelectOnClickPolicy to see how the filter was implemented there, and... It's not implemented. This works because all elements are selected in sequence (From the root to the leaf), each selection replacing the previous one. This means that selection listeners will be notified many times (1 for each level of depth of the selected element). In my example, I will receive 3 selection event in a row: Root, Package, Class (And Ctrl + Click for multi-selection seems chaotic)
For a diagram embedded in an Eclipse Editor, this is very visible, as e.g. the Properties View will be updated several times for each click.
Did I misunderstand the design (That's quite possible, as my code was based on the previous design, then updated to match the various recent refactorings, so I might have overlooked something), or is there a deeper issue with this new approach?
[1] 484690: Introduce AbstractFXTool that provides utility methods for prioritized target selection
https://bugs.eclipse.org/bugs/show_bug.cgi?id=484690
Camille Letavernier
[Updated on: Mon, 25 January 2016 14:14] Report message to a moderator
|
|
|
Re: [GEF4] AbstractFXTool & Policies [message #1721184 is a reply to message #1721180] |
Mon, 25 January 2016 14:44 |
|
Thank you for your feedback!
Actually, such a filter is implemented within FXFocusAndSelectOnClickPolicy (and some other interaction policies) so that only clicking on the background will clear the selection. The filter works by testing if a visual part other than the host is controlling the event target:
} else if (host instanceof IRootPart) {
// check if click on background (either one of the root visuals, or
// an unregistered visual)
Map<Node, IVisualPart<Node, ? extends Node>> visualPartMap = getHost()
.getRoot().getViewer().getVisualPartMap();
IVisualPart<Node, ? extends Node> targetPart = visualPartMap
.get(e.getTarget());
if (targetPart == null || targetPart == host) {
// unset focus
focusModel.setFocus(null);
// remove all selected
selectionModel.clearSelection();
}
}
The code can be adjusted in the same way to disregard selecting a content part if a more specific target part will also process the event. We should probably provide a utility method that implements this filter mechanism. I created Bugzilla #486477 [1] to keep track of the issue.
[1] https://bugs.eclipse.org/bugs/show_bug.cgi?id=486477
Best regards,
Matthias
|
|
| |
Re: [GEF4] AbstractFXTool & Policies [message #1721274 is a reply to message #1721190] |
Tue, 26 January 2016 09:16 |
Camille Letavernier Messages: 952 Registered: February 2011 |
Senior Member |
|
|
Hi,
I've looked a little bit further into this, and still thinks there's an issue with the current implementation. So I've replaced my own FocusAndSelectPolicy with the default FX one (org.eclipse.gef4.mvc.fx.policies.FXFocusAndSelectOnClickPolicy), and I've applied it on all content parts (Including Root, PackageContentPart and ClassContentPart, to reuse my previous example)
If I have a Package containing a Class, and I click on a Class, the following happen:
- The root policy receives the event first (Because AbstractFXTool iterates on the hierarchy of the clicked element)
- The root policy does nothing because it finds a part at the click location
- The package policy receives the event
- It selects its host (PackageContentPart)
- The Class policy receives the event
- It selects its host (ClassContentPart)
So I've received two selection change events (Still, the end result is as expected, i.e. the Class is selected)
Now if I do the same while holding Control, multi-selection happens:
- [...]
- The package policy receives the event
- The PackageContentPart is appended to the selection
- The class policy receives the event
- The ClassContentPart is appended to the selection
So if I start from an empty selection an Control-click on a Class, I now get 2 selected element (The clicked one, and all its parents, i.e. the package in this example). In this case, not only I receive 2 selection change events, but I end up with an invalid selection (Or at least, not what I expect)
This behavior seems quite confusing to me, and while I see how to work with that (By adding the right filters in each policy), I feel it would be much simpler if only the appropriate policy was invoked (And it might be the responsibility of the policy to delegate to the policy of its host's parent if needed). Or maybe there could be an extra parameter on policies so that policies that are not directly applied on the target part are skipped (I don't know all the GEF4 applications, but in my case that's definitely the expected default behavior)
Once again, I might have missed something more obvious, or the issue might be caused by my implementation, so any feedback is welcome!
Thanks,
Camille
Camille Letavernier
|
|
|
Re: [GEF4] AbstractFXTool & Policies [message #1721307 is a reply to message #1721274] |
Tue, 26 January 2016 13:02 |
|
Hi Camille,
you are correct in that another filter would be necessary to disregard processing events on parent parts if an explicit target part other than the host can be identified. Your use case demonstrates that this should be the default behavior for the FXFocusAndSelectOnClickPolicy, therefore, I created Bugzilla #486550 [1] to keep track of the issue. Note, that the filtering does not have to be sophisticated. It is sufficient to check if the EventTarget is registered for an IVisualPart other than the host. I already resolved Bugzilla #486477 [2], i.e. created convenience methods for the filter mechanism. Therefore, the filter can be implemented like this (example for an IFXOnHoverPolicy):
public void hover(MouseEvent e) {
// do nothing in case there is an explicit event target
if (isRegistered(e.getTarget()) && !isRegisteredForHost(e.getTarget())) {
return;
}
// ...
}
Your feedback brought up another issue: The only responsibility of the AbstractFXTool is to determine the target policies for a given JavaFX event. If you want to change that behavior, all tool implementations have to be replaced. In order to make it customizable, the AbstractFXTool should be removed and the target determination should be provided as a strategy to the tools. Then, it would be easy to exchange the behavior, all you would have to do is bind another TargetDeterminationStrategy. I created Bugzilla #486555 [3] to keep track of the issue. I will leave you a message here when the issues are resolved.
[1] https://bugs.eclipse.org/bugs/show_bug.cgi?id=486477
[2] https://bugs.eclipse.org/bugs/show_bug.cgi?id=486550
[3] https://bugs.eclipse.org/bugs/show_bug.cgi?id=486555
Best regards,
Matthias
|
|
| |
Re: [GEF4] AbstractFXTool & Policies [message #1721311 is a reply to message #1721309] |
Tue, 26 January 2016 14:10 |
Camille Letavernier Messages: 952 Registered: February 2011 |
Senior Member |
|
|
Thanks Matthias,
It's almost sufficient. I have one extra question related to this topic: when we create a JavaFX Label (in doCreateVisual()), GEF4 properly registers the visual to the VisualPartMap. However, when using setText/setImage later (in the doRefreshVisual() method), the Label receives 2 children (ImageView and/or LabeledText)
Whose responsibility is it to update the VisualPartMap in this case? (Currently it isn't updated at all, so the selection policy will not work, because the LabelText of the Label is not registered). Should GEF4 do it after each AbstractVisualPart#refreshVisual(), or is the application responsible for refreshing the map?
This is not an issue in most cases, as all the Nodes created in doCreateVisual() are properly registered by GEF4, and we don't create so many new nodes in the refresh operation. But in cases such as labels, JavaFX sometimes creates sub-nodes, and it can be difficult to spot
Regards,
Camille
Camille Letavernier
|
|
|
Re: [GEF4] AbstractFXTool & Policies [message #1721339 is a reply to message #1721311] |
Tue, 26 January 2016 16:54 |
|
Hi Camille,
you are definitely asking the right questions
We discussed this exact issue in the office. We concluded that the IVisualPart is responsible for updating the visual-part-map to prevent a generic updateRegistration() method from walking over the whole visual-part-map as well as the visual hierarchy of the part and its sub-parts after each refresh. However, I can imagine using a support class that can be used for the (un-)registration and keeps track of the registered visuals so that the unregistration does not need to walk over the whole visual-part-map. I will think about this and maybe open a Bugzilla after discussing it further with Alexander. For the time being, you can update the visual-part-map as follows, however, it might have a performance impact:
private void unregisterVisuals() {
Map<Node, IVisualPart<Node, ? extends Node>> visualPartMap = getRoot().getViewer().getVisualPartMap();
Set<Node> keySet = new HashSet<>(visualPartMap.keySet());
for (Node visual : keySet) {
if (visualPartMap.get(visual) == this) {
visualPartMap.remove(visual);
}
}
}
private void registerVisuals(Node visual) {
Map<Node, IVisualPart<Node, ? extends Node>> visualPartMap = getRoot().getViewer().getVisualPartMap();
visualPartMap.put(visual, this);
if (visual instanceof Parent) {
for (Node child : ((Parent) visual).getChildrenUnmodifiable()) {
registerVisuals(child);
}
}
}
private void updateRegistration() {
unregisterVisuals();
registerVisuals(getVisual());
}
The performance impact is within unregisterVisuals() as it walks over the whole visual-part-map. I will leave you a message here when I made progress on the issue.
Best regards,
Matthias
|
|
| |
Re: [GEF4] AbstractFXTool & Policies [message #1721637 is a reply to message #1721424] |
Thu, 28 January 2016 20:28 |
|
Hi Camille, Matthias,
within FXClickableAreaBehavior I have implemented this based on a listener:
private final ListChangeListener<Node> curveNodeChildrenObserver = new ListChangeListener<Node>() {
@Override
public void onChanged(ListChangeListener.Change<? extends Node> c) {
Map<Node, IVisualPart<Node, ? extends Node>> visualPartMap = getHost()
.getRoot().getViewer().getVisualPartMap();
// ensure clickable area is properly registered/unregistered at/from
// visual part map.
while (c.next()) {
for (Node n : c.getRemoved()) {
visualPartMap.remove(n);
}
for (Node n : c.getAddedSubList()) {
visualPartMap.put(n, getHost());
}
}
}
};
I think that's probably the easiest solution to compensate this. In your case, however, I would embed such a listener within the visual part.
|
|
|
Re: [GEF4] AbstractFXTool & Policies [message #1721693 is a reply to message #1721637] |
Fri, 29 January 2016 08:40 |
Camille Letavernier Messages: 952 Registered: February 2011 |
Senior Member |
|
|
Hi Alexander,
I've done something similar for my case. The main difference is that I add recursive registration of new nodes (Skipping nodes coming from child parts), and install the listener recursively on the registered nodes
It's quite easy for me as I have a single abstract ContentPart that all others will inherit, but I think this would make sense in the GEF4 IVisualPart (To also cover all Feedback/Handle parts and relieve clients from anticipating this issue)
So I had to override the "registerAtVisualPartMap" and "unregisterFromVisualPartMap" methods to ensure that the children listener is recursively installed on all relevant nodes
I also had to create new register/unregister methods, because "registerAtVisualPartMap" uses the generic type, thus can only be used to register the root visual
protected final ListChangeListener<Node> nodeChildrenListener = (change) -> updateOnChange(change);
protected void updateOnChange(Change<? extends Node> change) {
IViewer<Node> viewer = getRoot().getViewer();
while (change.next()) {
for (Node removedNode : change.getRemoved()) {
unregisterVisuals(viewer, removedNode);
}
for (Node addedNode : change.getAddedSubList()) {
registerVisuals(viewer, addedNode);
}
}
}
@Override
protected void registerAtVisualPartMap(IViewer<Node> viewer, N visual) {
// Override the parent behavior to ensure that listeners are properly installed when required
registerVisuals(viewer, visual);
}
@Override
protected void unregisterFromVisualPartMap(IViewer<Node> viewer, N visual) {
// Override the parent behavior to ensure that listeners are properly uninstalled
unregisterVisuals(viewer, visual);
}
protected void unregisterVisuals(IViewer<Node> viewer, Node visual) {
Map<Node, IVisualPart<Node, ? extends Node>> visualPartMap = viewer.getVisualPartMap();
// Do not unregister child content parts; only child figures for this content part
if (visualPartMap.get(visual) == this) {
visualPartMap.remove(visual);
if (visual instanceof Parent) {
Parent parentVisual = (Parent) visual;
for (Node childNode : parentVisual.getChildrenUnmodifiable()) {
unregisterVisuals(viewer, childNode);
}
parentVisual.getChildrenUnmodifiable().removeListener(nodeChildrenListener);
}
}
}
protected void registerVisuals(IViewer<Node> viewer, Node visual) {
Map<Node, IVisualPart<Node, ? extends Node>> visualPartMap = viewer.getVisualPartMap();
IVisualPart<Node, ? extends Node> currentPart = visualPartMap.get(visual);
if (currentPart == null) { // Not yet registered
visualPartMap.put(visual, this);
}
if (visual instanceof Parent && (currentPart == null || currentPart == this)) { // Do not register nested visuals if they are associated with a child part
Parent parentVisual = (Parent) visual;
parentVisual.getChildrenUnmodifiable().addListener(nodeChildrenListener);
for (Node child : parentVisual.getChildrenUnmodifiable()) {
registerVisuals(viewer, child);
}
}
}
Camille Letavernier
|
|
|
Goto Forum:
Current Time: Fri Apr 26 19:31:48 GMT 2024
Powered by FUDForum. Page generated in 0.03838 seconds
|