Community
Participate
Working Groups
Created attachment 268894 [details] How to reproduce the bug In the GEF logo example, when moving connections selected in group the connection visuals stop being redrawn when dragging after a few seconds. Then when releasing the mouse to stop moving the group, the connection visuals are not redrawn until several seconds later. How to reproduce: 1. Select a group of connections where none of the connections are attached to anything. 2. Drag them around for a few seconds. 3. Release. Expected: The connection visuals follow the mouse when dragging. When releasing the visuals are located where mouse was released. Actual: The connection visuals does not follow the mouse after a few seconds of dragging. Several seconds is needed for the visuals to be refreshed when releasing the mouse. The attached gif first shows what it looks like when moving a group not containing connections which is problem free and then what it looks like when dragging a group of connections. This was tested with integration release 5.0.0.201706100201 of GEF.
There are multiple reasons for the observed behavior: 1. IBendableContentPart realizes transforming and resizing via changes to the bend points, i.e. not a single transformation is adjusted, but new positions are computed for the individual bend points. 2. ClickDragGesture does not handle events asynchronously, i.e. new events are only handled when processing of previous events is finished. Ideally, the gesture would know that a previous drag event is still being processed, so that intermediary events can be dropped, or processing thereof can be postponed (for the final event of the same type). 3. The final mouse location (when releasing the initially depressed mouse button) is not considered, because the handlers perform changes in response to drag events. When the button is released, the changes are committed. 4. The JavaFX/SWT integration (FXCanvas) is slow w.r.t. redrawing (blit necessary to copy from JavaFX pixel buffer to SWT image data). Moreover, I experienced differences in responsiveness when only using redraw() or also update() within FXCanvasEx.EventDispatcherEx#dispatchEvent() to signal that a new frame should be presented (redraw interval duration exceeded). Each of which can be tackled individually: 1. Realize visual transformations for bendables using a JavaFX Affine. + no anchors need to be changed + only a transformation needs to be changed + solves the issue locally - needs to be done with caution to prevent problems (from the top of my head: redundant data (translation information is incorporated into the bend points) => duplicate transformations, incompatible bend points, possibly wrong Connection positions, etc.) 2. Process events asynchronously, dropping intermediary events of the same type as long as such an event is still processed. + event listening is "decoupled" from event processing + application remains responsive, even though less updates are carried out (as the corresponding events are dropped due to ongoing processing) - some handlers need to be run for all events (would need a mechanism for that) - some code needs to be executed on the JavaFX Application thread (would need to spawn that) - asynchronous control flow is difficult to reason about and to debug (synchronization mechanisms needed) 3. The gesture could emit a MOUSE_DRAGGED event for the final MOUSE_RELEASED event, reusing the mouse location from the released event, and the button state from the initial MOUSE_PRESSED event. OR: The handler could evaluate the event data given in endDrag() to perform an action similar to the one performed in drag(). + the final state will match the mouse location 4. We should contribute fixes directly to JavaFX to make FXCanvas perform better and enhance the support for keyboard and touch events, which is currently handled by GEF's FXCanvasEx. In the meantime, you could try to enable/disable the update() call within FXCanvasEx and check if it enhances your situation. I believe that working with bend points is just too slow in some cases, where it would be awesome if the transformation could be used, at least for updating the visuals during interaction. I will open tickets for the individual tasks after thinking about it again. As a fix will not be released until Oxygen.1 (although most certainly published way earlier on the master branch), the following workaround can be used in the interim: you can override the setVisualTransform() and getVisualTransform() methods within an IBendableContentPart implementation to replace the slow but consistent default behavior with a fast transformation-based alternative, as well as having to ensure that bend points (corresponding visuals, operations, policies, and handlers) are still working correctly. I believe that this issue is not critical, because it is 1. less noticeable when running standalone, 2. depending on the performance of the individual system where the application is executed, 3. can be obviated using the outlined workaround, and 4. is only related to performance, not to function ("make it work, make it right, make it fast").
This does not seem to be as big of a problem in 5.0.2.201805110202, the connection visuals do not get stuck as long as shown in the gif. However since it is still there I profiled moving the 4 connections for 30 seconds and I found that 27% of the time is spent in org.eclipse.gef.common.collections.ListListenerHelperEx#removeListener and 12% of the time is spent in org.eclipse.gef.common.collections.ListListenerHelperEx#notifyListChangeListeners. This for org.eclipse.gef.fx.nodes.Connection#anchors when calling org.eclipse.gef.fx.nodes.Connection#setAnchor. Attaching a patch that disables ListListenerHelperEx#removeListener ListListenerHelperEx#notifyListChangeListeners for Connection#anchors. Also attaching the Java Mission Control report.
Created attachment 274301 [details] hack to speed up connection transforms
Created attachment 274302 [details] Java Mission Control report of dragging connections as shown in the gif
With the patch applied, the connection visuals do not get stuck as shown in the gif.
See also the discussion in the forum thread https://www.eclipse.org/forums/index.php/t/1096276/ to improve the performance by removing the ListListenerHelperEx#checkCursor method invocations.
I removed the checkCursor() calls from ListListenerHelperEx. I will not yet close this ticket, because I want to do further analysis first.
Currently, the ObservableListTests.listenersNotProperlyIterating test cases fails with the following exception: ava.lang.ArrayIndexOutOfBoundsException: -1 at org.eclipse.gef.common.collections.ListListenerHelperEx$AtomicChange.getAddedSubList(ListListenerHelperEx.java:139) at org.eclipse.gef.common.collections.ListListenerHelperEx$AtomicChange.wasAdded(ListListenerHelperEx.java:208) at javafx.collections.ListChangeListener$Change.wasReplaced(ListChangeListener.java:213) at org.eclipse.gef.common.collections.ListListenerHelperEx$AtomicChange.wasReplaced(ListListenerHelperEx.java:223) at org.eclipse.gef.common.tests.ObservableListTests$9.onChanged(ObservableListTests.java:664) at org.eclipse.gef.common.collections.ListListenerHelperEx.notifyListChangeListeners(ListListenerHelperEx.java:650) at org.eclipse.gef.common.collections.ListListenerHelperEx.fireValueChangedEvent(ListListenerHelperEx.java:600) at org.eclipse.gef.common.collections.ObservableListWrapperEx.add(ObservableListWrapperEx.java:79) at org.eclipse.gef.common.tests.ObservableListTests.listenersNotProperlyIterating(ObservableListTests.java:897) The reason for the failing test is the change that the checkCursor() method has been removed from the ListListenerHelperEx class. I have set the test case temporary to ignore to make the build process succeed again, but we should analyse this deeper in the near future.
We introduced checkCursor() to prevent violations of the cursor contract, as this is a very likely thing to happen and I still think it is advantageous to have such a check. Instead of removing the checkCursor() we could think of introducing the notation of a production mode here as well (we have already done such a thing in AdapterInjectionSupport) and to only evaluate it if not in production mode. That should be fast enough for the production case, while in dev mode we would still have the desired safety. If we tend to introduce a production mode here as well, we could think of extracting this as a general property of GEF common.
