Add Helper Lines module to provide feedback and helper line manager

[martin-fleck-at]

Helper Lines:

• Draw helper lines when bounds of given elements match with existing
• Draw selection bounds if more than one element is considered
• Use manager class to draw them on client-side move or bounds change

Integration:

• Create PointPositionSnapper for more fine-grained snapping
• Use PointPositionSnapper in PointPositionUpdater
• Use PointPositionUpdater wherever necessary

Other changes:

• Centralize unsnap-modifier as Shift key
• Ensure unsnapped moves are possible in keyboard tool
• Fix typo in 'SetBoundsFeebackCommand' and some file names
• Make popup mouse event transparent by default

Fixes eclipse-glsp/glsp#621
Fixes eclipse-glsp/glsp#1190

[tortmayr]

Awesome change at first glance it seems to work really well. Nevertheless I would prefer If we add this feature as an "experimental feature" for now (similar to the a11y feature). So I would not add it to the set of default modules and mark it in the
feature matrix as experimental.This gives the opportunity to test it out in practice and polish the behavior/ fix potential bugs.

Also it seems like the helper line display as a bit too eager. If I simply select a node, the helper lines are displayed for a short period (flicker). IMO we should only display them if an actual move is in progress.

Please also make sure to add all newly created modules to the main index.

You did a lot of restructuring/refatoring in our default tools implementation. While is is not a breaking change per se we should still mention it in the changelog so that any adopters that customize the default tools know that there might be conflicts.

[martin-fleck-at]

@tortmayr Thank you for your review! I updated the PR accordingly. I'll also open a PR on the glsp parent repository to update the feature matrix as soon as we have this PR merged.

[tortmayr]

Thanks for the update! Looks good to me

[rsoika]

Hi @martin-fleck-at , this implementation looks really cool!
Can you please give a short example how to integrate this feature into a Diagram Module? I have no idea how to do this.

[martin-fleck-at]

Hi @rsoika, sure, what is it you need exactly? The helperLineModule is not part of the default modules but can be added like any other module to the diagram configuration. You can see that in the workflow example:

• Standalone - Configuring the container with custom modules:

  glsp-client/examples/workflow-glsp/src/workflow-diagram-module.ts

  Line 85 in f286b9d

  return initializeDiagramContainer(container, workflowDiagramModule, directTaskEditor, helperLineModule, ...containerConfiguration);

• Theia Integration - Implement configureContainer and use the customized container: https://github.com/eclipse-glsp/glsp-theia-integration/blob/237510c23d1d50591e700c704da021c6c7155588/examples/workflow-theia/src/browser/diagram/workflow-diagram-configuration.ts#L29

[rsoika]

ok, understood. Works great. Better than my own implementation.

But sometimes it looks a little bit strange, even if the alignment is still correct:

This oblique lines seem to arise only with circle elements and not in rectangles.
But maybe this is something caused by my view implementation in Open-BPMN.....

[martin-fleck-at]

@rsoika That looks very interesting indeed and might be the result of some rounding going on. By default we only draw lines if either their coordinates match but we do use an epsilon of 1 or 2 (depending) so maybe this is the result of that. In any case, the epsilon can be given when the action is created but I think an easier mechanism to customize that might be more useful. I'll see if I can reproduce this with circles somehow on our end.

[rsoika]

Thanks for you response. I tested now more in deep and in general this is really a great new feature!.

But I also come across a new requirement:
Not every element in an BPMN diagram is useful for computing the alignments. For example labels of a Event are not relevant here:

In this example, the circle element and the label below are two separate GNodes witch stick together but can also be moved separately. Also edges should not be used for alignment.

What would be the best way to provide a kind of filter-set of elements, that can be ignored by the algorithm?

[martin-fleck-at]

@rsoika I'll have a look what we can do to ignore certain elements a bit more generically some time that week and come back to you. As a quick fix it might be easiest to override the calcReferenceBounds method in the DrawHelperLinesFeedbackCommand as we use that to calculate the helper lines based on all selected, bounds-ware elements:

glsp-client/packages/client/src/features/helper-lines/helper-line-feedback.ts

