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

[RDY] Remove unused USE_TERM_CONSOLE ifdefs #1015

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@fornwall
Contributor

fornwall commented Jul 31, 2014

This is unused after dropped amiga and msdos support.

Remove unused USE_TERM_CONSOLE ifdefs
This is unused after dropped amiga and msdos support.
@aktau

This comment has been minimized.

Show comment
Hide comment
@aktau

aktau Jul 31, 2014

Member

LGTM

Member

aktau commented Jul 31, 2014

LGTM

@fornwall fornwall changed the title from Remove unused USE_TERM_CONSOLE ifdefs to [RDY] Remove unused USE_TERM_CONSOLE ifdefs Jul 31, 2014

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Aug 2, 2014

Member

In Vim, USE_TERM_CONSOLE is also defined in os_win32.h. But, rather than remember to deal with that for our Windows build, I just did a full review of USE_TERM_CONSOLE and term_console in the Vim source, and these affect only MSDOS and Amiga. The only relevance they have to Neovim is the default setting of 'ttyfast'. Anyone correct me if I'm wrong, but I think we should default 'ttyfast' to ON for everything.

This means we can also remove:

  • vim_is_fastterm
  • vim_is_vt300

Also need to update the doc for 'ttyfast'.

Member

justinmk commented Aug 2, 2014

In Vim, USE_TERM_CONSOLE is also defined in os_win32.h. But, rather than remember to deal with that for our Windows build, I just did a full review of USE_TERM_CONSOLE and term_console in the Vim source, and these affect only MSDOS and Amiga. The only relevance they have to Neovim is the default setting of 'ttyfast'. Anyone correct me if I'm wrong, but I think we should default 'ttyfast' to ON for everything.

This means we can also remove:

  • vim_is_fastterm
  • vim_is_vt300

Also need to update the doc for 'ttyfast'.

@justinmk justinmk added the refactor label Aug 2, 2014

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Aug 2, 2014

Member

Comment on vim_is_fastterm also says this:

This should include all windowed terminal emulators.

I assume the linux console (reached by ctrl-alt-F1) is considered a non-windowed terminal emulator. I've never had a problem using ttyfast in the linux console. I think it makes sense to set ttyfast by default, and if anyone has trouble with Neovim in a non-windowed terminal the option is still there to un-set it.

edit: Vim also defaults to &ttyfast=0 in some ssh sessions.

Member

justinmk commented Aug 2, 2014

Comment on vim_is_fastterm also says this:

This should include all windowed terminal emulators.

I assume the linux console (reached by ctrl-alt-F1) is considered a non-windowed terminal emulator. I've never had a problem using ttyfast in the linux console. I think it makes sense to set ttyfast by default, and if anyone has trouble with Neovim in a non-windowed terminal the option is still there to un-set it.

edit: Vim also defaults to &ttyfast=0 in some ssh sessions.

@fornwall

This comment has been minimized.

Show comment
Hide comment
@fornwall

fornwall Aug 2, 2014

Contributor

I assume the linux console (reached by ctrl-alt-F1) is considered a non-windowed terminal emulator. I've never had a problem using ttyfast in the linux console. I think it makes sense to set ttyfast by default, and if anyone has trouble with Neovim in a non-windowed terminal the option is still there to un-set it.

I agree, setting ttyfast to true by default and perhaps consider it deprecated should be the way forward.

That should probably be done in a separate commit/pull request from this one? I can do it after this one is merged!

Contributor

fornwall commented Aug 2, 2014

I assume the linux console (reached by ctrl-alt-F1) is considered a non-windowed terminal emulator. I've never had a problem using ttyfast in the linux console. I think it makes sense to set ttyfast by default, and if anyone has trouble with Neovim in a non-windowed terminal the option is still there to un-set it.

I agree, setting ttyfast to true by default and perhaps consider it deprecated should be the way forward.

That should probably be done in a separate commit/pull request from this one? I can do it after this one is merged!

justinmk added a commit that referenced this pull request Aug 7, 2014

Remove unused USE_TERM_CONSOLE ifdefs #1015
This is unused after dropped amiga and msdos support.
@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Aug 7, 2014

Member

Merged

Member

justinmk commented Aug 7, 2014

Merged

@justinmk justinmk closed this Aug 7, 2014

@fornwall fornwall deleted the fornwall:remove-term-console branch Aug 7, 2014

fmoralesc added a commit to fmoralesc/neovim that referenced this pull request Aug 19, 2014

Remove unused USE_TERM_CONSOLE ifdefs neovim#1015
This is unused after dropped amiga and msdos support.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment