Replies: 6 comments 17 replies
-
|
...maybe I should work with a And I found this comment mentioning feedback commands... But I did not found any code comments or docu. So maybe this is only for internal usage? |
Beta Was this translation helpful? Give feedback.
-
|
I believe the behavior of the position calculation in some of the tools changed from working on the absolute position and handing that over to the snapper to having the snapper only snap the delta of the previous and current position. You are correct in that there seems to be no obvious mentioning of this somewhere and based on your previous feedback we already improved the PR template so that such changes are better transferred and reported on release in the future. If you need to snap absolute positions, you may need to override respective handler (e.g., this is how it was done in another project for the ghost element feedback: https://github.com/axonivy/process-editor-client/pull/475/files#diff-8c754a52dc5e2ac166d5b3bfb3653a990a22aec24043017b9f625525712a4133R86). In general, this is very valuable feedback to us so we know which parts of the framework may need better customizablity. As for feedback commands, they are just regular commands with an additional priority and an already "empty" undo/redo handling, i.e., really more a convenience class that can be used to implement the handlers of feedback actions. All feedback actions that are registered with the feedback action dispatcher are automatically re-applied after each model update (in the order given by the priority of the handling command, see Client-side visual feedback such as moving an element (or creating an element with a ghost) is typically implemented as feedback that is active (i.e., registered with the feedback action dispatcher) while we are performing the action. So in your case, it might be that you need to adapt the position calculation in the |
Beta Was this translation helpful? Give feedback.
-
|
Hi @martin-fleck-at On the other hand, you mentioned that the snapper now only provides the delta of the previous and current position. And this is exactly what I observed. Would you mean also a snapper can still be a good choice to recalculate the delta position in my case? |
Beta Was this translation helpful? Give feedback.
-
|
To give you a better example of what I did with trying to adapt my snapper. I implemented (for example for the divider problem) the following snapper : @injectable()
export class BPMNElementSnapper implements ISnapper {
constructor(public grid: { x: number; y: number } = { x: 1, y: 1 }) { }
snap(position: Point, _element: GModelElement): Point {
if (isLaneDivider(_element)) {
return this.findLaneDividerSnapPoint(_element, position);
}
return {
x: Math.round(position.x),
y: Math.round(position.y)
};
}
private findLaneDividerSnapPoint(element: GModelElement, position: Point): Point {
const x = 0;
const y = position.y;
return { x: x, y: y };
}
}Very easy code. I just set the x(delta) always to 0. And this looks at the first glance fine but if I move the mouse aggressive left/right the element suddenly changes the x position even when I set it to 0 - this is the strange thing.....
This effect also happens when I move the mouse very fast. |
Beta Was this translation helpful? Give feedback.
-
|
That is very interesting but it might have something to do with how the calculated position is used in the end. I think I might have to dig a little bit deeper into your code to see what is really going on. The state that you show above, is that in your current main branch or do you have a dedicated branch that I could have a look at? |
Beta Was this translation helpful? Give feedback.
-
|
Hi @martin-fleck-at, I am sorry, but at the moment I did not understand anything. Even analyzing the code you referred did not help me. To me it looks over-killed extending the From the overall concept of GLSP I expect that I should be able to
at least I understand that I can not or must not overwrite the bounds property of an element in any action as this is always readonly property - right? Or did you expect that a You can see my current code base here and here. But it is more experimental at the moment. |
Beta Was this translation helpful? Give feedback.
-
|
I had a look at the code and I believe I know why the problem is happening: When you move the mouse really fast, the element that you are dragging (e.g., the lane divider) may not be moved fast enough and so the element we are trying to snap will be whatever else is beneath the mouse cursor. In most cases this will be the graph but it might be any other element as well (e.g., an edge). Since we are calculating the position difference first (on whatever element), snap that and then apply the delta to all the moving elements, it can happen that your snapper gets a graph element and therefore returns a delta that allows for horizontal movement. You can see the whole calculation in the So we can either adapt the FeedbackMoveMouseListener or - as a more slim workaround - adapt the snapper to not only consider the given element but also any selected elements, i.e., do something like this: @injectable()
export class BPMNElementSnapper extends GridSnapper {
constructor() {
super({ x: 5, y: 5 });
}
override snap(position: Point, element: GModelElement): Point {
if (isElementOrSelected(element, isLaneDivider)) {
return { ...position, x: 0 };
}
// move routing-points by given grid
if (isElementOrSelected(element, elem => 'volatile-routing-point' === elem.type)) {
return super.snap(position, element);
}
// default move on a 1x1 grid
return { x: Math.round(position.x), y: Math.round(position.y) };
}
}
/**
* Returns true if the given element or any of the selected elements in the indexed graph matches the given predicate.
*/
function isElementOrSelected(element: GModelElement, predicate: (modelElement: GModelElement) => boolean): boolean {
try {
return predicate(element) || getMatchingElements(element.index, isSelected).some(predicate);
} catch (error) {
return false;
}
}WARNING: I did not fully check the code so that might interfere with other places where the positioning is used. |
Beta Was this translation helpful? Give feedback.
-
|
Hi @martin-fleck-at, your guess is absolutely correct. I've seen exactly this by log messages, that the graph element or other elements has actually been transferred. I will try your suggestion extending the |
Beta Was this translation helpful? Give feedback.
-
|
Yes - you code works and it looks like it is solving the problem! I will continue working with this and implement the other snapping rules.... |
Beta Was this translation helpful? Give feedback.
-
|
Hi @martin-fleck-at , I need to correct myself. It does not work. Or at least the method From the following code snippet you would expect that ....
override snap(position: Point, _element: GModelElement): Point {
if (isElementOrSelected(_element, elem => 'lane-divider' === elem.type)) {
console.log(' ---> element (' + _element.type + ')');
return { ...position, x: 0 };
//return this.findLaneDividerSnapPoint(_element, position);
}
// default move 1x1...
return {
x: Math.round(position.x),
y: Math.round(position.y)
};
}
}
function isElementOrSelected(element: GModelElement, predicate: (modelElement: GModelElement) => boolean): boolean {
try {
return predicate(element) || getMatchingElements(element.index, isSelected).some(predicate);
} catch (error) {
return false;
}
}But the console.log shows also again the graph element :-(
|
Beta Was this translation helpful? Give feedback.
-
|
I think that the graph shows up is fine because you get the graph from the mouse event and then we go through the whole index of elements to find if a lane divider was selected. So the |
Beta Was this translation helpful? Give feedback.
-
|
Finally I found a working solution (not perfect). My code looks like this - Note that I need to explicit check the 'graph' case at the beginning of the snap method. Found no other solution. Only returning 0,0 in the 'graph' case healed the situation (very very strange): @injectable()
export class BPMNElementSnapper extends GridSnapper {
constructor() {
super({ x: 1, y: 1 });
}
override snap(position: Point, element: GModelElement): Point {
if ('graph' === element.type) {
console.log('causing problems...!');
return { x: 0, y: 0 };
}
if (this.isElementOrSelected(element, elem => 'lane-divider' === elem.type)) {
return this.findLaneDividerSnapPoint(element, position);
}
// default move 1x1...
return {
x: Math.round(position.x),
y: Math.round(position.y)
};
}
/**
* Returns true if the given element or any of the selected elements in the indexed graph matches the given predicate.
*
*/
public isElementOrSelected(element: GModelElement, predicate: (modelElement: GModelElement) => boolean): boolean {
try {
return predicate(element) || getMatchingElements(element.index, isSelected).some(predicate);
} catch (error) {
return false;
}
}
/*
* This helper method computes the snap Position of a Lane-Divider.
* The position is based on the Bounds of the containing Pool.
* The final position is always on the x position of the Pool.
*/
public findLaneDividerSnapPoint(element: GModelElement, position: Point): Point {
let x = 0;
let y = position.y;
// test min / max position
if (hasArgs(element) && isBoundsAware(element)) {
x = 0;
const yMin = Number(element.args.ymin);
const yMax = Number(element.args.ymax);
console.log(' ---> element (' + element.type + ') position: ' + element.bounds.x + ',' + element.bounds.y + ' yMin/yMax=' + yMin + '/' + yMax
+ ' Point--> ' + position.x + ',' + position.y);
if (element.bounds.y + position.y < yMin) {
y = 0;
}
if (element.bounds.y + position.y > yMax) {
y = 0;
}
}
// return the new position;
return { x: x, y: y };
}
}
|
Beta Was this translation helpful? Give feedback.
-
|
That is very curious indeed! I do not understand why you need to intercept the graph like this. If it is only the graph and nothing else is selected, the custom method should not trigger. Furthermore, if you return |
Beta Was this translation helpful? Give feedback.
-
Yes |
Beta Was this translation helpful? Give feedback.
-
|
Isn't it the |
Beta Was this translation helpful? Give feedback.
-
|
Hi @martin-fleck-at , maybe we should take a step back and rethink the situation. I am afraid that we are now proceeding in a way that is far too complicated. Why don't you just send in a method like I'm doing the same thing in Jakarta EE in my workflow engine. The engine transmits life cycle events at specific points in the process. These allow implementers to change the properties of a process instance. The engine itself doesn't use these events. It is just an interceptor interface. |
Beta Was this translation helpful? Give feedback.
-
|
Hi @martin-fleck-at , sorry but I did not understand what I can do.
I think the requirement to fit a moved element to a specific position in a diagram could be a very general and often needed one. The easiest way for a diagram project would be to react on a specific event. If the event provides the ID of the moved element and the current move position, maybe it is possible that an eventListener can return a new custom position. |
Beta Was this translation helpful? Give feedback.
-
|
Right, we would need to override the |
Beta Was this translation helpful? Give feedback.
-
|
Ah OK - yes of course I can wait for you. No problem. |
Beta Was this translation helpful? Give feedback.
-
|
Great, thank you! I'll try to get to that part soon as it is quite a core issue I'd say. If you need a solution before I can get to, please let me know and I'll try to guide you through the customization but basically what you would need to do is bind your own ChangeBoundsTool that inherits from the GLSP one and override the creation of the mouse listeners to provide your own custom mouse listener implementation with the adapted methods. |
Beta Was this translation helpful? Give feedback.
-
|
Only if you like to have kind of prototype of your idea. Otherwise I can wait and test the next nightly build.... |
Beta Was this translation helpful? Give feedback.
-
Hi,
I have a strange behavior since I migrated from GLSP 1.x to 2.x. In our Open-BPMN project we have some special cases where elements are forced to snap in specific positions.
For example, a so called BPMN BoundaryEvent is always placed on the edge of a Task, and a Lane-Divider should not move out of its pool:
I solved this in the past with a custom
ISnapperimplementation, where I simply calculated the possible absolute new position of a snap-point. So the result of the methodsnap(position: Point, element: SModelElement): Pointwas always an absolute position in the diagram plane.This is no longer true for GLSP version 2.x. I can't find any notes regarding the changed behavior in the release notes. I am very confused about this issue and I did not found a solution. This topic does not have any relation ship to the new HelperLine feature.
If I run my old code in GLSP 2.x it looks like this caused by the calculation of the absolute position:
I wonder how the
ISnapperis working and if using aISnappermay be the wrong approach for my feature to move some elements only aligned to a internal line... ?Beta Was this translation helpful? Give feedback.
All reactions