feat(workspace/#640): persistence infrastructure + persist workspace and window state

[glennsl]

This adds the necessary persistence infrastructure, as well as global and workspace stores for persisting the following items:

• Oni2 version
• Workspace path
• Window position
• Window size

It can be merged as-is, but there's a couple minor issues I'd like to iron out that requires upstream changes:

• Fix "jumping" when restoring position
• Store and restore maximized state

Edit: Actually, this shouldn't be merged until I've converted the write to use Libuv, as it's currently blocking.

Fixes #640
Fixes #619 - We now fall back to Documents instead
Fixes #906 - We now fall back to Documents, and then ultimately to $HOME
Addresses #639, #1659

<breaking>
  Oni2 will no longer open the working directory if no argument is passed. Instead you need to supply `.` 
</breaking>

persistence infrastructure + persist workspace and window state

Persists the last used version, the last opened workspace path and the window position and size for
each workspace.

If no file or folder argument is given, the last workspace path will be opened. Or, if this is the first time
Oni2 is launched, open the Documents folder if possible, or fall back to the home folder if not.

[bryphe]

So nice to have the window size and position persisted! 🎉

[bryphe]

Thanks for fixing #619 and #906 with this, @glennsl !

[bryphe]

I think the CI is hitting the same issue I saw in #1627 - I worked around it by adding this resolution:

"@opam/conf-pkg-config": "1.1"

[glennsl]

Yeah, I saw that and adopted it in revery-ui/revery#826 as well. Will bring it over here once that's merged, unless you've merged #1627 by then.

[glennsl]

Hmm, this seems to fail everywhere except on my computer. Currently building on my Macbook to try there, but could you try this on Windows as well @bryphe ?

[bryphe]

Hmm, this seems to fail everywhere except on my computer. Currently building on my Macbook to try there, but could you try this on Windows as well @bryphe ?

Sure, taking a look - my guess would be that the syntax server is failing to start for some reason - might've been impacted by changes to the cli or logging in Oni2_editor.re 🤔

[glennsl]

One of them definitely has an issue with the syntax highlighting, but the rest seem to all fail for different reasons. I'll restart to see if perhaps we can get even more flavors!

[glennsl]

/azp run

[bryphe]

Fun 😄 It seems on OSX, it's failing when running this command

esy x Oni2 -f --version

which outputs:

##[error]Bash exited with code '2'.

exit code 2 is usually an OCaml exception - so we could try adding --debug to that invocation and see if we can get a stacktrace.

[bryphe]

Same sort of failure on Windows. I think adding that --debug flag might give us a clue. I'll try it out on Windows too.

[bryphe]

I think it's related to not having a ~/.config/oni2 folder on the CI machines - I moved mine and am getting an exception now (esy x Oni2_editor -f --version isn't outputting anything)

...but it's great that it caught this!

[bryphe]

It seems like in the case where ~/.config/oni2 isn't created, it's crashing here:

  let store =
    instantiate("global", () => Schema.[entry(version), entry(workspace)]);

[glennsl]

Hmm, shouldn't that be created automatically? And shouldn't some integration test try to exercise user configuration or keybindings or some such?

[bryphe]

It gets created here in Filesystem.re:

let getStoreFolder = () =>
  getUserDataDirectory()
  >>= getOniDirectory
  >>= (dir => getPath(dir, "store"))
  >>= getOrCreateConfigFolder;

I think the problem is the getOrCreateConfigFolder (where the creation happens) is happening after trying to get the store path

[bryphe]

And shouldn't some integration test try to exercise user configuration or keybindings or some such?

The integration tests do exercise user config and keybindings, but they create separate folders for it (mainly to prevent it from paving a developer's configuration when they run it 😬 )

[glennsl]

There's really not much happening in instantiate before that gets called:

  let instantiate = (name, entries) => {
    let hash = Internal.hash(name);
    let filePath =
      Filesystem.getStoreFolder()
      |> Result.map(storeFolder => Filename.concat(storeFolder, hash))
      |> ResultEx.flatMap(Filesystem.getOrCreateConfigFolder)
      |> Result.map(folder => Filename.concat(folder, "store.json"))
      |> Result.get_ok;
    let store = {name, hash, filePath, entries: entries()};

[glennsl]

The integration tests do exercise user config and keybindings, but they create separate folders for it

Right, that's the complicated override code. It would be better to override the config folder more generally I think, and maybe that'd solve this issue as well.

[glennsl]

In the meantime I can make sure to handle this more gracefully in the Persistence code. Just log an error and continue without actually reading or writing anything.

[bryphe]

It looks like the bug is in the mkdir function in Filesystem - which we call in getOrCreateConfigFolder:

    | None => 
      /*
       No folder was present, lets make one.
       */
      mkdir(configDir, ()) >>= (() => return(configDir))

The function is:

let mkdir = (path, ~perm=userReadWriteExecute, ()) => {
  Unix.(
    try(mkdir(path, perm) |> return) {
    | Unix_error(err, _, _) =>
      error(
        "can't create directory '%s' because '%s",
        path,
        error_message(err),
      )
    }
  );
};

AFAIK, mkdir doesn't let you make a path of nested subdirectories... We need a better mkdir that handle a 'path' of subdirectories.

Before, we'd mkdir("~/.config/oni2"), but now we're using mkdir("~/.config/oni2/store")... and I guess that extra path is causing it to fail. A quick fix could be to ensure that we're creating the ~/.config/oni2 folder first, before anything else. A better long-term fix would be to set up mkdir to recursively create subdirectories, so that it can handle arbitrary paths.

And it seems like this could also fail in the case where a user had no ~/.config/ directory at all...

[glennsl]

@reason-native/fs has a mkDirP:

https://github.com/facebookexperimental/reason-native/blob/bb80cbca60210bd1832c280c1bbcfb2bf3add6d5/src/fs/Fs.rei#L209-L221

[bryphe]

I tried the latest, and I'm still seeing a crash on WIndows unfortunately.

Unfortunately I ran out of time today - I'll continue investigating Monday. In the meantime, one thing we could try splitting up the PR and getting pieces in - could help us localize what change is causing Windows to fail - some ideas:

• Bring in the new revery API separately, just to rule out any sort of native crash with SDL2 on Windows
• Bring in the fp/dir libraries separately, just in case they are broken on Windows - I haven't tested them there yet.

[bryphe]

One thing I've seen cause crashes on Windows is logging (writing to stdout/stderr) before we get a chance to init a console instance (which happens in Cli.parse, if -f is specified). But I removed this line, and it didn't fix the crash

[bryphe]

Ok, identified the crash!

(Invalid_argument "First token in path C:\\Users\\bryphe is not absolute.")
Raised at file "src/fp/Fp.re", line 220, characters 6-98
Called from file "src/dir/Dir.re", line 117, characters 16-35
Called from file "src/dir/Dir.re", line 187, characters 13-48
Called from file "src/dir/Dir.re", line 341, characters 8-20

Looks like Fp doesn't support windows style back-slashes. Looking at a place we can normalize these paths to work around it for now.

[bryphe]

Ok, took some time but got it working now - turns out there were a couple blockers... I documented them more thoroughly in a PR to dir: reasonml/reason-native#251

[glennsl]

Oh, I didn't notice that this is ready to be merged. I was waiting for reason-native to be updated! Thanks @bryphe!