211: Address review comments

- Use an options parameter for RequestCheckEdgeTargetAction.create() - Extract checkTarget() to its own method in EdgeCreationTool
eclipse-glsp · Sep 11, 2023 · 796399b · 796399b
1 parent b98c98b
commit 796399b
Show file tree

Hide file tree

Showing 2 changed files with 30 additions and 26 deletions.
diff --git a/packages/client/src/features/tools/edge-creation/edge-creation-tool.ts b/packages/client/src/features/tools/edge-creation/edge-creation-tool.ts
@@ -149,23 +149,7 @@ export class EdgeCreationToolMouseListener extends DragAwareMouseListener {
                 if (!this.isSourceSelected()) {
                     this.allowedTarget = this.isAllowedSource(newCurrentTarget);
                 } else if (!this.isTargetSelected()) {
-                    // Temporarily mark the target as invalid while we check for a proper result,
-                    // so a fast-clicking user doesn't get a chance to create the edge in the meantime.
-                    this.allowedTarget = false;
-                    const actions = this.isAllowedTarget(newCurrentTarget).then(allowedCheck => {
-                        // Make sure we didn't change the target element while
-                        // checking for valid target.
-                        if (allowedCheck.targetElement === this.currentTarget) {
-                            this.allowedTarget = allowedCheck.allowed;
-                            return this.updateEdgeFeedback();
-                        }
-                        // FIXME Is there a proper no-op action? We can't return <undefined> or [] here
-                        // because of the method signature.
-                        return <Action>{
-                            kind: 'no-op'
-                        };
-                    });
-                    return [actions];
+                    return this.checkTarget(newCurrentTarget);
                 }
                 return [this.updateEdgeFeedback()];
             }
@@ -174,6 +158,26 @@ export class EdgeCreationToolMouseListener extends DragAwareMouseListener {
         return [];
     }
 
+    protected checkTarget(newCurrentTarget: SModelElement | undefined): (Action | Promise<Action>)[] {
+        // Temporarily mark the target as invalid while we check for a proper result,
+        // so a fast-clicking user doesn't get a chance to create the edge in the meantime.
+        this.allowedTarget = false;
+        const actions = this.isAllowedTarget(newCurrentTarget).then(allowedCheck => {
+            // Make sure we didn't change the target element while
+            // checking for valid target.
+            if (allowedCheck.targetElement === this.currentTarget) {
+                this.allowedTarget = allowedCheck.allowed;
+                return this.updateEdgeFeedback();
+            }
+            // FIXME Is there a proper no-op action? We can't return <undefined> or [] here
+            // because of the method signature.
+            return <Action>{
+                kind: 'no-op'
+            };
+        });
+        return [actions];
+    }
+
     protected updateEdgeFeedback(): Action {
         if (this.allowedTarget) {
             const action = !this.isSourceSelected()
@@ -192,7 +196,7 @@ export class EdgeCreationToolMouseListener extends DragAwareMouseListener {
         let allowed = element !== undefined && isConnectable(element) && element.canConnect(this.proxyEdge, 'target');
         if (this.source && element && allowed && this.isDynamic(this.proxyEdge.type)) {
             const response = await this.actionDispatcher.request<CheckEdgeTargetResultAction>(
-                RequestCheckEdgeTargetAction.create(this.source, element, this.proxyEdge.type)
+                RequestCheckEdgeTargetAction.create({ sourceElement: this.source, targetElement: element, edgeTypeId: this.proxyEdge.type })
             );
             allowed = response.isValid;
         }

diff --git a/packages/protocol/src/action-protocol/element-type-hints.ts b/packages/protocol/src/action-protocol/element-type-hints.ts
@@ -189,16 +189,16 @@ export interface RequestCheckEdgeTargetAction extends RequestAction<CheckEdgeTar
 export namespace RequestCheckEdgeTargetAction {
     export const KIND = 'requestCheckEdgeTarget';
 
-    export function create(
-        sourceElement: SModelElement | string,
-        targetElement: SModelElement | string,
-        edgeTypeId: string
-    ): RequestCheckEdgeTargetAction {
+    export function create(options: {
+        sourceElement: SModelElement | string;
+        targetElement: SModelElement | string;
+        edgeTypeId: string;
+    }): RequestCheckEdgeTargetAction {
         return {
             kind: KIND,
-            edgeTypeId,
-            sourceElementId: getElementId(sourceElement),
-            targetElementId: getElementId(targetElement),
+            edgeTypeId: options.edgeTypeId,
+            sourceElementId: getElementId(options.sourceElement),
+            targetElementId: getElementId(options.targetElement),
             requestId: ''
         };
     }