Ensure we apply feedback actions already on SetModel (#322)

- Ensure we apply feedback actions already on SetModel - Avoid code duplication by moving function into feedback dispatcher - Use 'rank' instead of 'priority' for feedback commands - unify - ContributionProvider to replace @multiInject to better time lazy init Fixes eclipse-glsp/glsp#1239
eclipse-glsp · Mar 12, 2024 · 5e43503 · 5e43503
1 parent 645e72f
commit 5e43503
Show file tree

Hide file tree

Showing 14 changed files with 267 additions and 60 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -15,6 +15,10 @@
 -   [diagram] Restructure some tools to have a more common infrastructure and support helper lines [#306](https://github.com/eclipse-glsp/glsp-client/pull/306)
 -   [diagram] Fix a bug in `SelectionService` that caused issues with inversify when injecting certain services (e.g. `ActionDispatcher`) in `SelectionChangeListener` implementations [#305](https://github.com/eclipse-glsp/glsp-client/pull/305)
 -   [diagram] Ensure that the `SelectionService` does not trigger a change event if the selection did not change on model update [#313](https://github.com/eclipse-glsp/glsp-client/pull/313)
+-   [API] Apply feedback commands already on `SetModelCommand` and unify `rank` and `priority` property [#323](https://github.com/eclipse-glsp/glsp-client/pull/322).
+    -   Method `FeedbackAwareUpdateModelCommand.getFeedbackCommands` was move to `IFeedbackEmitter` for re-use, resulting in two new methods: `getFeedbackCommands` and `applyFeedbackCommands`.
+    -   Method `FeedbackAwareUpdateModelCommand.getPriority` was replaced by a generic `rank` property and the `Ranked` namespace.
+    -   The `priority` property (higher priority equals earlier execution) in `FeedbackCommand` was superseeded by a `rank` property (lower rank equals earlier execution).
 
 ## [v2.0.0 - 14/10/2023](https://github.com/eclipse-glsp/glsp-client/releases/tag/v2.0.0)
 

diff --git a/packages/client/src/base/action-handler-registry.ts b/packages/client/src/base/action-handler-registry.ts
@@ -14,17 +14,56 @@
  * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
  ********************************************************************************/
 
-import { injectable } from 'inversify';
-import { ActionHandlerRegistry } from '@eclipse-glsp/sprotty';
+import { ActionHandlerRegistration, ActionHandlerRegistry, IActionHandler, IActionHandlerInitializer, TYPES } from '@eclipse-glsp/sprotty';
+import { inject, injectable, named } from 'inversify';
+import { IContributionProvider } from '../utils/contribution-provider';
 
 @injectable()
 export class GLSPActionHandlerRegistry extends ActionHandlerRegistry {
+    @inject(TYPES.IContributionProvider)
+    @named(TYPES.ActionHandlerRegistration)
+    protected readonly registrations: IContributionProvider<ActionHandlerRegistration>;
+
+    @inject(TYPES.IContributionProvider)
+    @named(TYPES.IActionHandlerInitializer)
+    protected readonly initializers: IContributionProvider<IActionHandlerInitializer>;
+
+    protected initialized = false;
+
+    constructor() {
+        super([], []);
+    }
+
+    protected init(): void {
+        if (!this.initialized) {
+            this.initialized = true;
+            this.registrations.getContributions().forEach(registration => this.register(registration.actionKind, registration.factory()));
+            this.initializers.getContributions().forEach(initializer => this.initializeActionHandler(initializer));
+        }
+    }
+
+    override register(key: string, instance: IActionHandler): void {
+        this.init();
+        super.register(key, instance);
+    }
+
+    override get(key: string): IActionHandler[] {
+        this.init();
+        return super.get(key);
+    }
+
+    override initializeActionHandler(initializer: IActionHandlerInitializer): void {
+        this.init();
+        super.initializeActionHandler(initializer);
+    }
+
     /**
      * Retrieve a set of all action kinds for which (at least) one
      * handler is registered
      * @returns the set of handled action kinds
      */
     getHandledActionKinds(): string[] {
+        this.init();
         return Array.from(this.elements.keys());
     }
 }
diff --git a/packages/client/src/base/default.module.ts b/packages/client/src/base/default.module.ts
@@ -22,6 +22,7 @@ import {
     MoveCommand,
     SetDirtyStateAction,
     SetEditModeAction,
+    SetModelCommand,
     TYPES,
     bindAsService,
     bindOrRebind,
@@ -31,10 +32,12 @@ import {
 } from '@eclipse-glsp/sprotty';
 import '@vscode/codicons/dist/codicon.css';
 import '../../css/glsp-sprotty.css';
+import { bindContributionProvider } from '../utils/contribution-provider';
 import { GLSPActionDispatcher } from './action-dispatcher';
 import { GLSPActionHandlerRegistry } from './action-handler-registry';
 import { GLSPCommandStack } from './command-stack';
 import { EditorContextService } from './editor-context-service';
+import { FeedbackAwareSetModelCommand } from './feedback';
 import { ModifyCssFeedbackCommand } from './feedback/css-feedback';
 import { FeedbackActionDispatcher } from './feedback/feedback-action-dispatcher';
 import { FeedbackAwareUpdateModelCommand } from './feedback/update-model-command';
@@ -73,6 +76,7 @@ export const defaultModule = new FeatureModule((bind, unbind, isBound, rebind, .
     // Model update initialization ------------------------------------
     bind(TYPES.IFeedbackActionDispatcher).to(FeedbackActionDispatcher).inSingletonScope();
     configureCommand(context, FeedbackAwareUpdateModelCommand);
+    rebind(SetModelCommand).to(FeedbackAwareSetModelCommand);
 
     bind(GLSPMouseTool).toSelf().inSingletonScope();
     bindOrRebind(context, MouseTool).toService(GLSPMouseTool);
@@ -84,6 +88,9 @@ export const defaultModule = new FeatureModule((bind, unbind, isBound, rebind, .
     bindOrRebind(context, TYPES.ICommandStack).to(GLSPCommandStack).inSingletonScope();
     bind(GLSPActionDispatcher).toSelf().inSingletonScope();
     bindOrRebind(context, TYPES.IActionDispatcher).toService(GLSPActionDispatcher);
+
+    bindContributionProvider(bind, TYPES.ActionHandlerRegistration);
+    bindContributionProvider(bind, TYPES.IActionHandlerInitializer);
     bind(GLSPActionHandlerRegistry).toSelf().inSingletonScope();
     bindOrRebind(context, ActionHandlerRegistry).toService(GLSPActionHandlerRegistry);
 

diff --git a/packages/client/src/base/feedback/feedback-action-dispatcher.ts b/packages/client/src/base/feedback/feedback-action-dispatcher.ts
@@ -13,8 +13,21 @@
  *
  * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
  ********************************************************************************/
+import {
+    Action,
+    ActionHandlerRegistry,
+    Command,
+    CommandActionHandler,
+    CommandExecutionContext,
+    Disposable,
+    IActionDispatcher,
+    ICommand,
+    ILogger,
+    TYPES,
+    toTypeGuard
+} from '@eclipse-glsp/sprotty';
 import { inject, injectable } from 'inversify';
-import { Action, Disposable, IActionDispatcher, ILogger, TYPES } from '@eclipse-glsp/sprotty';
+import { getFeedbackRank } from './feedback-command';
 
 export interface IFeedbackEmitter {}
 
@@ -57,6 +70,16 @@ export interface IFeedbackActionDispatcher {
      * Retrieve all `actions` sent out by currently registered `feedbackEmitter`.
      */
     getRegisteredFeedback(): Action[];
+
+    /**
+     * Retrieves all commands based on the registered feedback actions, ordered by their rank (lowest rank first).
+     */
+    getFeedbackCommands(): Command[];
+
+    /**
+     * Applies all current feedback commands to the given command execution context.
+     */
+    applyFeedbackCommands(context: CommandExecutionContext): Promise<void>;
 }
 
 @injectable()
@@ -66,6 +89,8 @@ export class FeedbackActionDispatcher implements IFeedbackActionDispatcher {
     @inject(TYPES.IActionDispatcherProvider) protected actionDispatcher: () => Promise<IActionDispatcher>;
     @inject(TYPES.ILogger) protected logger: ILogger;
 
+    @inject(ActionHandlerRegistry) protected actionHandlerRegistry: ActionHandlerRegistry;
+
     registerFeedback(feedbackEmitter: IFeedbackEmitter, feedbackActions: Action[], cleanupActions?: Action[] | undefined): Disposable {
         if (feedbackActions.length > 0) {
             this.registeredFeedback.set(feedbackEmitter, feedbackActions);
@@ -97,6 +122,29 @@ export class FeedbackActionDispatcher implements IFeedbackActionDispatcher {
         return result;
     }
 
+    getFeedbackCommands(): Command[] {
+        return this.getRegisteredFeedback()
+            .flatMap(action => this.actionToCommands(action))
+            .sort((left, right) => getFeedbackRank(left) - getFeedbackRank(right));
+    }
+
+    async applyFeedbackCommands(context: CommandExecutionContext): Promise<void> {
+        const feedbackCommands = this.getFeedbackCommands() ?? [];
+        if (feedbackCommands?.length > 0) {
+            const results = feedbackCommands.map(command => command.execute(context));
+            await Promise.all(results);
+        }
+    }
+
+    protected actionToCommands(action: Action): ICommand[] {
+        return (
+            this.actionHandlerRegistry
+                ?.get(action.kind)
+                .filter(toTypeGuard(CommandActionHandler))
+                .map(handler => handler.handle(action)) ?? []
+        );
+    }
+
     protected async dispatchFeedback(actions: Action[], feedbackEmitter: IFeedbackEmitter): Promise<void> {
         try {
             const actionDispatcher = await this.actionDispatcher();

diff --git a/packages/client/src/base/feedback/feedback-command.ts b/packages/client/src/base/feedback/feedback-command.ts
@@ -13,11 +13,16 @@
  *
  * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
  ********************************************************************************/
-import { Command, CommandExecutionContext, CommandReturn } from '@eclipse-glsp/sprotty';
+/* eslint-disable deprecation/deprecation */
+import { Command, CommandExecutionContext, CommandReturn, ICommand } from '@eclipse-glsp/sprotty';
+import { Ranked } from '../ranked';
 
-export abstract class FeedbackCommand extends Command {
-    // used by the `FeedbackAwareUpdateModelCommand`
-    readonly priority: number = 0;
+export abstract class FeedbackCommand extends Command implements Ranked {
+    /** @deprecated Use rank instead. Please note that a lower rank implies higher priority, so the order is reversed. */
+    readonly priority?: number = 0;
+
+    // backwards compatibility: convert any existing priority to an equivalent rank
+    readonly rank: number = this.priority ? -this.priority : Ranked.DEFAULT_RANK;
 
     undo(context: CommandExecutionContext): CommandReturn {
         return context.root;
@@ -27,3 +32,9 @@ export abstract class FeedbackCommand extends Command {
         return context.root;
     }
 }
+
+/** Used for backwards compatibility, otherwise use Ranked.getRank or Ranked sort functions. */
+export function getFeedbackRank(command: ICommand): number {
+    const feedbackCommand = command as Partial<FeedbackCommand>;
+    return feedbackCommand?.priority ? -feedbackCommand.priority : feedbackCommand.rank ?? Ranked.DEFAULT_RANK;
+}
diff --git a/packages/client/src/base/feedback/index.ts b/packages/client/src/base/feedback/index.ts
@@ -16,4 +16,6 @@
 export * from './css-feedback';
 export * from './feedback-action-dispatcher';
 export * from './feedback-command';
+export * from './set-model-command';
 export * from './update-model-command';
+
diff --git a/packages/client/src/base/feedback/set-model-command.ts b/packages/client/src/base/feedback/set-model-command.ts
@@ -0,0 +1,45 @@
+/********************************************************************************
+ * Copyright (c) 2024 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 { CommandExecutionContext, GModelRoot, ILogger, SetModelAction, SetModelCommand, TYPES } from '@eclipse-glsp/sprotty';
+import { inject, injectable, optional } from 'inversify';
+import { IFeedbackActionDispatcher } from './feedback-action-dispatcher';
+
+@injectable()
+export class FeedbackAwareSetModelCommand extends SetModelCommand {
+    @inject(TYPES.ILogger)
+    protected logger: ILogger;
+
+    @inject(TYPES.IFeedbackActionDispatcher)
+    @optional()
+    protected feedbackActionDispatcher?: IFeedbackActionDispatcher;
+
+    constructor(@inject(TYPES.Action) action: SetModelAction) {
+        super(action);
+    }
+
+    override execute(context: CommandExecutionContext): GModelRoot {
+        const root = super.execute(context);
+        this.applyFeedback(root, context);
+        return root;
+    }
+
+    protected applyFeedback(newRoot: GModelRoot, context: CommandExecutionContext): void {
+        // Create a temporary context which defines the `newRoot` as `root`
+        // This way we do not corrupt the redo/undo behavior of the super class
+        const tempContext: CommandExecutionContext = { ...context, root: newRoot };
+        this.feedbackActionDispatcher?.applyFeedbackCommands(tempContext);
+    }
+}
diff --git a/packages/client/src/base/feedback/update-model-command.ts b/packages/client/src/base/feedback/update-model-command.ts
@@ -13,25 +13,20 @@
  *
  * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
  ********************************************************************************/
-import { inject, injectable, optional } from 'inversify';
 import {
-    ActionHandlerRegistry,
     Animation,
-    Command,
-    CommandActionHandler,
     CommandExecutionContext,
     CommandReturn,
+    GModelRoot,
     ILogger,
     MorphEdgesAnimation,
-    GModelRoot,
     TYPES,
     UpdateAnimationData,
     UpdateModelAction,
-    UpdateModelCommand,
-    toTypeGuard
+    UpdateModelCommand
 } from '@eclipse-glsp/sprotty';
+import { inject, injectable, optional } from 'inversify';
 import { IFeedbackActionDispatcher } from './feedback-action-dispatcher';
-import { FeedbackCommand } from './feedback-command';
 
 /**
  * A special {@link UpdateModelCommand} that retrieves all registered {@link Action}s from the {@link IFeedbackActionDispatcher}
@@ -47,50 +42,18 @@ export class FeedbackAwareUpdateModelCommand extends UpdateModelCommand {
     @optional()
     protected feedbackActionDispatcher: IFeedbackActionDispatcher;
 
-    protected actionHandlerRegistry?: ActionHandlerRegistry;
-
-    constructor(
-        @inject(TYPES.Action) action: UpdateModelAction,
-        @inject(TYPES.ActionHandlerRegistryProvider)
-        actionHandlerRegistryProvider: () => Promise<ActionHandlerRegistry>
-    ) {
+    constructor(@inject(TYPES.Action) action: UpdateModelAction) {
         super({ animate: true, ...action });
-        actionHandlerRegistryProvider().then(registry => (this.actionHandlerRegistry = registry));
     }
 
     protected override performUpdate(oldRoot: GModelRoot, newRoot: GModelRoot, context: CommandExecutionContext): CommandReturn {
-        if (this.feedbackActionDispatcher && this.actionHandlerRegistry) {
-            // Create a temporary context which defines the `newRoot` as `root`
-            // This way we do not corrupt the redo/undo behavior of the super class
-            const tempContext: CommandExecutionContext = {
-                ...context,
-                root: newRoot
-            };
-
-            const feedbackCommands = this.getFeedbackCommands(this.actionHandlerRegistry);
-            feedbackCommands.forEach(command => command.execute(tempContext));
-        }
-
+        // Create a temporary context which defines the `newRoot` as `root`
+        // This way we do not corrupt the redo/undo behavior of the super class
+        const tempContext: CommandExecutionContext = { ...context, root: newRoot };
+        this.feedbackActionDispatcher?.applyFeedbackCommands(tempContext);
         return super.performUpdate(oldRoot, newRoot, context);
     }
 
-    protected getFeedbackCommands(registry: ActionHandlerRegistry): Command[] {
-        const result: Command[] = [];
-        this.feedbackActionDispatcher.getRegisteredFeedback().forEach(action => {
-            const commands = registry
-                .get(action.kind)
-                .filter<CommandActionHandler>(toTypeGuard(CommandActionHandler))
-                .map(handler => handler.handle(action));
-            result.push(...commands);
-        });
-        // sort commands descanting by priority
-        return result.sort((a, b) => this.getPriority(b) - this.getPriority(a));
-    }
-
-    protected getPriority(command: Partial<FeedbackCommand>): number {
-        return command.priority ?? 0;
-    }
-
     // Override the `createAnimations` implementation and remove the animation for edge morphing. Otherwise routing & reconnect
     // handles flicker after each server update.
     protected override createAnimations(data: UpdateAnimationData, root: GModelRoot, context: CommandExecutionContext): Animation[] {

diff --git a/packages/client/src/base/ranked.ts b/packages/client/src/base/ranked.ts
@@ -21,11 +21,16 @@ import { AnyObject, hasNumberProp } from '@eclipse-glsp/sprotty';
  * orderable by a type or rank/priority.
  */
 export interface Ranked {
+    /**
+     * A rank implies the position of this element within a sequence of other ranked elements.
+     * A lower rank implies a position earlier in the list.
+     */
     rank: number;
 }
 
 export namespace Ranked {
     export const DEFAULT_RANK = 0;
+
     export function is(object: unknown): object is Ranked {
         return AnyObject.is(object) && hasNumberProp(object, 'rank');
     }
@@ -39,4 +44,13 @@ export namespace Ranked {
     export function getRank(object: unknown): number {
         return is(object) ? object.rank : DEFAULT_RANK;
     }
+
+    /** Sort function for lowest rank first. */
+    export const sortAsc = (left: unknown, right: unknown): number => getRank(left) - getRank(right);
+
+    /** Sort function for highest rank first. */
+    export const sortDesc = (left: unknown, right: unknown): number => getRank(right) - getRank(left);
+
+    /** Default sort function for rank: Lowest rank first */
+    export const sort = sortAsc;
 }