-
Notifications
You must be signed in to change notification settings - Fork 301
Conversation
browser/src/Editor/NeovimEditor.tsx
Outdated
// If the current buffer has modifications pending, then open in a new tab or split. | ||
// If the current buffer had no modification and is a new file, open in the current buffer. | ||
|
||
this._neovimInstance.command("if bufname('%') != ''\n" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we need extensive logic, it might be worth creating a function for this in vim\core\oni-core-interop\plugin\init.vim
. Something like:
function OniOpenFile(strategy, file)
if bufname('%') !== ''
exec a:strategy . a:file
elseif &modified
exec a:strategy . a:file
else
exec ":e " . a:file
endif
endfunction
You can then call this function by executing:
this._neovimInstance.callFunction("OniOpenFile", [message, files[0]])
A couple of benefits:
- Easier to read than the string concatenation
- You can debug quickly by running
:OniOpenFile
in ex mode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, I was wondering if there was an easier place to put something like this, since it doesn't read very nicely how I had it.
@@ -57,16 +59,16 @@ const buildMenu = (mainWindow, loadInit) => { | |||
{ | |||
label: 'Split Open...', | |||
click: (item, focusedWindow) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe to simplify, we could just handle the Tab
case with this PR? That case seems more compelling to me, as opening a bunch of files in a split would be somewhat unwieldy. And it looks like the Tab Open
case works well - I think this parallels pretty well with functionality in something like VSCode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fair, when I use splits I find it more intuitive to use the Fuzzy File finder or similar to open a new split.
browser/src/Editor/NeovimEditor.tsx
Outdated
"endif") | ||
|
||
// Open any subsequent files in a tab or split. | ||
for (let i = 1; i < files.length; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you mentioned, we'd want to chain the promises. I believe Neovim will execute these in-order, but it's still safer to wait for each command to complete. Chaining the promises in a loop gets a little messy, but we could leverage async/await potentially:
const openFiles = async (message: string, files: string[]) => {
// Open the first file.
// If the current buffer is named, then open in a new tab or split.
// If the current buffer has modifications pending, then open in a new tab or split.
// If the current buffer had no modification and is a new file, open in the current buffer.
await this._neovimInstance.command("if bufname('%') != ''\n" +
" exec \"" + message + normalizePath(files[0]) + "\"\n" +
"elseif &modified\n" +
" exec \"" + message + normalizePath(files[0]) + "\"\n" +
"else\n" +
" exec \":e " + normalizePath(files[0]) + "\"\n" +
"endif")
// Open any subsequent files in a tab or split.
for (let i = 1; i < files.length; i++) {
await this._neovimInstance.command("exec \"" + message + " " + normalizePath(files[i]) + "\"")
}
}
ipcRenderer.on("open-files", (_evt: any, message: string, files: string[]) => {
openFiles(message, files)
})
There's still a hanging promise at the end of the chain, and ideally we'd queue things up in a more disciplined way (nothing stops another area of the code from calling command!). This could even be handling internally by Neovim instance, by queuing or chaining the commands.
Tried it out and it works great for tabs! Thanks for putting this together and submitting the PR 😄 Regarding
This could actually be made a lot nicer with Hope that helps! |
@@ -283,6 +283,21 @@ export class NeovimEditor implements IEditor { | |||
} | |||
}) | |||
|
|||
const normalizePath = (fileName: string) => fileName.split("\\").join("/") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that you refactored this out - thanks!
Thanks for making the changes. Just tried it out and its working great! Let me know if you were planning on making any other changes - otherwise I'll go ahead and bring this in 👍 |
Bringing this in - thanks for the contribution, @CrossR ! |
Just putting this here so anyone can have a glance over as I'm doing it.
Currently, I'm calling some VimL to work out if the first file should be opened in the current buffer, or if it should be opened in a tab / split. The other files are then just opened in turn.
Works fine for tabs (at least how I expect it to, but I'm fine with changing it if needed), splits have an issue that I think is to do with buffer change issues...
A smarter way of doing this may be to use the
getCurrentBuffer()
command, and then do some checks against that instead? Think that would require getting the current filename, the changed state, and which buffer it is.Looks like the functions for that are in the Buffer.ts file, but I can't work out how they link to nvim.