Lines 110 to 112 in f286b9d

	protected calcReferenceBounds(referenceElements: BoundsAwareModelElement[]): Bounds {
	return referenceElements.map(element => element.bounds).reduce((combined, next) => Bounds.combine(combined, next), Bounds.EMPTY);
	}

[rsoika]

Hi @martin-fleck-at ,
Yes this was a good hint. I created a filter like this:

        const filteredElements = referenceElements.filter(element =>
            element.type !== 'BPMNLabel' &&
            element.type !== 'sequenceFlow' &&
            element.type !== 'messageFlow' &&
            element.type !== 'association' &&
            element.type !== 'lane-divider'
        );

To ignore a set of elements. But it is not only the method calcReferenceBounds that needs such a custom filter. Also the method getMatchingElements called by the method execute returns a much to large list of elements. The getMatchingElements for example returns also edges which I want to ignore too.

Applying such a filter to both methods, results in a wonderful pretty behavior.

Now the question is: how can I - as a user of the module - provide the module with such a filter method or a set of element types to be ignored? As you are the JavaScirpt expert here I leave the answer to you ;-)

[rsoika]

Hi @martin-fleck-at ,
I think I found out also the reason for the strange behavior regarding the wrong rounding.
This happens if you do not explicitly bind a GridSnapper to the diagram. My expectation was that this should not be necessary as the HelperLine Feature covers this already. But this is not the case.

If you do not bind an external GrindSnapper to your diagram, than your bounds can get Floating-point numbers. And from that moment on the computation of the snap position via the HelperLineManager go crazy and you no longer get equal X/Z position of two elements. This has nothing to do with the kind of the visual representation.

So currently you need to set a GrindSnapper!

Now, in Open-BPMN we have the situation that for example a Task has a default width of 110x50 and an Event has 36x36.
For this dimensions, it is not possible to set a GridSnapper to something like 10 or 5 or 20. This does of course not work, because then the elements become offset in any case! And this has of course nothing to do with the HelperLine Feature.

So a correct setup in Open-BPMN is to set an GridSnapper with 1x1

bind(TYPES.ISnapper).toConstantValue(new GridSnapper({ x: 1, y: 1 }));

and in the init method of the HelperLineManager I ignore the idea to set the snapSize depending on the external GridSnapper. When I leave the default value of the snapSize to { x: 20, y: 20 } or something like { x: 10, y: 10 } the result is what I expect from the HelperLine Feature:

Of course the snapSize is one of the most important variables in the HelperLine Feature.

So my suggestion is that, the HelperLine Feature is using a snapper of { x: 1, y: 1 } per default (if this is possible?). And the snapSize is something the diagram implementation can define by itself when configuring the Module.

I also think the HelperLine Feature should not be mixed up with the GridSnapper. These are at the end two different concepts.

What did you think?

[martin-fleck-at]

@rsoika Thank you for the detailed feedback! I agree that the options (of which there are now many) should be more configurable from a central place. From you description, I assume that we only need to apply a single filter on the elements that are then split into a set of selected elements and reference elements. I opened up a PR that should cover those use cases: #312

I also through about the defaults a little bit longer and I think you are right that using a { x: 1, y: 1 } as default move delta is more correct than using a high number and the Grid Snapper. I still like the idea of re-using the Grid Snapper so I just did that for the workflow module to also show how custom options can be applied. I also adapted the default filtering of the elements to only consider elements visible on the canvas to ease the computation load. However, all of this can now be customized with the PR that I have prepared. So in your case, you would need to provide a custom alignmentElementFilter which either re-uses the default filter and then applies your type filtering or does something completely custom. It would be great if you could have a look at the PR as well to see if that covers your use case.

[rsoika]

yes, I will test this.
But I have a general question:
When I build the glsp-client locally with yarn install how can I than link my own project to use the new local dependency 2.1.0-next ?

I guess I need to remove my :

  "dependencies": {
    "@eclipse-glsp/client": "2.1.0-next",
   .....
  }

and replace this with something like workspace?? Can you please assist me with my local dev setup. I am really not a yarn expert :-/

[martin-fleck-at]

@rsoika Good question! I'm also not too familiar with yarn and for GLSP we use a yarn link script that ensures everything works as it should (see README and the script). However, the problem is that you always need to ensure manually through resolution that you have the correct version of indirect dependencies as well.

