Merge pull request #3439 from github/robertbrignull/modeling-mode

Refactor events to fix the method modeling panel getting stuck showing "start modeling"

extensions/ql-vscode/src/common/interface-types.ts

@@ -698,16 +698,15 @@ interface SetMethodModelingPanelViewStateMessage {
   viewState: MethodModelingPanelViewState;
 }
 
-interface SetMethodMessage {
-  t: "setMethod";
-  method: Method | undefined;
-}
-
 interface SetMethodModifiedMessage {
   t: "setMethodModified";
   isModified: boolean;
 }
 
+interface SetNoMethodSelectedMessage {
+  t: "setNoMethodSelected";
+}
+
 interface SetSelectedMethodMessage {
   t: "setSelectedMethod";
   method: Method;
@@ -719,9 +718,9 @@ interface SetSelectedMethodMessage {
 
 export type ToMethodModelingMessage =
   | SetMethodModelingPanelViewStateMessage
-  | SetMethodMessage
   | SetMultipleModeledMethodsMessage
   | SetMethodModifiedMessage
+  | SetNoMethodSelectedMessage
   | SetSelectedMethodMessage
   | SetInModelingModeMessage
   | SetInProgressMessage

extensions/ql-vscode/src/model-editor/method-modeling/method-modeling-panel.ts

@@ -2,10 +2,8 @@ import { window } from "vscode";
 import type { App } from "../../common/app";
 import { DisposableObject } from "../../common/disposable-object";
 import { MethodModelingViewProvider } from "./method-modeling-view-provider";
-import type { Method } from "../method";
 import type { ModelingStore } from "../modeling-store";
 import { ModelConfigListener } from "../../config";
-import type { DatabaseItem } from "../../databases/local-databases";
 import type { ModelingEvents } from "../modeling-events";
 
 export class MethodModelingPanel extends DisposableObject {
@@ -36,11 +34,4 @@ export class MethodModelingPanel extends DisposableObject {
       ),
     );
   }
-
-  public async setMethod(
-    databaseItem: DatabaseItem,
-    method: Method,
-  ): Promise<void> {
-    await this.provider.setMethod(databaseItem, method);
-  }
 }

extensions/ql-vscode/src/model-editor/method-modeling/method-modeling-view-provider.ts

@@ -16,6 +16,7 @@ import type { ModelingEvents } from "../modeling-events";
 import type { QueryLanguage } from "../../common/query-language";
 import { tryGetQueryLanguage } from "../../common/query-language";
 import { createModelConfig } from "../languages";
+import type { ModeledMethod } from "../modeled-method";
 
 export class MethodModelingViewProvider extends AbstractWebviewViewProvider<
   ToMethodModelingMessage,
@@ -37,7 +38,7 @@ export class MethodModelingViewProvider extends AbstractWebviewViewProvider<
   }
 
   protected override async onWebViewLoaded(): Promise<void> {
-    await Promise.all([this.setViewState(), this.setInitialState()]);
+    await this.setInitialState();
     this.registerToModelingEvents();
     this.registerToModelConfigEvents();
   }
@@ -52,46 +53,60 @@ export class MethodModelingViewProvider extends AbstractWebviewViewProvider<
     });
   }
 
