Remove dry-run popup, replace with configureOnOpen questions (#573)

* update default to null for configureOnOpen

* haven't tested, but pop-up code

* implement working UI

* add telemetry

* remove dry-run popup

* slight update string

* update question

* update questions to ask about intellisense

* remove TODO

package.json

@@ -479,7 +479,7 @@
         },
         "makefile.configureOnOpen": {
           "type": "boolean",
-          "default": true,
+          "default": null,
           "description": "%makefile-tools.configuration.makefile.configureOnOpen.description%",
           "scope": "resource"
         },

src/extension.ts

@@ -7,7 +7,6 @@ import * as configuration from "./configuration";
 import * as cpptools from "./cpptools";
 import * as launch from "./launch";
 import { promises as fs } from "fs";
-import * as logger from "./logger";
 import * as make from "./make";
 import * as parser from "./parser";
 import * as path from "path";
@@ -20,7 +19,7 @@ import * as vscode from "vscode";
 import * as cpp from "vscode-cpptools";
 
 import * as nls from "vscode-nls";
-import { readBuildLog } from "./configuration";
+import { TelemetryEventProperties } from "@vscode/extension-telemetry";
 nls.config({
   messageFormat: nls.MessageFormat.bundle,
   bundleFormat: nls.BundleFormat.standalone,
@@ -732,7 +731,7 @@ export async function activate(
       )
     );
   }
-  // === Commands only for testing ===
+  // === End of commands only for testing ===
 
   const parseCompilerArgsScript: string = util.parseCompilerArgsScriptFile();
 
