[BUG] Terminal id collisions with custom terminals

[willothy]

Is there an existing issue for this?

• I have searched the existing issues

Current Behavior

When creating multiple custom terminals with Terminal:new(), ids are assigned automatically if they're not provided.

However, the next_id function only checks existing terminals, not ones that have been newly created (not yet spawned).

This leads to terminals having identical IDs, and therefore toggleterm losing track of one of them when the other is opened.

Both will remain open, but the previous is removed from the terminals dict and therefore is inaccessible to API functions such as get_all, get, etc.

Expected Behavior

Created terminals should never overwrite each other; Id conflicts should be handled properly.

Steps To Reproduce

local term = require("toggleterm.terminal")
local Terminal = term.Terminal

local one = Terminal:new()
local two = Terminal:new()
one:open()
two:open()

vim.print(#term.get_all(true)) -- should return 2, but returns 1

Environment

- OS: Linux
- neovim version: 0.10.0
- Shell: zsh

Anything else?

Happy to contribute a PR for this, but I think it needs some discussion first. I know that the way the next_id function works is done that way to keep id numbers low, for ease of use. This is good and should be kept, something like a counter that increments for each new terminal is out of the question.

So, my idea is using an additional cache set, like terminals, but for reserved terminals that have been initialized but not spawned. This set would be checked anywhere terminals is, to ensure that id collisions never occur.

Another option could be only assigning terminal ids to custom terminals when they're spawned.

See #488, assigning terminal ids on spawn was a very simple change and does not affect terminals created with a specific id.

Would appreciate some input :)

[willothy]

@akinsho mind reopening since #488 changes were reverted? Sorry about that, didn't mean to cause another issue haha :) I'll look into an alternate solution.

[kevinm6]

@willothy I didn't dig into the code, but maybe one possible solution could be to provide a custom id for custom terminals.
Since they are special, maybe the ids should IDK, starting from 100 and increments on new spawns! But this should be a choice of @akinsho that already made an awesome plugin I use almost every time.
From my usage, I have just 5 custom terminals, that I use sometimes, but not always.

[willothy]

@willothy I didn't dig into the code, but maybe one possible solution could be to provide a custom id for custom terminals. Since they are special, maybe the ids should IDK, starting from 100 and increments on new spawns! But this should be a choice of @akinsho that already made a awesome plugin I use almost every time. From my usage, I have just 5 custom terminals, that I use sometimes, but not always.

That could work, but since regular and custom terminals use the same class, differentiating between custom and regular terminals would be difficult. Ideally we'd find something that keeps IDs consistent between custom and standard terminals as well so that you can still toggle custom terminals by id without having to type a bunch of characters. Maybe using some sort of "reserved" id list could work like I initially suggested. I'll try these ideas out and see how it goes.

Giving custom terminals string IDs instead of number IDs in config works as well, but kinda defeats the purpose of having an id at all as it can't be accessed by count.

[akinsho]

fixed by #490