-  public async setMethod(
-    databaseItem: DatabaseItem | undefined,
-    method: Method | undefined,
+  private async setDatabaseItem(databaseItem: DatabaseItem): Promise<void> {
+    this.databaseItem = databaseItem;
+
+    await this.postMessage({
+      t: "setInModelingMode",
+      inModelingMode: true,
+    });
+
+    this.language = tryGetQueryLanguage(databaseItem.language);
+    await this.setViewState();
+  }
+
+  private async setSelectedMethod(
+    databaseItem: DatabaseItem,
+    method: Method,
+    modeledMethods: readonly ModeledMethod[],
+    isModified: boolean,
+    isInProgress: boolean,
+    processedByAutoModel: boolean,
   ): Promise<void> {
     this.method = method;
     this.databaseItem = databaseItem;
-    this.language = databaseItem && tryGetQueryLanguage(databaseItem.language);
+    this.language = tryGetQueryLanguage(databaseItem.language);
 
-    if (this.isShowingView) {
-      await this.postMessage({
-        t: "setMethod",
-        method,
-      });
-    }
+    await this.postMessage({
+      t: "setSelectedMethod",
+      method,
+      modeledMethods,
+      isModified,
+      isInProgress,
+      processedByAutoModel,
+    });
   }
 
   private async setInitialState(): Promise<void> {
-    if (this.modelingStore.hasStateForActiveDb()) {
-      const selectedMethod = this.modelingStore.getSelectedMethodDetails();
-      if (selectedMethod) {
-        this.databaseItem = selectedMethod.databaseItem;
-        this.language = tryGetQueryLanguage(
-          selectedMethod.databaseItem.language,
-        );
-        this.method = selectedMethod.method;
+    await this.setViewState();
 
-        await this.postMessage({
-          t: "setSelectedMethod",
-          method: selectedMethod.method,
-          modeledMethods: selectedMethod.modeledMethods,
-          isModified: selectedMethod.isModified,
-          isInProgress: selectedMethod.isInProgress,
-          processedByAutoModel: selectedMethod.processedByAutoModel,
-        });
-      }
+    const stateForActiveDb = this.modelingStore.getStateForActiveDb();
+    if (!stateForActiveDb) {
+      return;
+    }
 
-      await this.postMessage({
-        t: "setInModelingMode",
-        inModelingMode: true,
-      });
+    await this.setDatabaseItem(stateForActiveDb.databaseItem);
+
+    const selectedMethod = this.modelingStore.getSelectedMethodDetails();
+    if (selectedMethod) {
+      await this.setSelectedMethod(
+        stateForActiveDb.databaseItem,
+        selectedMethod.method,
+        selectedMethod.modeledMethods,
+        selectedMethod.isModified,
+        selectedMethod.isInProgress,
+        selectedMethod.processedByAutoModel,
+      );
     }
   }
 
@@ -183,33 +198,21 @@ export class MethodModelingViewProvider extends AbstractWebviewViewProvider<
     this.push(
       this.modelingEvents.onSelectedMethodChanged(async (e) => {
         if (this.webviewView) {
-          this.method = e.method;
-          this.databaseItem = e.databaseItem;
-          this.language = tryGetQueryLanguage(e.databaseItem.language);
-
-          await this.postMessage({
-            t: "setSelectedMethod",
-            method: e.method,
-            modeledMethods: e.modeledMethods,
-            isModified: e.isModified,
-            isInProgress: e.isInProgress,
-            processedByAutoModel: e.processedByAutoModel,
-          });
+          await this.setSelectedMethod(
+            e.databaseItem,
+            e.method,
+            e.modeledMethods,
+            e.isModified,
+            e.isInProgress,
+            e.processedByAutoModel,
+          );
         }
       }),
     );
 
     this.push(
       this.modelingEvents.onDbOpened(async (databaseItem) => {
-        this.databaseItem = databaseItem;
-
-        await this.postMessage({
-          t: "setInModelingMode",
-          inModelingMode: true,
-        });
-
-        this.language = tryGetQueryLanguage(databaseItem.language);
-        await this.setViewState();
+        await this.setDatabaseItem(databaseItem);
       }),
     );
 
@@ -223,7 +226,9 @@ export class MethodModelingViewProvider extends AbstractWebviewViewProvider<
         }
 
         if (dbUri === this.databaseItem?.databaseUri.toString()) {
-          await this.setMethod(undefined, undefined);
+          await this.postMessage({
+            t: "setNoMethodSelected",
+          });
         }
       }),
     );

extensions/ql-vscode/src/model-editor/methods-usage/methods-usage-panel.ts