@@ -740,9 +739,96 @@ export async function activate(
   // 0x755 means rwxr-xr-x (read and execute for everyone, write for owner).
   await fs.chmod(parseCompilerArgsScript, 0o755);
 
-  if (configuration.getConfigureOnOpen() && extension.getFullFeatureSet()) {
+  if (extension.getFullFeatureSet()) {
-    // Always clean configure on open
+    let shouldConfigure = configuration.getConfigureOnOpen();
-    await make.cleanConfigure(make.TriggeredBy.cleanConfigureOnOpen);
+    if (shouldConfigure === null) {
+      // Ask if the user wants to configure on open with the Makefile Tools extension.
+      interface Choice1 {
+        title: string;
+        doConfigure: boolean;
+      }
+      const chosen = await vscode.window.showInformationMessage<Choice1>(
+        localize(
+          "extension.configureOnOpen",
+          "Would you like to configure C++ IntelliSense for this workspace using information from your Makefiles?"
+        ),
+        {},
+        { title: localize("yes", "Yes"), doConfigure: true },
+        { title: localize("no", "No"), doConfigure: false }
+      );
+      if (!chosen) {
+        // User cancelled, they don't want to configure.
+        shouldConfigure = false;
+        telemetry.logConfigureOnOpenTelemetry(false);
+      } else {
+        // ask them if they always want to configure on open.
+        // TODO: More work to do here to have the right flow.
+        const persistMessage = chosen.doConfigure
+          ? localize(
+              "always.configure.on.open",
+              "Always configure C++ IntelliSense using information from your Makefiles upon opening?"
+            )
+          : localize(
+              "never.configure.on.open",
+              "Configure C++ IntelliSense using information from your Makefiles upon opening?"
+            );
+        const buttonMessages = chosen.doConfigure
+          ? [localize("yes.button", "Yes"), localize("no.button", "No")]
+          : [
+              localize("never.button", "Never"),
+              localize(
+                "never.for.this.workspace.button",
+                "Not for this workspace"
+              ),
+            ];
+        interface Choice2 {
+          title: string;
+          persistMode: telemetry.ConfigureOnOpenScope;
+        }
+
+        vscode.window
+          .showInformationMessage<Choice2>(
+            persistMessage,
+            {},
+            { title: buttonMessages[0], persistMode: "user" },
+            { title: buttonMessages[1], persistMode: "workspace" }
+          )
+          .then(async (choice) => {
+            if (!choice) {
+              // User cancelled. Do nothing.
+              telemetry.logConfigureOnOpenTelemetry(chosen.doConfigure);
+              return;
+            }
+
+            let configTarget = vscode.ConfigurationTarget.Global;
+            if (choice.persistMode === "workspace") {
+              configTarget = vscode.ConfigurationTarget.Workspace;
+            }
+            const workspaceFolder = vscode.workspace.workspaceFolders?.[0];
+            if (workspaceFolder) {
+              await vscode.workspace
+                .getConfiguration(undefined, workspaceFolder)
+                .update(
+                  "makefile.configureOnOpen",
+                  chosen.doConfigure,
+                  configTarget
+                );
+            }
+
+            telemetry.logConfigureOnOpenTelemetry(
+              chosen.doConfigure,
+              choice.persistMode
+            );
+          });
+
+        shouldConfigure = chosen.doConfigure;
+
+        if (shouldConfigure === true) {
+          // We've opened a new workspace folder, and the user wants us to configure it now.
+          await make.cleanConfigure(make.TriggeredBy.cleanConfigureOnOpen);
+        }
+      }
+    }
   }
 
   // Analyze settings for type validation and telemetry

src/make.ts

@@ -1267,66 +1267,6 @@ export async function configure(
   triggeredBy: TriggeredBy,
   updateTargets: boolean = true
 ): Promise<number> {
-  // Before running the --dry-run type of configure for the first time, even in a trusted workspace,
-  // notify the user that it is possible some code to be executed even under "--dry-run" mode.
-  // Note that this is a state variable and can be reseted via the command resetState, causing the popup to show again
-  // even if a successfull --dry-run configure has been run in the past.
-  let ranDryRunInCodebaseLifetime: boolean =
-    extension.getState().ranDryRunInCodebaseLifetime;
-  if (!ranDryRunInCodebaseLifetime && !configuration.getBuildLog()) {
-    // The window popup message should be concise but more logging can be useful in the output channel.
-    logger.message(
-      "The Makefile Tools extension process of configuring your project is about to run 'make --dry-run' in order to parse the output for useful information. " +
-        "This is needed to calculate accurate IntelliSense and targets information. " +
-        "Although in general 'make --dry-run' only lists (without executing) the operations 'make' would do in the current context, " +
-        "it is still possible some code to be executed, like $(shell) syntax in the makefile or recursive invocations of the $(MAKE) variable."
-    );
-    logger.message(
-      "If you don't feel comfortable allowing this configure process and 'make --dry-run' to be invoked by the extension, " +
-        "you can chose a recent full, clean, verbose and up-to-date build log as an alternative, via the setting 'makefile.buildLog'. "
-    );
-    // Also, show the output channel for that message to be visible.
-    logger.showOutputChannel();
-    let yesButton: string = localize(
-      "yes.dont.show.again",
-      "Yes (don't show again)"
-    );
-    let noButton: string = localize("no", "No");
-    const chosen: vscode.MessageItem | undefined =
-      await vscode.window.showErrorMessage<vscode.MessageItem>(
-        localize(
-          "code.can.execute.dryrun.mode.continue",
-          "Configuring project. Code can still execute in --dry-run mode. Do you want to continue?"
-        ),
-        {
-          title: yesButton,
-          isCloseAffordance: false,
-        },
-        {
-          title: noButton,
-          isCloseAffordance: true,
-        }
-      );
-
-    // The 'code possibly executed under --dry-run' warning makes sense only for the configure operation.
-    // This is when the user may not expect this to happen. Does not apply for a build or debug/launch and even for pre/post configure which,
-    // if set by the user, they represent intentional commands.
-
-    // If the user answered yes to continue with the --dry-run (configure), `dryRunApproved` will be true.
-    // If the user decided not to continue with the --dry-run (configure), `dryRunApproved` will be false.
-    const telemetryProperties: telemetry.Properties = {
-      dryRunApproved: chosen?.title === yesButton ? "true" : "false",
-      triggeredBy: triggeredBy,
-    };
-    telemetry.logEvent("dryRunWarning", telemetryProperties);
-
-    if (chosen === undefined || chosen.title === noButton) {
-      return ConfigureBuildReturnCodeTypes.untrusted;
-    }
-
-    extension.getState().ranDryRunInCodebaseLifetime = true;
-  }
-
   // Mark that this workspace had at least one attempt at configuring (of any kind: --dry-run or buildLog), before any chance of early return,
   // to accurately identify in telemetry whether this project configured successfully out of the box or not.
   let ranConfigureInCodebaseLifetime: boolean =

src/state.ts

@@ -53,18 +53,6 @@ export class StateManager {
     this._update("ranConfigureInCodebaseLifetime", v);
   }
 
-  // Whether this project had any --dry-run specific configure attempt before
-  // (it didn't have to succeed or even complete).
-  // This is used in order to notify the user via a Yes(don't show again)/No popup
-  // that some makefile code could still execute even in --dry-run mode.
-  // Once the user decides 'Yes(don't show again)' the popup is not shown.
-  get ranDryRunInCodebaseLifetime(): boolean {
-    return this._get<boolean>("ranDryRunInCodebaseLifetime") || false;
-  }
-  set ranDryRunInCodebaseLifetime(v: boolean) {
-    this._update("ranDryRunInCodebaseLifetime", v);
-  }
-
   // If the project needs a clean configure as a result
   // of an operation that alters the configure state
   // (makefile configuration change, build target change,
@@ -87,7 +75,6 @@ export class StateManager {
     this.buildTarget = undefined;
     this.launchConfiguration = undefined;
     this.ranConfigureInCodebaseLifetime = false;
-    this.ranDryRunInCodebaseLifetime = false;
     this.configureDirty = false;
 
     if (reloadWindow) {

src/telemetry.ts

@@ -422,3 +422,14 @@ function getPackageInfo(): IPackageInfo {
     aiKey: "AIF-d9b70cd4-b9f9-4d70-929b-a071c400b217",
   };
 }
+
+export type ConfigureOnOpenScope = "user" | "workspace";
+export function logConfigureOnOpenTelemetry(
+  configureOnOpen: boolean,
+  scope: ConfigureOnOpenScope | undefined = undefined
+) {
+  logEvent("configureOnOpenPopup", {
+    configureOnOpen: configureOnOpen.toString(),
+    scope: scope ?? "N/A",
+  });
+}