fix(workspace/onivim#2742): OSX - Crash with Cmd+P on first launch (o…

…nivim#2761)

__Issue:__ Onivim may crash on OSX with `Cmd+P` when launching from finder for the first time

__Defect:__ The default directory for Onivim was set to the `Documents` folder. However, Onivim may not actually have permission to access this folder due to OSX's sandboxing. 

__Fix:__ If we don't have permission, don't set a workspace and fall-back to `Cmd+P` opening active buffers.

onivim#2711 helped this particular scenario, by setting up state and UI for the case where no workspace is opened / the working directory is not valid. However, there is still the case that we may have persisted the `Documents` folder, and we could still get that or another invalid folder coming from persistence. Therefore, we need to check permission for the folder prior to changing directory.

__TODO:__
- [x] Remove getWorkingDirectory in QuickMenuStoreConnector
- [x] Test on OSX w/ no persistence set
- [x] Test on OSX w/ persistence set to 'documents' folder
- [x] Test on Windows w/ no persistence set
- [x] Test on Linux w/ no persistence set

Fixes onivim#2742

manual_test/cases.md

@@ -14,3 +14,28 @@
 - Run Onivim 2 from dock
 - Validate Onivim 2 launches and PATH is correct
 
+# 2. First-Run Experience
+
+Test cases covering launching and using Onivim without any persistence or configuration.
+
+# 2.1 No directory set [OSX|Win|Linux]
+
+- Clear the Onivim 2 configuration folder (`rm -rf ~/.config/oni2`)
+- Launch Onivim
+- Verify explorer shows 'No folder opened'
+- Verify Control+P/Command+P shows the 'Welcome' buffer
+- Verify Control+Shift+P/Command+P shows the command palette
+
+# 2.2 Home directory set [OSX]
+
+This case is related to #2742 - previous builds of Onivim may have persisted
+the startup folder as `~/Documents`. This is problematic because Onivim may
+not have permission to read that folder.
+
+- Launch a developer build of Onivim
+- `:cd ~/Documents` (change to documents folder)
+- Re-launch developer build of Onivim - verify `~/Documents` is the current workspace.
+- Launch the release build of Onivim _from Finder_
+- Verify explorer shows 'No folder opened'
+- Verify Control+P/Command+P shows only the 'Welcome' buffer
+- Verify Control+Shift+P/Command+Shift+P shows the command palette

src/Core/Buffer.re

@@ -105,7 +105,7 @@ let getMediumFriendlyName =
        | FilePath(fp) =>
          switch (workingDirectory) {
          | Some(base) => Path.toRelative(~base, fp)
-         | _ => Sys.getcwd()
+         | None => fp
          }
        }
      );

src/Store/QuickmenuStoreConnector.re

@@ -107,8 +107,9 @@ let start = () => {
       Lwt.wakeup_exn(resolver, MenuCancelled)
     });
 
-  let makeBufferCommands = (languageInfo, iconTheme, buffers) => {
-    let workingDirectory = Rench.Environment.getWorkingDirectory(); // TODO: This should be workspace-relative
+  let makeBufferCommands = (workspace, languageInfo, iconTheme, buffers) => {
+    // Get the current workspace, if available
+    let maybeWorkspace = Feature_Workspace.openedFolder(workspace);
 
     buffers
     |> Feature_Buffers.all
@@ -118,7 +119,10 @@ let start = () => {
        )
     |> List.filter_map(buffer => {
          let maybeName =
-           Buffer.getMediumFriendlyName(~workingDirectory, buffer);
+           Buffer.getMediumFriendlyName(
+             ~workingDirectory=?maybeWorkspace,
+             buffer,
+           );
          let maybePath = Buffer.getFilePath(buffer);
 
          OptionEx.map2(
@@ -173,7 +177,8 @@ let start = () => {
       )
 
     | QuickmenuShow(EditorsPicker) =>
-      let items = makeBufferCommands(languageInfo, iconTheme, buffers);
+      let items =
+        makeBufferCommands(workspace, languageInfo, iconTheme, buffers);
 
       (
         Some({
@@ -194,7 +199,8 @@ let start = () => {
 
     | QuickmenuShow(FilesPicker) =>
       if (Feature_Workspace.openedFolder(workspace) == None) {
-        let items = makeBufferCommands(languageInfo, iconTheme, buffers);
+        let items =
+          makeBufferCommands(workspace, languageInfo, iconTheme, buffers);
 
         (
           Some({...Quickmenu.defaults(OpenBuffersPicker), items}),
@@ -242,7 +248,8 @@ let start = () => {
       );
 
     | QuickmenuShow(OpenBuffersPicker) =>
-      let items = makeBufferCommands(languageInfo, iconTheme, buffers);
+      let items =
+        makeBufferCommands(workspace, languageInfo, iconTheme, buffers);
 
       (
         Some({...Quickmenu.defaults(OpenBuffersPicker), items}),
@@ -251,7 +258,8 @@ let start = () => {
 
     | QuickmenuShow(FileTypesPicker({bufferId, languages})) =>
       if (Feature_Workspace.openedFolder(workspace) == None) {
-        let items = makeBufferCommands(languageInfo, iconTheme, buffers);
+        let items =
+          makeBufferCommands(workspace, languageInfo, iconTheme, buffers);
 
         (
           Some({...Quickmenu.defaults(OpenBuffersPicker), items}),
@@ -549,42 +557,51 @@ let subscriptions = (ripgrep, dispatch) => {
     );
   };
 
-  let ripgrep = (languageInfo, iconTheme, configuration) => {
+  let ripgrep = (workspace, languageInfo, iconTheme, configuration) => {
     let filesExclude =
       Configuration.getValue(c => c.filesExclude, configuration);
-    let directory = Rench.Environment.getWorkingDirectory(); // TODO: This should be workspace-relative
-
-    let stringToCommand = (languageInfo, iconTheme, fullPath) =>
-      Actions.{
-        category: None,
-        name: Path.toRelative(~base=directory, fullPath),
-        command: () => Actions.OpenFileByPath(fullPath, None, None),
-        icon:
-          Component_FileExplorer.getFileIcon(
-            ~languageInfo,
-            ~iconTheme,
-            fullPath,
-          ),
-        highlight: [],
-        handle: None,
-      };
-
-    RipgrepSubscription.create(
-      ~id="workspace-search",
-      ~filesExclude,
-      ~directory,
-      ~ripgrep,
-      ~onUpdate=
-        items => {
-          items
-          |> List.map(stringToCommand(languageInfo, iconTheme))
-          |> addItems;
 
-          dispatch(Actions.QuickmenuUpdateRipgrepProgress(Loading));
-        },
-      ~onComplete=() => Actions.QuickmenuUpdateRipgrepProgress(Complete),
-      ~onError=_ => Actions.Noop,
-    );
+    switch (Feature_Workspace.openedFolder(workspace)) {
+    | None =>
+      // It's not really clear what the behavior should be for the ripgrep subscription w/o
+      // an opened workspace / folder. In the current logic, we shouldn't ever get here -
+      // when opening the 'files picker', if there is no workspace, we'll open the 'buffers picker'
+      // instead.
+      []
+    | Some(directory) =>
+      let stringToCommand = (languageInfo, iconTheme, fullPath) =>
+        Actions.{
+          category: None,
+          name: Path.toRelative(~base=directory, fullPath),
+          command: () => Actions.OpenFileByPath(fullPath, None, None),
+          icon:
+            Component_FileExplorer.getFileIcon(
+              ~languageInfo,
+              ~iconTheme,
+              fullPath,
+            ),
+          highlight: [],
+          handle: None,
+        };
+      [
+        RipgrepSubscription.create(
+          ~id="workspace-search",
+          ~filesExclude,
+          ~directory,
+          ~ripgrep,
+          ~onUpdate=
+            items => {
+              items
+              |> List.map(stringToCommand(languageInfo, iconTheme))
+              |> addItems;
+
+              dispatch(Actions.QuickmenuUpdateRipgrepProgress(Loading));
+            },
+          ~onComplete=() => Actions.QuickmenuUpdateRipgrepProgress(Complete),
+          ~onError=_ => Actions.Noop,
+        ),
+      ];
+    };
   };
 
   let updater = (state: State.t) => {
@@ -600,10 +617,14 @@ let subscriptions = (ripgrep, dispatch) => {
       | Extension({hasItems, _}) =>
         hasItems ? [filter(query, quickmenu.items)] : []
 
-      | FilesPicker => [
-          filter(query, quickmenu.items),
-          ripgrep(state.languageInfo, state.iconTheme, state.configuration),
-        ]
+      | FilesPicker =>
+        [filter(query, quickmenu.items)]
+        @ ripgrep(
+            state.workspace,
+            state.languageInfo,
+            state.iconTheme,
+            state.configuration,
+          )
 
       | FileTypesPicker(_) => [filter(query, quickmenu.items)]
 

src/bin_editor/Oni2_editor.re

@@ -80,15 +80,37 @@ switch (eff) {
       | None => Store.Persistence.Global.workspace()
       };
 
+    let couldChangeDirectory = ref(false);
     maybePath
     |> Option.iter(path => {
-         Log.info("Startup: Changing folder to: " ++ path);
-         try(Sys.chdir(path)) {
-         | Sys_error(msg) => Log.error("Folder does not exist: " ++ msg)
+         Log.infof(m => m("Startup: Trying to change folder to: %s", path));
+
+         let chdirResult = {
+           open Base.Result.Let_syntax;
+           // First, check if we have permission to read the directory.
+           // In some cases - like #2742 - the directory might not be valid.
+           let%bind _: Luv.File.Dir.t = Luv.File.Sync.opendir(path);
+           Log.info(" - Have read permission");
+           let%bind () = Luv.Path.chdir(path);
+           Log.info("- Ran chdir");
+           Ok();
          };
+
+         chdirResult
+         |> Result.iter(() => {
+              couldChangeDirectory := true;
+              Log.infof(m =>
+                m("Successfully changed working directory to: %s", path)
+              );
+            });
        });
 
-    maybePath;
+    // The directory that was persisted is a valid workspace, so we can use it
+    if (couldChangeDirectory^) {
+      maybePath;
+    } else {
+      None;
+    };
   };
 
   // Fix for https://github.com/onivim/oni2/issues/2229
@@ -198,9 +220,12 @@ switch (eff) {
     Oni2_KeyboardLayout.init();
     Oni2_Sparkle.init();
 
+    // Grab initial working directory prior to trying to set it -
+    // in some cases, a directory that does not have permissions may be persisted (ie #2742)
+    let initialWorkingDirectory = Sys.getcwd();
     let maybeWorkspace = initWorkspace();
     let workingDirectory =
-      maybeWorkspace |> Option.value(~default=Sys.getcwd());
+      maybeWorkspace |> Option.value(~default=initialWorkingDirectory);
 
     let window =
       createWindow(

src/bin_editor/dune

@@ -7,4 +7,4 @@
    Oni2.syntax_client Oni2.syntax_server Oni2.ui reason-sdl2 reason-harfbuzz
    Oni2.sparkle Oni2.KeyboardLayout fp dir.lib)
  (preprocess
-  (pps lwt_ppx brisk-reconciler.ppx)))
+  (pps ppx_let lwt_ppx brisk-reconciler.ppx)))