@@ -57,7 +57,7 @@ export class MethodsUsagePanel extends DisposableObject {
     };
   }
 
-  public async revealItem(
+  private async revealItem(
     methodSignature: string,
     usage: Usage,
   ): Promise<void> {
@@ -108,6 +108,12 @@ export class MethodsUsagePanel extends DisposableObject {
         }
       }),
     );
+
+    this.push(
+      this.modelingEvents.onSelectedMethodChanged(async (event) => {
+        await this.revealItem(event.method.signature, event.usage);
+      }),
+    );
   }
 
   private async handleStateChangeEvent(): Promise<void> {

extensions/ql-vscode/src/model-editor/model-editor-module.ts

@@ -37,8 +37,6 @@ export class ModelEditorModule extends DisposableObject {
   private readonly queryStorageDir: string;
   private readonly modelingStore: ModelingStore;
   private readonly modelingEvents: ModelingEvents;
-  private readonly methodsUsagePanel: MethodsUsagePanel;
-  private readonly methodModelingPanel: MethodModelingPanel;
   private readonly modelConfig: ModelConfigListener;
 
   private constructor(
@@ -53,10 +51,10 @@ export class ModelEditorModule extends DisposableObject {
     this.queryStorageDir = join(baseQueryStorageDir, "model-editor-results");
     this.modelingEvents = new ModelingEvents(app);
     this.modelingStore = new ModelingStore(this.modelingEvents);
-    this.methodsUsagePanel = this.push(
+    this.push(
       new MethodsUsagePanel(this.modelingStore, this.modelingEvents, cliServer),
     );
-    this.methodModelingPanel = this.push(
+    this.push(
       new MethodModelingPanel(app, this.modelingStore, this.modelingEvents),
     );
     this.modelConfig = this.push(new ModelConfigListener());
@@ -107,7 +105,11 @@ export class ModelEditorModule extends DisposableObject {
   private registerToModelingEvents(): void {
     this.push(
       this.modelingEvents.onSelectedMethodChanged(async (event) => {
-        await this.showMethod(event.databaseItem, event.method, event.usage);
+        await showResolvableLocation(
+          event.usage.url,
+          event.databaseItem,
+          this.app.logger,
+        );
       }),
     );
 
@@ -126,16 +128,6 @@ export class ModelEditorModule extends DisposableObject {
     );
   }
 
-  private async showMethod(
-    databaseItem: DatabaseItem,
-    method: Method,
-    usage: Usage,
-  ): Promise<void> {
-    await this.methodsUsagePanel.revealItem(method.signature, usage);
-    await this.methodModelingPanel.setMethod(databaseItem, method);
-    await showResolvableLocation(usage.url, databaseItem, this.app.logger);
-  }
-
   private async openModelEditor(): Promise<void> {
     {
       const db = this.databaseManager.currentDatabaseItem;

extensions/ql-vscode/src/view/method-modeling/MethodModelingView.tsx

@@ -53,15 +53,19 @@ export function MethodModelingView({
           case "setInModelingMode":
             setInModelingMode(msg.inModelingMode);
             break;
-          case "setMethod":
-            setMethod(msg.method);
-            break;
           case "setMultipleModeledMethods":
             setModeledMethods(msg.modeledMethods);
             break;
           case "setMethodModified":
             setIsMethodModified(msg.isModified);
             break;
+          case "setNoMethodSelected":
+            setMethod(undefined);
+            setModeledMethods([]);
+            setIsMethodModified(false);
+            setIsModelingInProgress(false);
+            setIsProcessedByAutoModel(false);
+            break;
           case "setSelectedMethod":
             setMethod(msg.method);
             setModeledMethods(msg.modeledMethods);

extensions/ql-vscode/test/__mocks__/model-editor/modelingEventsMock.ts

@@ -4,6 +4,7 @@ import type { ModelingEvents } from "../../../src/model-editor/modeling-events";
 export function createMockModelingEvents({
   onActiveDbChanged = jest.fn(),
   onDbClosed = jest.fn(),
+  onSelectedMethodChanged = jest.fn(),
   onMethodsChanged = jest.fn(),
   onHideModeledMethodsChanged = jest.fn(),
   onModeChanged = jest.fn(),
@@ -16,6 +17,7 @@ export function createMockModelingEvents({
 }: {
   onActiveDbChanged?: ModelingEvents["onActiveDbChanged"];
   onDbClosed?: ModelingEvents["onDbClosed"];
+  onSelectedMethodChanged?: ModelingEvents["onSelectedMethodChanged"];
   onMethodsChanged?: ModelingEvents["onMethodsChanged"];
   onHideModeledMethodsChanged?: ModelingEvents["onHideModeledMethodsChanged"];
   onModeChanged?: ModelingEvents["onModeChanged"];
@@ -29,6 +31,7 @@ export function createMockModelingEvents({
   return mockedObject<ModelingEvents>({
     onActiveDbChanged,
     onDbClosed,
+    onSelectedMethodChanged,
     onMethodsChanged,
     onHideModeledMethodsChanged,
     onModeChanged,

extensions/ql-vscode/test/vscode-tests/no-workspace/model-editor/methods-usage/methods-usage-panel.test.ts

@@ -1,5 +1,5 @@
 import type { TreeView } from "vscode";
-import { window } from "vscode";
+import { EventEmitter, window } from "vscode";
 import type { CodeQLCliServer } from "../../../../../src/codeql-cli/cli";
 import type { Method } from "../../../../../src/model-editor/method";
 import { MethodsUsagePanel } from "../../../../../src/model-editor/methods-usage/methods-usage-panel";
@@ -66,6 +66,8 @@ describe("MethodsUsagePanel", () => {
     const modeledMethods: Record<string, ModeledMethod[]> = {};
     const modifiedMethodSignatures: Set<string> = new Set();
     const usage = createUsage();
+    const selectedMethodChangedEmitter: ModelingEvents["onSelectedMethodChangedEventEmitter"] =
+      new EventEmitter();
 
     beforeEach(() => {
       mockTreeView = mockedObject<TreeView<unknown>>({
@@ -74,7 +76,9 @@ describe("MethodsUsagePanel", () => {
       jest.spyOn(window, "createTreeView").mockReturnValue(mockTreeView);
 
       modelingStore = createMockModelingStore();
-      modelingEvents = createMockModelingEvents();
+      modelingEvents = createMockModelingEvents({
+        onSelectedMethodChanged: selectedMethodChangedEmitter.event,
+      });
     });
 
     it("should reveal the correct item in the tree view", async () => {
@@ -97,7 +101,15 @@ describe("MethodsUsagePanel", () => {
         modifiedMethodSignatures,
       );
 
-      await panel.revealItem(method.signature, usage);
+      selectedMethodChangedEmitter.fire({
+        databaseItem: dbItem,
+        method,
+        usage,
+        modeledMethods: modeledMethods[method.signature],
+        isModified: modifiedMethodSignatures.has(method.signature),
+        isInProgress: false,
+        processedByAutoModel: false,
+      });
 
       expect(mockTreeView.reveal).toHaveBeenCalledWith(
         expect.objectContaining({
@@ -124,7 +136,15 @@ describe("MethodsUsagePanel", () => {
         modifiedMethodSignatures,
       );
 
-      await panel.revealItem(method.signature, usage);
+      selectedMethodChangedEmitter.fire({
+        databaseItem: dbItem,
+        method,
+        usage,
+        modeledMethods: modeledMethods[method.signature],
+        isModified: modifiedMethodSignatures.has(method.signature),
+        isInProgress: false,
+        processedByAutoModel: false,
+      });
 
       expect(mockTreeView.reveal).not.toHaveBeenCalled();
     });