Spawn terminal job using a list (and other jobstart() opts)

[jedrzejboczar]

Would it be possible to use a list as terminal command instead of using a string? The jobstart() api it is possible to supply a list, which is often convenient when calling a process, because all the argument quoting will be done by jobstart().

I looked at the code of task:__spawn() and it seems that every command gets a comment with terminal id appended, e.g. ;#toggleterm#1. I was wondering what is the reason for doing this? Are these id/filetype used somewhere else? Or would it be safe to remove it? This way it would be possible to just pass shell directly.

I guess that this is also the reason why https://github.com/akinsho/toggleterm.nvim/issues/184 happens. In local cmd = self.cmd or config.get("shell") a new shell invocation is created, but later termopen also starts a shell process, so there are 2 shells.

And a follow up question: would it be possible to pass other opts to jobstart() api. The interesting option is env which can be helpful for starting jobs that e.g. need to first load some environment like when using Python virtual environments. Then we could just pass PYTHONPATH via env instead of doing things like cmd = "sh -c 'source venv/bin/activate && python script.py'".

I could create a PR, but wanted to hear your opinion first.

[akinsho]

@jedrzejboczar so it can't be instead of since that is a major breaking change for this plugin, it would have to be also taking a list the same way that jobstart can take a string or a list. In either case, I'd keep the toggleterm ID

Regarding the #toggleterm#1 i.e. the toggleterm ID, this is actually very important from my perspective because essentially it's the only way to identify a terminal that was created by toggleterm, you can use buffer variables, but those don't persist in a neovim session. Whereas the title of a terminal always does, so we can always recreate it, and it's essentially the basis for the resurrect functionality and a lot of other things. So in short, it definitely cannot be removed without a large refactor, which I'm not keen on and would be destructive.

I guess that this is also the reason why https://github.com/akinsho/toggleterm.nvim/issues/184 happens. In local cmd = self.cmd or config.get("shell") a new shell invocation is created, but later termopen also starts a shell process, so there are 2 shells.

I'm not sure if I see where the duplication is happening, but I do want to fix this, can you clarify what you mean? config.get doesn't create a shell, so not sure what you mean.

Also, re. adding more arguments to jobstart this sounds perfectly fine to me, no one else has ever asked for that but seems perfectly fine to add that as well 🤷🏾‍♂️

[jedrzejboczar]

@jedrzejboczar so it can't be instead of since that is a major breaking change for this plugin, it would have to be also taking a list the same way that jobstart can take a string or a list. In either case, I'd keep the toggleterm ID

I actually meant also, just used unfortunate words :) But the thing is, when using a list you wouldn't be able to pass ;#toggleterm#1 to the command. This is due to the difference between String and List described in jobstart() api. When you specify a list there is not shell in between, so Vim just executes the program args[0] and all other arguments go to that program. Compare:

vim.fn.termopen('echo "hello"; #toggleterm#1')
-- hello
-- 
-- [Process exited 0]

with:

vim.fn.termopen({'echo', 'hello', '#termopen#1'})
-- hello #termopen#1
--
-- [Process exited 0]

While echo will just print that argument, most other programs would fail to parse command line arguments.

And the above may be a reason of https://github.com/akinsho/toggleterm.nvim/issues/184, because:

If {cmd} is a String it runs in the 'shell', like this: :call jobstart(split(&shell) + split(&shellcmdflag) + ['{cmd}'])

so it would execute e.g. /bin/bash -c '/bin/bash; #toggleterm#1'. But I tried to test it on my machine and couldn't find 2 processes being spawned, just 1, so I am not sure if my understanding is true.

So do you thing it would be ok to allow cmd to be a list, and then we just wouldn't add the ;#toggleterm#1 to arguments? If I understand correctly this would prevent the ressurect functionality, but whoever passes a list would just have to keep this in mind?

The reason why I am asking for this and why I created https://github.com/akinsho/toggleterm.nvim/pull/218 is that I am writing a plugin that allows to spawn multiple tasks, selected via Telescope and discovered from JSON config files. These tasks are spawned in the background and you can then open their terminals via another Telescope picker. I already have a working demo but it requires https://github.com/akinsho/toggleterm.nvim/pull/218. I won't have time to work on this for the next 2-3 days, but then I want to clean things up for a working version.

[akinsho]

@jedrzejboczar thanks for the detailed explanation, 🤔 so this whole method of appending the comment and using it in the title of the window I ~stole~ borrowed from neoterm not sure if there's actually a better way to keep track of which buffers this plugin created that persists across sessions, maybe a g:ToggleTerms but this will require the user to have sessionopts+=globals.

I guess plugins using toggleterm in this way are less interested in re-identifying across session, although this mechanism isn't just used per session but also across the plugin in the terms.identify function. Which is used for other basic identification functionality, not just resurrection. I think you'll find that not having this identifier will cause other types of breakages.

I think the mechanism might need to be addressed entirely rather than just allowing it to be ignored for this use case since a bunch of different things will probably fail to work

[jedrzejboczar]

There is probably no good solution to store this information via mksession, at least I cannot think of any. One option is to offload this responsibility to session management plugins. A plugin that I've written stores session data in JSON, so the whole Terminal table could be just stored on before_save and retrieved on after_load. But of course it's sub-optimal and it would probably be best to have a builtin solution.

But during runtime, it should be easier to identify terminals by just keeping track of them in a Lua table { buf = term, ... }. This way it would be possible to use the current version of terms.identify only for the resurrection functionality. And resurrection would just not be available for terminals spawned using a List.

I actually thought that using a List to spawn terminals would involve less work. I still think that it would be nice to have, but I wouldn't say it's an important feature (and the cost of spawning one more shell process is minimal). Now, when I think about it, an option to pass env would be a more useful feature than passing commands as List, so maybe I would think about it first.

[jedrzejboczar]

A random note: it just came to me that the solution would be to use a custom URI scheme for toggleterm terminals. So toggleterm buffers could use toggleterm://{id}:cmd or something similar to neovim terminal emulator which uses term://{cwd}//{pid}:{cmd}. And in BufReadCmd the URI would be parsed and a terminal job started, just like vim-fugitive populates it's fugittive://*. This seems like a cleaner solution than the current one but I'm not sure how hard to implement it would be.

[akinsho]

@jedrzejboczar the skin used by the neovim terminal is actually not under my control. It's chosen on purpose and is used by neovim to restore terminals when you reload a session so I can't actually change its structure which is why I currently just append a comment to the name of the terminal which includes the toggle term ID

[jedrzejboczar]

@akinsho Sure, I know that the current term:// is the default and neovim restores the terminals. My suggestion was that it should be possible to instead use toggleterm:// - in such case when neovim restores session it would first create a buffer with the same name as the saved buffer without creating a terminal, but there would be an autocmd on the pattern toggleterm://* (defined by this plugin, probably some event like BufNew but I'm guessing), that would reconfigure the buffer to be a terminal, so it would do the job that neovim normally does automatically. But it is just an idea, it might be too problematic to implement and I'm not saying it's a good idea, just an idea that could potentially solve this issue.

[akinsho]

I think terminals have to follow neovim's native format, changing their names seems like it would cause a bunch of bugs or issues I'd definitely rather stay away from messing with something that has significance internally and could lead to unpredictable weirdness