Cross Oni Movement

[CrossR]

Potentially fixes #67.

I've not actually got 2 monitors, so I've written this such that it should work broadly moving across any Oni instances. Plus I don't think the screen number actually matters since I think if you have 2 1080p monitors, the top left of your primary is 0,0 and then the other ones are either -1920,0 or 1920,0, so this approach (ie using the current location) should work.

That said! I don't want to put a bunch of effort in based on vague assumptions I've had from reading StackExchange and GitHub issues, so if someone with 2 monitors could test it, that would be great. Currently, I've not bothered hooking up the movement keys, I'm just using the Ctrl+Shift+P menu to add Up, Down, Left, Right movements.

Assuming that works on multiple monitors, since it works for me on one at least:

• Check issue with windows being correctly emptied, since currently if you close a Window, this stops working due to an object being destroyed.
• Actually link this up up the to unhandled movement option.
• Move the code out of main and into a separate file (+ tidy it)
• Unit test it, and spend a little time seeing if CI tests are feasible.
• Deal with minimised windows? Ie currently if you have 3 windows of oni, with the middle minimised, the middle will open up again.
• Check the selection logic, and make sure it prioritises the smallest distance of the current movement.
• Move logic out of OniEditor.

If not, well at least I only spent an hour or so on this to avoid other work.

[bryphe]

This is awesome, @CrossR ! Once we have this I might never need to leave Oni... I'll put browser and terminal in one window and code in the other haha

so if someone with 2 monitors could test it, that would be great.

Tested it out, worked perfectly moving right/left between my two monitors 👍

Unit test it, and spend a little time seeing if CI tests are feasible.

Thanks for thinking about the tests!

The CiTests may be tricky - we don't have any tests that launch multiple Oni instances today, and in automation, we don't force a single instance of the main process (so this actually wouldn't work, unless we made further changes to add a setting to allow that). I think a unit test for the window-switching logic would be sufficient, and we could document a manual test case here: https://github.com/onivim/oni/blob/master/test/Manual.md#manual-test-cases

It would be nice to have a full CiTest eventually, but I'm not sure it's the right tradeoff of value vs time at the moment (up to you though).

Other bullet points look great - excited to have this in!

[codecov]

Codecov Report

Merging #1747 into master will increase coverage by 0.09%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1747      +/-   ##
==========================================
+ Coverage   42.37%   42.46%   +0.09%     
==========================================
  Files         163      165       +2     
  Lines        6252     6349      +97     
  Branches      881      889       +8     
==========================================
+ Hits         2649     2696      +47     
- Misses       3429     3477      +48     
- Partials      174      176       +2

Impacted Files	Coverage Δ
...owser/src/Services/Learning/Achievements/index.tsx	33.33% <0%> (-6.67%)	⬇️
browser/src/Utility.ts	54.54% <0%> (-1.98%)	⬇️
...vices/Learning/Achievements/AchievementsManager.ts	78% <0%> (-1.6%)	⬇️
browser/src/neovim/NeovimAutoCommands.ts	5.71% <0%> (-0.35%)	⬇️
browser/src/neovim/NeovimInstance.ts	5.7% <0%> (-0.09%)	⬇️
browser/src/Store.ts	23.8% <0%> (ø)
...g/Achievements/AchievementNotificationRenderer.tsx	44.11% <0%> (ø)
browser/src/Editor/Editor.ts	72.72% <0%> (+0.85%)	⬆️
browser/src/neovim/NeovimBufferUpdateManager.ts	45.09% <0%> (+35.34%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6d9cd90...7aa6bc6. Read the comment docs.

[CrossR]

It looks like the error I get isn't due to the windows list not being correctly updated on close, but rather when the initial Oni window (that is, the first Oni window to be opened) is closed, the windows list gets deleted entirely.

Need to look into why that is happening, and how to fix it...

[CrossR]

It seems that for some reason, when the main Window is closed, the first item in the windows array becomes unreferenced...for some reason. Which then causes issues since I need to check over all the values in that array. I'll poke around it for a bit tomorrow, since I've not had that much time today to check.

[bryphe]

It seems that for some reason, when the main Window is closed, the first item in the windows array becomes unreferenced...for some reason.

Ah, interesting. I think I see one problem (it's really an existing bug, just easier to hit now with this change).

The repro I did was open two Oni instances:

• Focus the second instance
• Close the first instance
• <C-w><C-h> to the boundary
• Hit this exception:

It looks like the issue is this line here:

    mainWindow.on("closed", () => {
        // Dereference the window object, usually you would store windows
        // in an array if your app supports multi windows, this is the time
        // when you should delete the corresponding element.
        windows = windows.filter(m => m !== mainWindow)
        mainWindow = null
    })

The problem is that, mainWindow always gets set to the newest editor. So when we create the second window, mainWindow now points to that second window. Then, when we close the first window, we filter out the second window, leaving the now-disposed first window.

A quick fix is to hold a reference to the current window in the closure:

const currentWindow =  mainWindow = new BrowserWindow({

And then use that in the filter:

    // Emitted when the window is closed.
    mainWindow.on("closed", () => {
        // Dereference the window object, usually you would store windows
        // in an array if your app supports multi windows, this is the time
        // when you should delete the corresponding element.
        windows = windows.filter(m => m !== currentWindow)
    })

It might be good eventually to get rid of the mainWindow. There's nothing really special about the first window we create - it might be better to replace the places that use mainWindow with the currently focused window, or the first window in the windows array.

[bryphe]

Solid! Thanks for fixing this 👍

[CrossR]

I've added tests for all the easily testable parts. The remaining function that hasn't been tested doesn't return anything, so we'd need a CI test that can check the focus of a window, which sounds pretty awkward.

Only thing left here I think is I want to check is that where I've added the subscription makes sense in OniEditor?

Oh and the original ticket mentions adding commands for Expand Up/Down etc, but I think that is distinct enough to be done in a separate PR. Could probably also do with making the movement into an action that users can bind, such that a user can use it without having to move to the edge of the window with their own keybinds.

[bryphe]

Very cool! Great set of unit tests covering this functionality.

[bryphe]

Thanks for setting this up! 💯

[bryphe]

Just saw your comment - there is one problem with the location here. In the 'multiplexing' scenario (when editor.split.mode is 'oni'), we will potentially have multiple OniEditor instances - so that means this would get fired multiple times.

One option would be to plop this code in browser/src/index.tsx, after we've created the WindowManager. Or add an activate method in MultiProcess, like we have for some of the other services, pass in the windowManager as a dependency, and wire up the event there.

[bryphe]

I've added tests for all the easily testable parts.

Awesome! Thanks for wiring up unit tests for our main process - big step forward to have some of that code covered under test. This is a great example of a change that improves the quality of the code while implementing new functionality.

The remaining function that hasn't been tested doesn't return anything, so we'd need a CI test that can check the focus of a window, which sounds pretty awkward.

Agree 100% - I don't think a CiTest is necessary for this.

Only thing left here I think is I want to check is that where I've added the subscription makes sense in OniEditor?

Ah ya, good catch - there might be an issue in the 'multiplexing' case - I left a more detailed comment on the file. But we probably will want to move it to a place that only gets initialized once.

Oh and the original ticket mentions adding commands for Expand Up/Down etc, but I think that is distinct enough to be done in a separate PR. Could probably also do with making the movement into an action that users can bind, such that a user can use it without having to move to the edge of the window with their own keybinds.

Yep, makes sense to have that be a separate PR 👍

Nice work! 💯

[CrossR]

Assuming no failures on the CI side of things, this should be done now.

[bryphe]

Awesome! Code changes look great 👍 Approving now