Eclipse Community Forums
Forum Search:

Search      Help    Register    Login    Home
Home » Eclipse Projects » GEF » [GEF4] AbstractFXTool & Policies
[GEF4] AbstractFXTool & Policies [message #1721180] Mon, 25 January 2016 14:13 Go to next message
Camille Letavernier is currently offline Camille LetavernierFriend
Messages: 876
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 Go to previous messageGo to next message
Matthias Wienand is currently offline Matthias WienandFriend
Messages: 143
Registered: March 2015
Senior Member
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 #1721190 is a reply to message #1721184] Mon, 25 January 2016 15:08 Go to previous messageGo to next message
Camille Letavernier is currently offline Camille LetavernierFriend
Messages: 876
Registered: February 2011
Senior Member
Humm... I've looked at my own implementation of the FocusAndSelect policy instead of the GEF4 one. They used to be similar, but I haven't updated mine to match the new implementation, thus the confusion...

FXFocusAndSelectOnClickPolicy indeed works as expected, so I have to update my own version

Apologies for the confusion Smile

That's still quite a lot of changes compared to the previous implementation; I think I've missed something in the previous refactoring, so I'll dig a little bit more and see if I can fix it simply from my side. More information when I have double-checked my own implementation Smile

Regards,
Camille


Camille Letavernier
Re: [GEF4] AbstractFXTool & Policies [message #1721274 is a reply to message #1721190] Tue, 26 January 2016 09:16 Go to previous messageGo to next message
Camille Letavernier is currently offline Camille LetavernierFriend
Messages: 876
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! Smile

Thanks,
Camille


Camille Letavernier
Re: [GEF4] AbstractFXTool & Policies [message #1721307 is a reply to message #1721274] Tue, 26 January 2016 13:02 Go to previous messageGo to next message
Matthias Wienand is currently offline Matthias WienandFriend
Messages: 143
Registered: March 2015
Senior Member
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 #1721309 is a reply to message #1721307] Tue, 26 January 2016 13:29 Go to previous messageGo to next message
Matthias Wienand is currently offline Matthias WienandFriend
Messages: 143
Registered: March 2015
Senior Member
Hi Camille,

just to let you know, I adjusted the FXFocusAndSelectOnClickPolicy, so that it should fit your needs now.

Best regards,
Matthias
Re: [GEF4] AbstractFXTool & Policies [message #1721311 is a reply to message #1721309] Tue, 26 January 2016 14:10 Go to previous messageGo to next message
Camille Letavernier is currently offline Camille LetavernierFriend
Messages: 876
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 Go to previous messageGo to next message
Matthias Wienand is currently offline Matthias WienandFriend
Messages: 143
Registered: March 2015
Senior Member
Hi Camille,

you are definitely asking the right questions Wink

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 #1721424 is a reply to message #1721339] Wed, 27 January 2016 12:09 Go to previous messageGo to next message
Camille Letavernier is currently offline Camille LetavernierFriend
Messages: 876
Registered: February 2011
Senior Member
Thanks Matthias,

I've tried this approach but it only works after an explicit refresh. The trick is that JavaFX doesn't create the children of a Label immediately when doing a setText/setImage. These elements are created later, during the rendering of the FX Graph. This means that calling updateRegistration before the rendering happens will not properly register the children of a Label

Since createVisual and refreshVisual happen before the rendering, it is not possible to properly register the visuals implicitly created by JavaFX. The only solution in this case is to listen on the children of a Visual (Parent) and update the VisualPartMap whenever changes occur

I will investigate on this approach a little bit more, but I think it would be best to implement this directly in GEF4

Regards,
Camille


Camille Letavernier
Re: [GEF4] AbstractFXTool & Policies [message #1721637 is a reply to message #1721424] Thu, 28 January 2016 20:28 Go to previous messageGo to next message
Alexander Nyssen is currently offline Alexander NyssenFriend
Messages: 233
Registered: July 2009
Location: L√ľnen
Senior Member
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 Go to previous message
Camille Letavernier is currently offline Camille LetavernierFriend
Messages: 876
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
Previous Topic:Adding another GraphicalViewer in GraphicalEditorWithFlyoutPalette
Next Topic:[Zest] Is it possible to use the LayoutAlgorithms without using SWT?
Goto Forum:
  


Current Time: Mon Apr 23 15:42:22 GMT 2018

Powered by FUDForum. Page generated in 0.02348 seconds
.:: Contact :: Home ::.

Powered by: FUDforum 3.0.2.
Copyright ©2001-2010 FUDforum Bulletin Board Software