issue with position of GIssueMarker popup

[rileydanejohnston]

Hello,
I’m trying to see if there is a way to fix the positioning of a GIssueMarker’s popup when the GIssueMarker is a child of a GEdge. Instead of the popup appearing near the GIssueMarker/cursor, the popup appears either to the left of the client window or at the top of the client window. I think the PopupPositionUpdater class is updating the left/top position to 0px when the popup’s boundingClientRect values are negative.

The problem only happens when I have a GIssueMarker as a child of a GEdge. Is there a way to make it so the popup appears next to the GIssueMarker/cursor like it usually does?

What the problem looks like:

Here’s what the inline styles look like (top: 0px):

Here’s what printing action.bounds in the popup-factory looks like. I think this is what's triggering PopupPositionUpdater to change position top to 0px: { x: 359, y: -5384, width: -1, height: -1 }

[martin-fleck-at]

@rileydanejohnston Thank you for raising this! I just tested this and you are correct, there is something wrong with the position calculation on edges. As far as I can tell, we do not calculate the absolute bounds of the issue marker correctly because the localToParent method is not implemented by SEdge whereas nodes inherit a correct implementation from SShapeElement.

I have an idea how it could work and I can create a PR for it but you can check it first if you want. Basically, we need to adapt the absolute bounds calculation to properly consider routable elements. For this, you need to subclass and rebind the GlspHoverMouseListener and use the following overridden implementation:

@injectable()
export class MyHoverMouseListener extends GlspHoverMouseListener {

    protected override computePopupBounds(target: SModelElement, mousePosition: Point): Bounds {
        // same implementation as super class just with custom 'getAbsoluteBounds'
        let offset: Point = { x: -5, y: 20 };

        const targetBounds = this.getAbsoluteBounds(target);
        const canvasBounds = target.root.canvasBounds;
        const boundsInWindow = Bounds.translate(targetBounds, canvasBounds);
        const distRight = boundsInWindow.x + boundsInWindow.width - mousePosition.x;
        const distBottom = boundsInWindow.y + boundsInWindow.height - mousePosition.y;
        if (distBottom <= distRight && this.allowSidePosition(target, 'below', distBottom)) {
            // Put the popup below the target element
            offset = { x: -5, y: Math.round(distBottom + 5) };
        } else if (distRight <= distBottom && this.allowSidePosition(target, 'right', distRight)) {
            // Put the popup right of the target element
            offset = { x: Math.round(distRight + 5), y: -5 };
        }
        let leftPopupPosition = mousePosition.x + offset.x;
        const canvasRightBorderPosition = canvasBounds.x + canvasBounds.width;
        if (leftPopupPosition > canvasRightBorderPosition) {
            leftPopupPosition = canvasRightBorderPosition;
        }
        let topPopupPosition = mousePosition.y + offset.y;
        const canvasBottomBorderPosition = canvasBounds.y + canvasBounds.height;
        if (topPopupPosition > canvasBottomBorderPosition) {
            topPopupPosition = canvasBottomBorderPosition;
        }
        return { x: leftPopupPosition, y: topPopupPosition, width: -1, height: -1 };
    }

    protected getAbsoluteBounds(element: SModelElement): Bounds {
        // same as default getAbsoluteBounds but considers SRoutableElement
        const boundsAware = findParentByFeature(element, isBoundsAware);
        if (boundsAware !== undefined) {
            let bounds = boundsAware.bounds;
            let current: SModelElement = boundsAware;
            while (current instanceof SChildElement) {
                if (current instanceof SRoutableElement) {
                    bounds = getAbsoluteRouteBounds(current);
                    break;
                } else {
                    const parent = current.parent;
                    bounds = parent.localToParent(bounds);
                    current = parent;
                }
            }
            return bounds;
        } else if (element instanceof SModelRoot) {
            const canvasBounds = element.canvasBounds;
            return { x: 0, y: 0, width: canvasBounds.width, height: canvasBounds.height };
        } else {
            return Bounds.EMPTY;
        }
    }

}

[martin-fleck-at]

@rileydanejohnston Great that it works for you! I would suggest to rebind the existing mouse listener instead of binding a new one, so:

bind(MyNewHoverMouseListener).toSelf().inSingletonScope();
rebind(GlspHoverMouseListener).toService(MyNewHoverMouseListener);

[rileydanejohnston]

Awesome, thank you @martin-fleck-at! Are you planning on doing a PR for this? I'm trying to figure out if these changes are going to be part of GLSP (so I could discard these files/changes) or if I need to keep these changes/files in my project.

[martin-fleck-at]

@rileydanejohnston I opened eclipse-glsp/glsp-client#221 to fix this issue with a custom GLSP type. If we keep this approach, you should use GEdge instead of SEdge in the future. Until the whole review process is through I'd suggest you keep the fix in your code but in the long run you should be able to remove it.

[rileydanejohnston]

Hi @martin-fleck-at, I just wanted to follow up to check what version of eclipse-glsp/glsp-client I would need to import the GEdge. My project is on v0.9.0 and I'm unable to import. I tried updating to 1.0.0 but it doesn't seem to work for me either. Thanks!

[martin-fleck-at]

@rileydanejohnston I think the safest option is to set it to the concrete version: 1.1.0-next.9fabd84.216.

Little background: So if you look at the commit history, you'll see that the merged fix has the commit 9fabd84... which is part of the NPM version. There is a handy tool that you can use to see which version declaration matches which NPM packages: https://semver.npmjs.com/ (using @eclipse-glsp/client as the package name). Due to our version naming it is not so easy to always get the right version, we will probably rework that a bit in the near future.

Just FYI: There will be a new release candidate released by the end of this week so you could also wait for that as well. Since you are switching from version 0.9.0, I'd expect some breaking changes either way.