Bugfix/open new file after close

[akinsho]

This PR handles cases where the mainWindow has been closed and user attempts to run a menubar function like new file or copy (relates #1193). Note that for commands that require an initialised oni it will not carry out those commands such as split window etc. it just defaults to opening a window so there is no error.

It replicates vscode's functionality which simply opens a new window which seems the easiest solution to me. A more adventurous possible solution to this could be passing the createWindow function a third argument of events(and their arguments) to execute once the app has loaded 😟

Update: tried the alternative method I mentioned above but it does not work unfortunately

[bryphe]

I'm wondering if this should return a Promise? In other words - I'm not sure if we will properly handle all of the commands we send if we are sending them immediately when the window is created. I believe we might need to wait for the window to be fully initialized before we send some of these commands.

An example of this would be menu-item-click and open-files - these are listened for in the NeovimEditor class, which might take a bit of time after opening to be initialized and start listening for those commands.

[akinsho]

@bryphe thats definitely the case the issue I encountered, my thought though is that the initialisation just triggers an event which then sets off other async processes but the createWindow function itself from what I can see is sync, I'm not entirely sure how to make it wait, an arbitrary timeout would be a messy/hacky solution, I thought maybe of having the ipcMain process send an event when neovim is initialised and then responding to it in an event handler in the main process sort of the reverse of what we've been doing for the most part so outside in, not sure if thats possible

[bryphe]

Ah ok, luckily for us, there is a way we can make createWindow async - or at least return a Promise - to help us out here.

I thought maybe of having the ipcMain process send an event when neovim is initialised and then responding to it in an event handler in the main process sort of the reverse of what we've been doing for the most part so outside in, not sure if thats possible

Yes, definitely - this is a good idea!

We already hook an event today to figure out when the initialization happens:

oni/main/src/main.ts

Line 141 in c01ae06

mainWindow.webContents.send("init", {

What we could do is the following:

• From createWindow, we return a Promise
• In that did-finish-load handler (or our handler we add), we resolve the promise we returned
• We gate the command handlers on the createWindow promise (using await)

That should be enough to make things work. If the timing still isn't quite right, we could send an explicit event from the browser process -> to the main process letting it know that we're completely initialized.

As you mentioned, though, we might want to explicitly send a message up from the renderer process. We could do that somewhere here:

oni/browser/src/index.tsx

Line 139 in c01ae06

Performance.endMeasure("Oni.Start.Activate")

And have the main process listen for that event, in order to resolve the promise.

Hope that helps give you some ideas - let me know if you have any questions on the specifics. I think we can get this working though 👍

[akinsho]

@bryphe I've taken a different tack to what you suggested as I'm not sure I follow unfortunately specifically re the promise. What i've done is create an optional third argument for the window function which are delayed events, so that in the menu I pass the delayed events in case a window doesn't exist then call them once an oni.started event is received which I dispatch just under the performance measure ends. This currently works but may not fit with what you'd prefer.

As things stand the did finish load handler is inside the create window function but moreover the send("init") call would only be the beginning of the point where we need to wait since thats when oni only starts to load so resolving the process there would be problematic, we could do it a handler once oni's has started but the flow of resolving the promise in the handler and awaiting it in the menu fn I can't see how to implement that if my current tack isn't ideal could you post a snippet of pseudo-code re what you mean for the handler and then the awaiting

[bryphe]

This approach looks great to me - thanks @Akin909 !

[bryphe]

Looking at this - I'm wondering, will this be called for every window? In other words, if createWindow is called multiple times, will this event get registered multiple times, and send the delayedEvent multiple times to mainWindow?

I'm wondering if this should be ipcMain.once instead of ipcMain.on to protect against this. The existing rebuild-menu event might have the same issue, too, but it seems less critical. For this event, it seems like if we call createWindow with a delayed event, and then we call it again with no delayedEvent, the listener in the first createWindow call would send us that original delayedEvent again, to the new window.

[akinsho]

Ah @bryphe right on the money 👍 I had noticed this quirk yesterday and wasn't sure what it was and wracking my brain as to what to do did not realise there was a once method which fully resolves that so that delayed events are only called once (what I was getting was open file of a closed window click open new file which works but then close that window and click again and the same command would execute, using once however resolves that).

[bryphe]

What is the purpose of these changes to oni.sh? Are they related to the menu changes, or fixing a different bug? Just curious.

[akinsho]

@bryphe potential overzealousness on my part I use shellcheck to lint shell scripts and an error it flagged on multiple lines was that those variable were not quoted which helps prevent word splitting and globbing the link is to a wiki page where the rationale for that is explained, I initially made that change because I thought that script wasn't working and was what had led to the file args not being passed to oni on open. I'm more than happy to take them out since its not really (as far as I can tell related to the mac os no file args issue)

[bryphe]

What i've done is create an optional third argument for the window function which are delayed events, so that in the menu I pass the delayed events in case a window doesn't exist then call them once an oni.started event is received which I dispatch just under the performance measure ends. This currently works but may not fit with what you'd prefer.

Seems like a reasonable approach to me, thanks @Akin909 !

As things stand the did finish load handler is inside the create window function but moreover the send("init") call would only be the beginning of the point where we need to wait since thats when oni only starts to load so resolving the process there would be problematic

I think the way you have it set up is fine - the only concern I have is that since we are hooking the ipcMain.on for every createWindow call, multiple createWindow calls may be problematic. Consider the case where we call createWindow with a delayed event, and then createWindow again w/o a delayed event. In that case, it seems that the ipcMain.on(..) event would be triggered twice - in the original createWindow and in the second one, and the problem is the first hook would send the old delayedEvent object to the new window. So we'll need to fix that issue, as it may cause unpredictability when opening additional windows.

[bryphe]

Just tested this out - looks like it's working great. Thanks for reverting the oni.sh changes, if they don't change the functionality it's safer to keep as-is (especially because we don't have any tests around that script today!).

Looks like we may finally be able to close #484 with this 👍 Thanks for the contribution, @Akin909 ! Bringing it in now.