Skip to content

Conversation

@claudiocabral
Copy link
Contributor

This PR adds the functionality requested on #37.
If opened inside tmux or screen or if vim does not have the :term command,
it defaults to the old behaviour

@capocasa
Copy link
Member

That's some seriously cool stuff! Love the fallback to the old behavior.
I don't have my dev environment for SC right now but I'll be looking into it! Will also happily merge if reported working by others, especially no regressions.

@claudiocabral
Copy link
Contributor Author

I'll gladly fix any bugs people find :)
Have been using it mostly with neovim and fine tuning it a bit to work in a way that i like, gonna play a concert with it tonight.

@capocasa
Copy link
Member

capocasa commented Nov 6, 2019

Works like a charm on neovim, real smooth.

On vim8, an additional horizontal split is added to the output window, mirroring the buffer that was open when SClangStart was launched. This is, I'm afraid, a blocker for merge.

It is also a requirement for merge to add a configuration variable that defaults to off. When a certain maturity has been achieved, we can discuss enabling it by default with the community. Documenting it prominently is fine.

@claudiocabral
Copy link
Contributor Author

I can reproduce the weird behaviour you found, I'll work on it on the weekend, shouldn't be too hard.
For this case, I don't think we need a variable since:

  1. if your vim version doesn't support it, it defaults to normal behaviour
  2. if you open it inside tmux/screen (expected usage without this feature), it defaults to the old behaviour.
  3. the old behaviour outside tmux/screen is plain weird in my opinion. We lunch it in the background, and I'm not that sure that it actually does anything.

All that said, as I was writing this I realized that we could have a varible with the following behaviour:

g:sclangUseTerminalBuffer == 'never' (default) -> old behaviour
g:sclangUseTerminalBuffer == 'standalone' -> use it if no multiplexer
g:sclangUseTerminalBuffer == 'always' -> use it even if inside a multiplexer

what do you think? I don't mind the values themselves, could also be "no", "yes", and "sometimes" or any other suggestions

@capocasa
Copy link
Member

capocasa commented Nov 7, 2019

Great!!! That's real cool. Thanks for contributing!

I'm positive that the configuration option is really really important- the old behavior opens up a new window with sclang. Works fine, and you don't need to know or care about :term support, tmux or vim splits (and it's actually super userful when using a tiling window manager). I'm sure lots of people will love the new behavior, and my opinion is that it's the future of scvim, but people need to stay in control of whether and when they get it or even learn about it. That's why we use vim in the first place, isn't it?

The config option's just fine- I'd suggest shortening it, perhaps g:scTerminalBuffer? I'd also suggest to scrap standalone mode- keep it simple. (These are just suggestions, not requirements)

@claudiocabral
Copy link
Contributor Author

I fixed the windowing issue, but further there's still work to be done. Specifically, vim doesn't seem to close its terminal as gracefully as neovim, so I need to add proper clean up as well.

@claudiocabral
Copy link
Contributor Author

Should be working properly with vim now, at least it did on my computer.
Let me know if you find any other bugs!

@capocasa
Copy link
Member

Fantastic, works a charm. Thank you very much for your great work!!!

Could you kindly add documentation to the README.md? One liner describing the option is fine, or elaborate a little if you think it make sense.

I was thinking of enabling the internal terminal even with screen/tmux when g:TerminalBuffer="on", do you think that would make sense?

Copy link
Member

@capocasa capocasa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please describe configuration option in readme

@claudiocabral
Copy link
Contributor Author

It makes sense, to be honest I just forgot to include it in the code.
I'm travelling this week, but I'll try to squeeze in the documentation and that modification before that. Looking forward to get this merged!

@capocasa
Copy link
Member

I just checked to confirm if we really can't do without disabling by default, because by default scvim didn't open a window on executing SClangStart for me either. However, that only happens when there is no x-terminal-emulator program on linux systems. Usually it exists, but this should be fixed .

In order to avoid dragging out the merge more than necessary I would suggest to stick to the default-off behavior as planned, and look at the defaulting behavior from different angles in a seperate PR via #62.

Finally, what would you think of scrapping the new flag and using ":term" as terminal command instead to use internal? Not sure if it's more or less clear and simple.

@claudiocabral
Copy link
Contributor Author

I didn't manage to make the changes before travelling and ended not having time until now. Option description was added to README.md and vim now checks if the user wants to use :term before checking for multiplexers.

@capocasa capocasa merged commit d80cf6e into supercollider:master Dec 2, 2019
@capocasa
Copy link
Member

capocasa commented Dec 2, 2019

Looks great! Thank you very much for your efforts, Claudio!

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.

2 participants