Alternatively, if you just can use verdaccio to set up a local npm registry really quickly, deploy the version you want to use and consume it from there. You can install it globally or just as a dev dependency in your project and then call yarn verdaccio and set the registry accordingly.

[rsoika]

Thanks! Ok I was not sure if yarn link is recommended. I will try this...

[rsoika]

Ok, after several hours and a bunch of ugly hacks I am now able to build my project against the pull request.

Now again I need your help: Can you please give an example code how the AlignmentElementFilter should be used? I do not understand this.

[martin-fleck-at]

@rsoika Purely from what you shared in your code above, I'd hope that setting the filter to something like this should be enough:

   bind<IHelperLineOptions>(TYPES.IHelperLineOptions).toConstantValue({
        alignmentElementFilter: element =>
            element.type !== 'BPMNLabel' &&
            element.type !== 'sequenceFlow' &&
            element.type !== 'messageFlow' &&
            element.type !== 'association' &&
            element.type !== 'lane-divider'
    });

And you can also provide the minimumMoveDelta (previous snapSize) like that.

[rsoika]

@martin-fleck-at - thanks for the missing part.
Yes now its behavior is nearly perfect!! ;-)

But in Open-BPMN we have another special case. We are using containers to group elements (a basic concept in BPMN)

Before your last update containers did not work at all. But now its behavior seems to be improved.

But as you can see, it is not working inside a container. I think the problem here is, that the coordinates within a container are relative to the position of the container itself. This means we need to adjust the bounds. But I cant identify the method relevant here (I know you do not like comments ;-)

[martin-fleck-at]

@rsoika That is an interesting point, I didn't actually test with containers as by default I only go for the top-level alignable element, see https://github.com/eclipse-glsp/glsp-client/pull/312/files#diff-6c893407606d7218b882368bcfc480b84eca96bfee6d2e0b435b3887564453beR86. I believe that is the behavior change that you are seeing as well since you set a custom filter, it is now going for all elements. If you wanted - but apparently you do not ;-) - to get that behavior back you would need to simply add your filter, i.e.:

     element => DEFAULT_ALIGNABLE_ELEMENT_FILTER(element) &&
            element.type !== 'BPMNLabel' &&
            element.type !== 'sequenceFlow' &&
            element.type !== 'messageFlow' &&
            element.type !== 'association' &&
            element.type !== 'lane-divider'

Even if you do not use the DEFAULT_ALIGNABLE_ELEMENT_FILTER you may be interested to filter out only visible elements (isVisibleOnCanvas).

I'll see if I can get a closer look at the container in the next few days as well.

[rsoika]

Hi @martin-fleck-at , I think it is nearly perfect.
My current idea regarding the containers, is to test within the method calcHelperLines if the elements are in a container or in the root element.
If the elements are part of a container, we need the bounds of the container, which are now the x/y offset. And this offset need to be added to the final helperline coordinates.

[martin-fleck-at]

@rsoika I pushed another commit to my PR (94ba501) which should fix the compartment handling by using absolute bounds. Only using parent-relative bounds is not enough as it might be that you have two (or more compartments) and their children are all aligned similarly resulting in helper lines in compartments that you are not even working on. I believe with this, we should be very close to a good solution :D

[rsoika]

@martin-fleck-at I was afraid that you would want to solve this special use-case with two or more compartments ;-)

I personally do not think that this is necessary, since users can also realign the compartments by themself. So I think it should be fine if helperlines work only within the boundaries of a compartment and do not go accross multiple compartments.
In very large diagrams, this can even be too much of help:

But anyway, I am not sure if the absolut bounds are working here. At least in my own implementation the helperlines where only be placed correctly when I took the bounds of the compartment as an offset to compute the helper line.

Look now what is happening when I move the elements or zoom the diagram:

[martin-fleck-at]

@rsoika Interesting point! I'll double check with the zooming because that is definitely a bug. As for the compartments, it is a bit hard to solve that generically and it really depends on how compartments are used. If it is mostly about structures cross-compartment lines may make sense but definitely not in all cases. However, only taking the compartment-relative bounds will not solve the problem as multiple elements (in different compartments) may then match the boundaries so in that case we would already need to make that distinction when checking which elements should be considered alignable given the selected reference elements. I wonder if I should introduce another filter for that so you could ensure that only elements in the same compartment are considered.

