Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better terminal list handling #20

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

musjj
Copy link
Contributor

@musjj musjj commented Aug 18, 2022

The state of the terminal list are often outdated by the time nvterm does things with it, so we should always verify it first. The verification is now done in the lower-level functions, so they are no longer needed in some functions and thus removed.
Also, fixed some outdated(?) references to non-existing tables.

musjj added 2 commits August 18, 2022 19:45
The state of the terminal list are often outdated by the time nvterm does things with it, so we should always verify it first.
@zbirenbaum
Copy link
Owner

I would have to test this pretty extensively. A lot of those verifications are done because of things like the terminal picker in the extensions repo calling the plugin in ways that aren't expected or causes edge cases. I used to just verify the low level functions, but there were tons of issue posts because people are doing unexpected things and getting an error message.

@musjj
Copy link
Contributor Author

musjj commented Aug 20, 2022

Yeah, it might need some testing, but I don't think it should break anything major. All verify_terminals does is just check if the buffers are still valid and if the windows are still open.

@Jee-vim
Copy link

Jee-vim commented Sep 11, 2022

Hey i love using this config
But how to open terminal when using nvchad?

@siduck
Copy link
Collaborator

siduck commented Sep 12, 2022

Hey i love using this config But how to open terminal when using nvchad?

alt i , alt h , alt v

leader v , leader h

@zbirenbaum
Copy link
Owner

zbirenbaum commented Oct 29, 2022

I agree the list handling is in need of improvement, but there's still the issue of having to use list_terms() from every function in this PR making it hard to maintain. I think a more permanent solution that makes it so updating the list manually isn't a concern at all within the api calls themselves is in order.

I'm thinking of modifying the API to be higher level, so that there is only one command called by users. something like nvterm.exec('new', <type>, <shell_override>) or `nvterm.exec('new', {type=, shell_override=<shell_override>}). Here's an example of what an implementation could look like for the variable args format.

nvterm.exec = function (fn, ...)
  terminals = list_terms() -- verification happens in list_terms
  nvterm_functions[fn](unpack(arg)) -- nvterm_functions table not exported so users can't cause unexpected behavior
end

The benefit of the variable arg format is that the existing functions wouldn't need to be changed much. The table of arguments is probably a bit more intuitive to most users, but would require them to actually know the names of the arguments to the functions which currently are not well documented, and thus would also require writing and maintaining docs.

What do you think @siduck @musjj

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants