Skip to content

Commit

Permalink
fix(windows/#3151): Support UNC paths when changing directories (#3162)
Browse files Browse the repository at this point in the history
__Issue:__ Trying to `:cd` into a [UNC Path](https://docs.microsoft.com/en-us/dotnet/standard/io/file-path-formats#unc-paths) on Windows would fail

__Defect:__ #3142 moved away from using `string`s to strongly-typed paths (`Fp.t`), to help ensure consistent normalization. However, the parsing logic for ingesting a `string` -> `option(Fp.t)` did not handle parsing UNC paths, so the system would treat it as a failed path.

__Fix:__ Ideally, `Fp` could handle this out of the box: reasonml/reason-native#262 

In the meantime, add an 'adapter' layer around `Fp` - when ingesting strings for Windows, first try to see if it is a UNC-style path. If it is, grab the server and share name and store it around an `Fp` path. Proxy all operations to and from the `Fp` path.

When the `Fp` is converted back to a string, make sure to use backslashes for that type of path.

__Next steps:__
- Considering porting fix to `Fp`(after some dependent PRs are in, like reasonml/reason-native#261)

Fixes #3151
  • Loading branch information
bryphe authored Feb 19, 2021
1 parent 1aaad90 commit fe06c60
Show file tree
Hide file tree
Showing 49 changed files with 427 additions and 170 deletions.
1 change: 1 addition & 0 deletions CHANGES_CURRENT.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
- #3077 - Terminal: Use editor font as default (related #3062)
- #3146 - Vim: Fix command-line staying open when clicking the editor or file explorer (fixes #3031)
- #3161 - Configuration: Turn soft word-wrap on by default (fixes #3161)
- #3162 - Windows: Support opening UNC paths (fixes #3151)

### Performance

Expand Down
3 changes: 2 additions & 1 deletion integration_test/lib/Oni_IntegrationTestLib.re
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
module Core = Oni_Core;
module FpExp = Core.FpExp;
module Utility = Core.Utility;

module Model = Oni_Model;
Expand Down Expand Up @@ -157,7 +158,7 @@ let runTest =
|> Printf.fprintf(oc, "%s\n");

close_out(oc);
tempFilePath |> Fp.absoluteCurrentPlatform |> Option.get;
tempFilePath |> FpExp.absoluteCurrentPlatform |> Option.get;
};

let keybindingsFilePath =
Expand Down
15 changes: 15 additions & 0 deletions manual_test/cases.md
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,21 @@ __Pass:__
- [ ] OSX
- [ ] Linux

## 8.2 Windows-style path handling

### 8.2.1 Verify can `:cd` into a UNC path

Regression test for #3151

- Open Onivim 2
- `:cd` into a UNC path - for example: `\\\\LOCALHOST\\c$\\oni2`
- Verify the explorer is refreshed
- Verify directory nodes can be expanded
- Verify files can be opened

__Pass:__
- [ ] Win

# 9. Terminal

## 9.1 Check that `ONIVIM_TERMINAL` is set
Expand Down
6 changes: 4 additions & 2 deletions src/CLI/Oni_CLI.re
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
*
* Module for handling command-line arguments for Oni2
*/
open Oni_Core;
open Kernel;
open Rench;

Expand All @@ -14,7 +15,7 @@ type t = {
folder: option(string),
filesToOpen: list(string),
forceScaleFactor: option(float),
overriddenExtensionsDir: option(Fp.t(Fp.absolute)),
overriddenExtensionsDir: option(FpExp.t(FpExp.absolute)),
shouldLoadExtensions: bool,
shouldLoadConfiguration: bool,
shouldSyntaxHighlight: bool,
Expand Down Expand Up @@ -271,7 +272,8 @@ let parse = (~getenv: string => option(string), args) => {
forceScaleFactor: scaleFactor^,
gpuAcceleration: gpuAcceleration^,
overriddenExtensionsDir:
extensionsDir^ |> Utility.OptionEx.flatMap(Fp.absoluteCurrentPlatform),
extensionsDir^
|> Utility.OptionEx.flatMap(FpExp.absoluteCurrentPlatform),
shouldLoadExtensions: shouldLoadExtensions^,
shouldLoadConfiguration: shouldLoadConfiguration^,
shouldSyntaxHighlight: shouldSyntaxHighlight^,
Expand Down
4 changes: 3 additions & 1 deletion src/CLI/Oni_CLI.rei
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
open Oni_Core;

type t = {
gpuAcceleration: [ | `Auto | `ForceSoftware | `ForceHardware],
folder: option(string),
filesToOpen: list(string),
forceScaleFactor: option(float),
overriddenExtensionsDir: option(Fp.t(Fp.absolute)),
overriddenExtensionsDir: option(FpExp.t(FpExp.absolute)),
shouldLoadExtensions: bool,
shouldLoadConfiguration: bool,
shouldSyntaxHighlight: bool,
Expand Down
20 changes: 10 additions & 10 deletions src/Components/FileExplorer/Component_FileExplorer.re
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ module Internal = {
};

let luvDirentToFsTree = (~cwd, {name, kind}: Luv.File.Dirent.t) => {
let path = Fp.At.(cwd / name);
let path = FpExp.At.(cwd / name);

if (kind == `FILE || kind == `LINK) {
Some(FsTreeNode.file(path));
Expand All @@ -32,7 +32,7 @@ module Internal = {
};
};

let luvDirentsToFsTree = (~cwd: Fp.t(Fp.absolute), ~ignored, dirents) => {
let luvDirentsToFsTree = (~cwd: FpExp.t(FpExp.absolute), ~ignored, dirents) => {
dirents
|> List.filter(({name, _}: Luv.File.Dirent.t) =>
name != ".." && name != "." && !List.mem(name, ignored)
Expand All @@ -53,12 +53,12 @@ module Internal = {
*/
let getFilesAndFolders = (~ignored, cwd) => {
cwd
|> Fp.toString
|> FpExp.toString
|> Service_OS.Api.readdir
|> Lwt.map(luvDirentsToFsTree(~ignored, ~cwd));
};

let getDirectoryTree = (cwd: Fp.t(Fp.absolute), ignored) => {
let getDirectoryTree = (cwd: FpExp.t(FpExp.absolute), ignored) => {
let childrenPromise = getFilesAndFolders(~ignored, cwd);

childrenPromise
Expand All @@ -71,7 +71,7 @@ module Internal = {
module Effects = {
let load = (directory, configuration, ~onComplete) => {
Isolinear.Effect.createWithDispatch(~name="explorer.load", dispatch => {
let directoryStr = Fp.toString(directory);
let directoryStr = FpExp.toString(directory);
Log.infof(m => m("Loading nodes for directory: %s", directoryStr));
let ignored =
Configuration.getValue(c => c.filesExclude, configuration);
Expand Down Expand Up @@ -110,7 +110,7 @@ type outmsg =
| GrabFocus;

let setTree = (tree, model) => {
let uniqueId = (data: FsTreeNode.metadata) => Fp.toString(data.path);
let uniqueId = (data: FsTreeNode.metadata) => FpExp.toString(data.path);
let (rootName, firstLevelChildren) =
switch (tree) {
| Tree.Leaf(_) => ("", [])
Expand Down Expand Up @@ -282,14 +282,14 @@ let update = (~configuration, msg, model) => {
c => c.workbenchEditorEnablePreview,
configuration,
)
? PreviewFile(Fp.toString(node.path))
: OpenFile(Fp.toString(node.path)),
? PreviewFile(FpExp.toString(node.path))
: OpenFile(FpExp.toString(node.path)),
)
| Component_VimTree.Selected(node) =>
// Set active here to avoid scrolling in BufferEnter
(
model |> setActive(Some(node.path)),
OpenFile(Fp.toString(node.path)),
OpenFile(FpExp.toString(node.path)),
)
| Component_VimTree.Nothing => (model, Nothing)
};
Expand All @@ -313,7 +313,7 @@ let sub = (~configuration, {rootPath, _}) => {
Service_OS.Sub.dir(
~uniqueId="FileExplorerSideBar",
~toMsg,
Fp.toString(rootPath),
FpExp.toString(rootPath),
);
};

Expand Down
8 changes: 4 additions & 4 deletions src/Components/FileExplorer/Component_FileExplorer.rei
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,14 @@ type msg;

module Msg: {
let keyPressed: string => msg;
let activeFileChanged: option(Fp.t(Fp.absolute)) => msg;
let activeFileChanged: option(FpExp.t(FpExp.absolute)) => msg;
};

type model;

let initial: (~rootPath: Fp.t(Fp.absolute)) => model;
let setRoot: (~rootPath: Fp.t(Fp.absolute), model) => model;
let root: model => Fp.t(Fp.absolute);
let initial: (~rootPath: FpExp.t(FpExp.absolute)) => model;
let setRoot: (~rootPath: FpExp.t(FpExp.absolute), model) => model;
let root: model => FpExp.t(FpExp.absolute);

let keyPress: (string, model) => model;

Expand Down
10 changes: 5 additions & 5 deletions src/Components/FileExplorer/FileTreeView.re
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,8 @@ let nodeView =
let path = node.path;
switch (decoration) {
| Some((decoration: Feature_Decorations.Decoration.t)) =>
Fp.toString(path) ++ " • " ++ decoration.tooltip
| None => Fp.toString(path)
FpExp.toString(path) ++ " • " ++ decoration.tooltip
| None => FpExp.toString(path)
};
};

Expand All @@ -100,7 +100,7 @@ let make =
~focusedIndex,
~treeView:
Component_VimTree.model(FsTreeNode.metadata, FsTreeNode.metadata),
~active: option(Fp.t(Fp.absolute)),
~active: option(FpExp.t(FpExp.absolute)),
~theme,
~decorations: Feature_Decorations.model,
~font: UiFont.t,
Expand Down Expand Up @@ -136,14 +136,14 @@ let make =
font
iconTheme
languageInfo
path={Fp.toString(data.path)}
path={FpExp.toString(data.path)}
/>,
data,
)
};
let decorations =
Feature_Decorations.getDecorations(
~path=Fp.toString(data.path),
~path=FpExp.toString(data.path),
decorations,
);
<nodeView
Expand Down
21 changes: 12 additions & 9 deletions src/Components/FileExplorer/FsTreeNode.re
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ open Utility;

[@deriving show({with_path: false})]
type metadata = {
path: [@opaque] Fp.t(Fp.absolute),
path: [@opaque] FpExp.t(FpExp.absolute),
displayName: string,
hash: int // hash of basename, so only comparable locally
};
Expand All @@ -17,35 +17,35 @@ module PathHasher = {
// type t = list(string);

let make = (~base, path) => {
switch (Fp.relativize(~source=base, ~dest=path)) {
switch (FpExp.relativize(~source=base, ~dest=path)) {
| Ok(relativePath) => FpEx.explode(relativePath) |> List.map(hash)
| Error(_) => []
};
};

let%test "equivalent paths" = {
// TODO: Is this case even correct?
make(~base=Fp.(root), Fp.(root)) == [];
make(~base=FpExp.(root), FpExp.(root)) == [];
};

let%test "simple path" = {
make(~base=Fp.(root), Fp.(At.(root / "abc"))) == [hash("abc")];
make(~base=FpExp.(root), FpExp.(At.(root / "abc"))) == [hash("abc")];
};

let%test "multiple paths" = {
make(~base=Fp.(root), Fp.(At.(root / "abc" / "def")))
make(~base=FpExp.(root), FpExp.(At.(root / "abc" / "def")))
== [hash("abc"), hash("def")];
};
};

let file = path => {
let basename = Fp.baseName(path) |> Option.value(~default="(empty)");
let basename = FpExp.baseName(path) |> Option.value(~default="(empty)");

Tree.leaf({path, hash: PathHasher.hash(basename), displayName: basename});
};

let directory = (~isOpen=false, path, ~children) => {
let basename = Fp.baseName(path) |> Option.value(~default="(empty)");
let basename = FpExp.baseName(path) |> Option.value(~default="(empty)");

Tree.node(
~expanded=isOpen,
Expand Down Expand Up @@ -139,7 +139,10 @@ let replace = (~replacement, tree) => {
};

loop(
PathHasher.make(~base=Fp.dirName(getPath(tree)), getPath(replacement)),
PathHasher.make(
~base=FpExp.dirName(getPath(tree)),
getPath(replacement),
),
tree,
);
};
Expand Down Expand Up @@ -167,7 +170,7 @@ let updateNodesInPath =
| _ => node
};

loop(PathHasher.make(~base=Fp.dirName(getPath(tree)), path), tree);
loop(PathHasher.make(~base=FpExp.dirName(getPath(tree)), path), tree);
};

let toggleOpen =
Expand Down
16 changes: 9 additions & 7 deletions src/Components/FileExplorer/FsTreeNode.rei
Original file line number Diff line number Diff line change
Expand Up @@ -2,26 +2,28 @@ open Oni_Core;

[@deriving show({with_path: false})]
type metadata = {
path: Fp.t(Fp.absolute),
path: FpExp.t(FpExp.absolute),
displayName: string,
hash: int // hash of basename, so only comparable locally
};

[@deriving show({with_path: false})]
type t = Tree.t(metadata, metadata);

let file: Fp.t(Fp.absolute) => t;
let directory: (~isOpen: bool=?, Fp.t(Fp.absolute), ~children: list(t)) => t;
let file: FpExp.t(FpExp.absolute) => t;
let directory:
(~isOpen: bool=?, FpExp.t(FpExp.absolute), ~children: list(t)) => t;

let getPath: t => Fp.t(Fp.absolute);
let getPath: t => FpExp.t(FpExp.absolute);
let displayName: t => string;

let findNodesByPath:
(Fp.t(Fp.absolute), t) => [ | `Success(list(t)) | `Partial(t) | `Failed];
let findByPath: (Fp.t(Fp.absolute), t) => option(t);
(FpExp.t(FpExp.absolute), t) =>
[ | `Success(list(t)) | `Partial(t) | `Failed];
let findByPath: (FpExp.t(FpExp.absolute), t) => option(t);

let replace: (~replacement: t, t) => t;
let updateNodesInPath: (t => t, Fp.t(Fp.absolute), t) => t;
let updateNodesInPath: (t => t, FpExp.t(FpExp.absolute), t) => t;
let toggleOpen: t => t;
let setOpen: t => t;

Expand Down
12 changes: 6 additions & 6 deletions src/Components/FileExplorer/Model.re
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
open Oni_Core;
// MODEL

[@deriving show]
type msg =
| ActiveFilePathChanged([@opaque] option(Fp.t(Fp.absolute)))
| ActiveFilePathChanged([@opaque] option(FpExp.t(FpExp.absolute)))
| TreeLoaded(FsTreeNode.t)
| TreeLoadError(string)
| NodeLoaded(FsTreeNode.t)
Expand All @@ -16,14 +17,14 @@ module Msg = {
};

type model = {
rootPath: Fp.t(Fp.absolute),
rootPath: FpExp.t(FpExp.absolute),
rootName: string,
tree: option(FsTreeNode.t),
treeView: Component_VimTree.model(FsTreeNode.metadata, FsTreeNode.metadata),
isOpen: bool,
scrollOffset: [ | `Start(float) | `Middle(float) | `Reveal(int)],
active: option(Fp.t(Fp.absolute)),
focus: option(Fp.t(Fp.absolute)) // path
active: option(FpExp.t(FpExp.absolute)),
focus: option(FpExp.t(FpExp.absolute)) // path
};

let initial = (~rootPath) => {
Expand Down Expand Up @@ -64,6 +65,5 @@ let getIndex = (path, model) => {
};

let getFocusedIndex = model => {
model.active
|> Oni_Core.Utility.OptionEx.flatMap(path => getIndex(path, model));
model.active |> Utility.OptionEx.flatMap(path => getIndex(path, model));
};
4 changes: 2 additions & 2 deletions src/Core/Config.re
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,10 @@ module Settings = {
};

let fromFile = path =>
try(path |> Fp.toString |> Yojson.Safe.from_file |> fromJson) {
try(path |> FpExp.toString |> Yojson.Safe.from_file |> fromJson) {
| Yojson.Json_error(message) =>
Log.errorf(m =>
m("Failed to read file %s: %s", path |> Fp.toString, message)
m("Failed to read file %s: %s", path |> FpExp.toString, message)
);
empty;
};
Expand Down
2 changes: 1 addition & 1 deletion src/Core/Config.rei
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ module Settings: {

let fromList: list((string, Json.t)) => t;
let fromJson: Json.t => t;
let fromFile: Fp.t(Fp.absolute) => t;
let fromFile: FpExp.t(FpExp.absolute) => t;

let get: (key, t) => option(Json.t);

Expand Down
Loading

0 comments on commit fe06c60

Please sign in to comment.