[rsoika]

hi @martin-fleck-at , yes I think we should not start over-engineering it now. I agree with you that compartments are very special and depend on the type of a diagram.
On the other hand, I now understand that the alignmentElementFilter can also depend on a more specific context. So in my case I would not only filter by type but also only to elements in the same container. And this a very special requirement to Open-BPMN. This is nothing that have to be covered by the HelperLine Feature.
I do not want that you investigate to much time as I can easily adapt this feature more to my needs later.
If you find out why the core functionality got lost this would be great. The version before your last change was really good.

[martin-fleck-at]

@rsoika Thank you for your continuous feedback! I pushed a large commit that should fix the lines now (I used getAbsoluteBounds instead of toAbsoluteBounds) and also provides the reference element ids to the alignmentElementFilter so you already know which elements are basically selected and being moved.

[rsoika]

Hi @martin-fleck-at ,
Yes now it is working also fine in containers.

[rsoika]

There is something that I did not notice before. Sometimes helperlines are detected where no helper line should be detected.
It looks to me that the element that I am moving is also considered as an alignable element.

[martin-fleck-at]

@rsoika Could you turn off debugging through the options (and ensure that the general debugging level is DEBUG)? That should give you info about the elements that are being matched in the console log of the browser. Purely from the screenshot I'm also not sure to which elements the line go but it really shouldn't be the element that is being moved.

[rsoika]

Yes I will try this to analyze the issue further.
stupid question: How do I switch on the debug level?

[martin-fleck-at]

@rsoika So the debug flag for this feature is in the options:

   bind<IHelperLineOptions>(TYPES.IHelperLineOptions).toConstantValue({
        [...]
        debug: true
    });

this is picked up by the helper-line-feedback.ts which uses the default GLSP logger on "debug" level. You can change the log level of the GLSP logger with:

    bindOrRebind(context, TYPES.LogLevel).toConstantValue(LogLevel.log);

[rsoika]

The reason for the magic helperlines were embedded elements in my bpmn model nodes.
For example a Task node includes a mult-line-text node and a Event node includes an icon and so on. And all this 'inner' elements become part of the alignableElements.
Extending my filter solves the problem.
But it is a surprising effect. If this was your intention, we can leave it at that.

[martin-fleck-at]

@rsoika Thank you! Yeah, previously we only looked at top level elements but that doesn't work in some scenarios like compartments and may not be a suitable default. So we really go through the whole model graph and treat each element as a candidate. By default we ignore elements not visible on the canvas, edges, decoration, and labels but if you provide a custom filter you are back to all elements again. Since it is impossible to find a perfect default here and the adaptation can be done with the filter, I'd say we leave it as is. I really appreciate your feedback on all of this and I'm sure the feature is a lot better because of it, thank you!

[rsoika]

Yes - with the the filter it is now easy to define the behavior and this gives users much more flexibility how they use the helper lines. I would agree that this is the final version.

[rsoika]

Hi @martin-fleck-at I have one more question regarding the relationship of an external snapper and the helperlines

In the following example I have 3 elements. The first and last are in line. The element in the middle is not. Using a snapper with 10x10 makes it impossible to get the elements in line.

I do not understand what is happening here and how I can influence the behavior. Is it the external snapper that avoids to get the middle element in line? Or does this have something to do with the getMinimumMoveDelta ?

[martin-fleck-at]

@rsoika Yes, it is definitely the external snapper. The helper line snapping (minimum move delta) is only if you already have a helper line and you want to move through it. What you can do is use the shift key and only then move the element as the shift key disables the snapping. If you want you can release the shift key as soon as you see a helper line as it restores the snapping (and helper line snapping) which makes it easier to move along the helper line

[rsoika]

ah - this worked. I did not know about the shift-key feature...

[rsoika]

@martin-fleck-at , can you merge your changes now? So I can continue work on a 'normal' release?

[martin-fleck-at]

Sure, I'll try to ask for a review from Tobias today so we can get that in.