GLSP-1116: Folllow-up fixes (#288)

* GLSP-1116: Folllow-up fixes

- Refactor diagramLoader+modelsource: Avoid concurrency/timing issues between diagramloader & modelsource. In some cases the loader did send actions before the model source was fully configured. Now the loader explictly configures the model source before dispatching the model request actions.
- Ensure that client session is disposed if `GlspModelSource` is disposed
- Ensure that the `dispatch` method correctly awaits the super result
- Fix typing of `requstUntil`. This method can actually also return undefined if the timeout was reached 
  and `rejectOnTimeout` is false + adapt affected code
- Refactor `handleAction` method to allow dispatching of received responses 
- Rename that contains `GLSPContextMenuMouseListener` to reflect class name
- Refactor `IContextMenuServiceProvider` to return an no-op implementation if no `IContextMenuService` is bound instead of rejecting the promise
- Fix typing for Request actions (see eclipse-sprotty/sprotty#378)
- Add test cases

- Remove deprecated code
- Fix return type of `ExportSvgAction.is`

Follow up for eclipse-glsp/glsp/issues/1116

* Add early-exit condition to `FeedbackActionDispatcher` 

Add early-exit condition to `FeedbackActionDispatcher` to avoid unnecesary feedback dispatching (and logging) when the actions array is empty

packages/client/src/base/action-dispatcher.spec.ts

@@ -0,0 +1,87 @@
+/********************************************************************************
+ * Copyright (c) 2023 EclipseSource and others.
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Public License v. 2.0 which is available at
+ * http://www.eclipse.org/legal/epl-2.0.
+ *
+ * This Source Code may also be made available under the following Secondary
+ * Licenses when the conditions for such availability set forth in the Eclipse
+ * Public License v. 2.0 are satisfied: GNU General Public License, version 2
+ * with the GNU Classpath Exception which is available at
+ * https://www.gnu.org/software/classpath/license.html.
+ *
+ * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
+ ********************************************************************************/
+import { expect } from 'chai';
+import { Container } from 'inversify';
+import { ActionHandlerRegistry, IActionHandler, RequestAction, ResponseAction, TYPES } from '~glsp-sprotty';
+import { IDiagramOptions } from '..';
+import { GLSPActionDispatcher } from './action-dispatcher';
+import { defaultModule } from './default.module';
+
+const container = new Container();
+container.load(defaultModule);
+container.bind(TYPES.IDiagramOptions).toConstantValue(<IDiagramOptions>(<unknown>{
+    clientId: 'client1',
+    diagramType: 'diagramType',
+    glspClientProvider: async () => ({} as any)
+}));
+const registry = container.get(ActionHandlerRegistry);
+const actionDispatcher = container.get(GLSPActionDispatcher);
+
+let testHandlerDelay = 0;
+const testHandler: IActionHandler = {
+    handle: action => {
+        const request = action as RequestAction<ResponseAction>;
+        new Promise(resolve => setTimeout(resolve, testHandlerDelay)).then(() =>
+            actionDispatcher.dispatch(<ResponseAction>{
+                kind: 'response',
+                responseId: request.requestId
+            })
+        );
+    }
+};
+registry.register('request', testHandler);
+// eslint-disable-next-line @typescript-eslint/no-empty-function
+registry.register('response', { handle: () => {} });
+actionDispatcher.initialize().then(() => {
+    actionDispatcher['blockUntil'] = undefined;
+});
+
+describe('GLSPActionDispatcher', () => {
+    describe('requestUntil', () => {
+        it('should resolve successfully if response dispatched within timeout', async () => {
+            testHandlerDelay = 15;
+            const requestAction = { kind: 'request', requestId: '' };
+            const response = await actionDispatcher.requestUntil(requestAction, 150);
+            expect(response?.responseId).to.be.equal(requestAction.requestId);
+        });
+        it('should resolve to `undefined` if no response dispatched within timeout & `rejectOnTimeout` flag is false', async () => {
+            testHandlerDelay = 30;
+            const requestAction = { kind: 'request', requestId: '' };
+            const response = await actionDispatcher.requestUntil(requestAction, 5);
+            expect(response).to.be.undefined;
+        });
+        it('should be rejected if no response dispatched within timeout & `rejectOnTimeout` flag is true', async () => {
+            testHandlerDelay = 30;
+            const requestAction = { kind: 'request', requestId: '' };
+            const gotRejected = await actionDispatcher.requestUntil(requestAction, 5, true).then(
+                () => false,
+                () => true
+            );
+            expect(gotRejected, 'Response promise should be rejected').to.be.true;
+        });
+    });
+    describe('request & re-dispatch', () => {
+        it('should be possible to re-dispatch the response of a `request` call', async () => {
+            const requestAction = { kind: 'request', requestId: '' };
+            const response = await actionDispatcher.request(requestAction);
+            const dispatchSuccessful = await actionDispatcher.dispatch(response).then(
+                () => true,
+                err => false
+            );
+            expect(dispatchSuccessful, 'Promise of re-dispatch should resolve successfully').to.be.true;
+        });
+    });
+});

packages/client/src/base/action-dispatcher.ts

@@ -54,8 +54,8 @@ export class GLSPActionDispatcher extends ActionDispatcher {
         this.initializationConstraint.onInitialized(() => this.dispatchAll(actions));
     }
 
-    override dispatch(action: Action): Promise<void> {
-        const result = super.dispatch(action);
+    override async dispatch(action: Action): Promise<void> {
+        const result = await super.dispatch(action);
         this.initializationConstraint.notifyDispatched(action);
         return result;
     }
@@ -69,11 +69,11 @@ export class GLSPActionDispatcher extends ActionDispatcher {
                 this.timeouts.delete(action.responseId);
             }
 
-            // we might have reached a timeout, so we simply drop the response
+            // Check if we have a pending request for the response.
+            // If not the  we clear the responseId => action will be dispatched normally
             const deferred = this.requests.get(action.responseId);
             if (deferred === undefined) {
-                this.logger.log(this, 'No matching request for response', action);
-                return Promise.resolve();
+                action.responseId = '';
             }
         }
         return super.handleAction(action);
@@ -97,7 +97,11 @@ export class GLSPActionDispatcher extends ActionDispatcher {
      * If `rejectOnTimeout` is set to false (default) the returned promise will be resolved with
      * no value, otherwise it will be rejected.
      */
-    requestUntil<Res extends ResponseAction>(action: RequestAction<Res>, timeoutMs = 2000, rejectOnTimeout = false): Promise<Res> {
+    requestUntil<Res extends ResponseAction>(
+        action: RequestAction<Res>,
+        timeoutMs = 2000,
+        rejectOnTimeout = false
+    ): Promise<Res | undefined> {
         if (!action.requestId && action.requestId === '') {
             // No request id has been specified. So we use a generated one.
             action.requestId = RequestAction.generateRequestId();
@@ -122,6 +126,6 @@ export class GLSPActionDispatcher extends ActionDispatcher {
         }, timeoutMs);
         this.timeouts.set(requestId, timeout);
 
-        return super.request(action);
+        return super.request<Res>(action);
     }
 }

packages/client/src/base/default.module.ts

@@ -62,16 +62,7 @@ export const defaultModule = new FeatureModule((bind, unbind, isBound, rebind, .
     const context = { bind, unbind, isBound, rebind };
 
     bind(EditorContextService).toSelf().inSingletonScope();
-    bind(TYPES.IEditorContextServiceProvider).toProvider<EditorContextService>(
-        ctx => () =>
-            new Promise<EditorContextService>((resolve, reject) => {
-                if (ctx.container.isBound(EditorContextService)) {
-                    resolve(ctx.container.get<EditorContextService>(EditorContextService));
-                } else {
-                    reject();
-                }
-            })
-    );
+    bind(TYPES.IEditorContextServiceProvider).toProvider<EditorContextService>(ctx => async () => ctx.container.get(EditorContextService));
 
     configureActionHandler(context, SetEditModeAction.KIND, EditorContextService);
     configureActionHandler(context, SetDirtyStateAction.KIND, EditorContextService);

packages/client/src/base/feedback/feedback-action-dispatcher.ts

@@ -67,14 +67,16 @@ export class FeedbackActionDispatcher implements IFeedbackActionDispatcher {
     @inject(TYPES.ILogger) protected logger: ILogger;
 
     registerFeedback(feedbackEmitter: IFeedbackEmitter, feedbackActions: Action[], cleanupActions?: Action[] | undefined): Disposable {
-        this.registeredFeedback.set(feedbackEmitter, feedbackActions);
-        this.dispatchFeedback(feedbackActions, feedbackEmitter);
+        if (feedbackActions.length > 0) {
+            this.registeredFeedback.set(feedbackEmitter, feedbackActions);
+            this.dispatchFeedback(feedbackActions, feedbackEmitter);
+        }
         return Disposable.create(() => this.deregisterFeedback(feedbackEmitter, cleanupActions));
     }
 
     deregisterFeedback(feedbackEmitter: IFeedbackEmitter, cleanupActions?: Action[] | undefined): void {
         this.registeredFeedback.delete(feedbackEmitter);
-        if (cleanupActions) {
+        if (cleanupActions && cleanupActions.length > 0) {
             this.dispatchFeedback(cleanupActions, feedbackEmitter);
         }
     }

packages/client/src/base/model/diagram-loader.ts

@@ -49,6 +49,7 @@ export interface IDiagramOptions {
     diagramType: string;
     /**
      * The provider function to retrieve the GLSP client used by this diagram to communicate with the server.
+     * Multiple invocations of the provder function should always return the same `GLSPClient` instance.
      */
     glspClientProvider: () => Promise<GLSPClient>;
     /**
@@ -172,8 +173,7 @@ export class DiagramLoader {
             },
             enableNotifications: options.enableNotifications ?? true
         };
-        await this.actionDispatcher.initialize();
-        // Set placeholder model until real model from server is available
+        // Set empty place holder model until actual model from server is available
         await this.actionDispatcher.dispatch(SetModelAction.create(EMPTY_ROOT));
         await this.invokeStartupHook('preInitialize');
         await this.initialize(resolvedOptions);
@@ -190,21 +190,23 @@ export class DiagramLoader {
     }
 
     protected async requestModel(options: ResolvedDiagramLoadingOptions): Promise<void> {
-        return this.actionDispatcher.dispatch(RequestModelAction.create({ options: options.requestModelOptions }));
+        const response = await this.actionDispatcher.request<SetModelAction>(
+            RequestModelAction.create({ options: options.requestModelOptions })
+        );
+        return this.actionDispatcher.dispatch(response);
     }
 
     protected async initialize(options: ResolvedDiagramLoadingOptions): Promise<void> {
         if (options.enableNotifications) {
-            this.actionDispatcher.dispatch(StatusAction.create('Initializing...', { severity: 'INFO' }));
+            await this.actionDispatcher.dispatch(StatusAction.create('Initializing...', { severity: 'INFO' }));
         }
 
         const glspClient = await this.options.glspClientProvider();
-
         await glspClient.start();
-
         if (!glspClient.initializeResult) {
             await glspClient.initializeServer(options.initializeParameters);
         }
+        this.modelSource.configure(glspClient);
 
         if (options.enableNotifications) {
             this.actionDispatcher.dispatch(StatusAction.create('', { severity: 'NONE' }));

packages/client/src/base/model/glsp-model-source.ts

@@ -14,14 +14,16 @@
  * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
  ********************************************************************************/
 
-import { inject, injectable, postConstruct, preDestroy } from 'inversify';
+import { inject, injectable, preDestroy } from 'inversify';
 import {
     Action,
     ActionMessage,
     Disposable,
     DisposableCollection,
+    DisposeClientSessionParameters,
     GLSPClient,
     ILogger,
+    InitializeClientSessionParameters,
     InitializeResult,
     ModelSource,
     SModelRootSchema,
@@ -71,8 +73,9 @@ export class GLSPModelSource extends ModelSource implements Disposable {
     protected toDispose = new DisposableCollection();
     clientId: string;
 
-    protected _glspClient: GLSPClient | undefined;
     protected _currentRoot: SModelRootSchema;
+    protected registry: GLSPActionHandlerRegistry;
+    protected glspClient?: GLSPClient;
 
     get diagramType(): string {
         return this.options.diagramType;
@@ -82,32 +85,52 @@ export class GLSPModelSource extends ModelSource implements Disposable {
         return this.options.sourceUri;
     }
 
-    get glspClient(): GLSPClient | undefined {
-        return this._glspClient;
-    }
+    /**
+     * Configure forwarding of server-handled actions to the given {@link GLSPClient} and
+     * handling of action received from the `GLSPClient` (i.e. server). It is expected that the
+     * given GLSP client has already been initialized.
+     * @param glspClient The GLSP  to use.
+     * @throws An error if the given `GLSPClient` has not been initialized yet or if the set of server handled
+     *         action kinds could not be derived from the initialize result
+     */
+    configure(glspClient: GLSPClient): Promise<void> {
+        this.glspClient = glspClient;
+        if (!glspClient.initializeResult) {
+            throw new Error('Could not configure model source. The GLSP client is not initialized yet!');
+        }
 
-    @postConstruct()
-    protected postConstruct(): void {
-        this.clientId = this.options.clientId ?? this.viewerOptions.baseDiv;
-    }
+        const initializeParams = this.createInitializeClientSessionParameters(glspClient.initializeResult);
+        this.configureServeActions(glspClient.initializeResult);
 
-    configure(registry: GLSPActionHandlerRegistry, initializeResult: InitializeResult): Promise<void> {
-        const serverActions = initializeResult.serverActions[this.diagramType];
-        if (!serverActions || serverActions.length === 0) {
-            throw new Error(`No server-handled actions could be derived from the initialize result for diagramType: ${this.diagramType}!`);
-        }
-        // Retrieve all currently handled action kinds. We do this before registering the server actions
-        // to ensure that the array will only contain client-side handled actions
-        const clientActionKinds = registry.getHandledActionKinds();
+        this.toDispose.push(
+            glspClient.onActionMessage(message => this.messageReceived(message), this.clientId),
+            Disposable.create(() => glspClient.disposeClientSession(this.createDisposeClientSessionParameters()))
+        );
 
-        serverActions.forEach(action => registry.register(action, this));
-        this.toDispose.push(this.glspClient!.onActionMessage(message => this.messageReceived(message), this.clientId));
+        return glspClient!.initializeClientSession(initializeParams);
+    }
 
-        return this.glspClient!.initializeClientSession({
+    protected createInitializeClientSessionParameters(_initializeResult: InitializeResult): InitializeClientSessionParameters {
+        const clientActionKinds = this.registry.getHandledActionKinds();
+        return {
             clientSessionId: this.clientId,
             clientActionKinds,
             diagramType: this.diagramType
-        });
+        };
+    }
+
+    protected createDisposeClientSessionParameters(): DisposeClientSessionParameters {
+        return {
+            clientSessionId: this.clientId
+        };
+    }
+
+    protected configureServeActions(initializeResult: InitializeResult): void {
+        const serverActions = initializeResult.serverActions[this.diagramType];
+        if (serverActions?.length === 0) {
+            throw new Error(`No server-handled actions could be derived from the initialize result for diagramType: ${this.diagramType}!`);
+        }
+        serverActions.forEach(action => this.registry.register(action, this));
     }
 
     protected messageReceived(message: ActionMessage): void {
@@ -124,20 +147,10 @@ export class GLSPModelSource extends ModelSource implements Disposable {
         // Registering actions here is discouraged and it's recommended
         // to implemented dedicated action handlers.
         if (!this.clientId) {
-            this.clientId = this.viewerOptions.baseDiv;
+            this.clientId = this.options.clientId ?? this.viewerOptions.baseDiv;
         }
 
-        this.options.glspClientProvider().then(glspClient => {
-            this._glspClient = glspClient;
-            if (glspClient.initializeResult) {
-                this.configure(registry, glspClient.initializeResult);
-            } else {
-                const initializeListener = glspClient.onServerInitialized(result => {
-                    this.configure(registry, result);
-                    initializeListener.dispose();
-                });
-            }
-        });
+        this.registry = registry;
     }
 
     handle(action: Action): void {

packages/client/src/base/selection-service.spec.ts

@@ -16,7 +16,7 @@
 import { AssertionError, expect } from 'chai';
 import { Container, injectable } from 'inversify';
 import * as sinon from 'sinon';
-import { Action, Disposable, SGraphFactory, SModelElementSchema, SModelRoot, TYPES, initializeContainer } from '~glsp-sprotty';
+import { Action, Disposable, SGraphFactory, SModelRoot, SNode, TYPES, initializeContainer } from '~glsp-sprotty';
 import { defaultModule } from './default.module';
 import { IFeedbackActionDispatcher, IFeedbackEmitter } from './feedback/feedback-action-dispatcher';
 import { ISelectionListener, SelectFeedbackAction, SelectionService } from './selection-service';
@@ -62,15 +62,13 @@ function createContainer(): Container {
 
 describe('SelectionService', () => {
     // eslint-disable-next-line deprecation/deprecation
-    let graphFactory: SGraphFactory;
     let root: SModelRoot;
     let selectionService: SelectionService;
     let feedbackDispatcher: MockFeedbackActionDispatcher;
 
     beforeEach(() => {
         const container = createContainer();
         // eslint-disable-next-line deprecation/deprecation
-        graphFactory = container.get<SGraphFactory>(TYPES.IModelFactory);
         root = createRoot('node1', 'node2', 'node3', 'node4', 'node5');
         selectionService = container.get<SelectionService>(SelectionService);
         feedbackDispatcher = container.get<MockFeedbackActionDispatcher>(TYPES.IFeedbackActionDispatcher);
@@ -243,13 +241,16 @@ describe('SelectionService', () => {
     });
 
     function createRoot(...nodes: string[]): SModelRoot {
-        const children: SModelElementSchema[] = [];
-        nodes.forEach(node => children.push({ id: node, type: 'node:circle' }));
-        return graphFactory.createRoot({
-            id: 'selection-service-spec',
-            type: 'graph',
-            children: children
+        const model = new SModelRoot();
+        model.id = 'selection-service-spec';
+        model.type = 'graph';
+        nodes.forEach(id => {
+            const node = new SNode();
+            node.type = 'node:circle';
+            node.id = id;
+            model.add(node);
         });
+        return model;
     }
 
     function assertSelectionAndFeedback(expectedSelection: string[], expectedDeselection: string[]): void {