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

[RFC] Remove 'ttyfast' #1945

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@ghost

ghost commented Feb 5, 2015

refs #1045 #1051

ttyfast was enabled by default a while ago, and has apparently not created any issues (at least noted on GH). The amount of actual code related to it was tiny, so I just removed it.

@marvim marvim added the RFC label Feb 5, 2015

@tarruda

This comment has been minimized.

Show comment
Hide comment
@tarruda

tarruda Feb 5, 2015

Member

👍

Member

tarruda commented Feb 5, 2015

👍

@justinmk

View changes

Show outdated Hide outdated runtime/doc/term.txt Outdated
@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Feb 6, 2015

Squashed and repushed with the recommended changes.

ghost commented Feb 6, 2015

Squashed and repushed with the recommended changes.

@ghost ghost changed the title from [RFC] Remove 'ttyfast' & refs to 'fast terminals' to [RFC] Remove 'ttyfast' Feb 6, 2015

Michael Reed
Remove 'ttyfast'
refs #1045 #1051

This was enabled by default a while ago (#1051), and has apparently not
created any issues.  The amount of actual code related to it is tiny, so
it has been removed.
@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Feb 8, 2015

I left a comment in screen.c for a condition I was unsure about (I'm pretty sure comments on my branch aren't sent to people subscribed to this PR).

ghost commented Feb 8, 2015

I left a comment in screen.c for a condition I was unsure about (I'm pretty sure comments on my branch aren't sent to people subscribed to this PR).

@justinmk justinmk added the refactor label Feb 8, 2015

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Feb 8, 2015

Member

My first instinct wonders if this feature could still be useful. But if I understand it correctly, this is only useful on terminals that do not support a "scrolling region". After searching, I have the impression that most terminals support scrolling regions. So the presence of 'ttyfast' may do more harm than good for users, insofar as it adds a useless knob for users to waste time investigating.

So LGTM.

Member

justinmk commented Feb 8, 2015

My first instinct wonders if this feature could still be useful. But if I understand it correctly, this is only useful on terminals that do not support a "scrolling region". After searching, I have the impression that most terminals support scrolling regions. So the presence of 'ttyfast' may do more harm than good for users, insofar as it adds a useless knob for users to waste time investigating.

So LGTM.

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Feb 8, 2015

Member

@tarruda does the 'ttybuiltin' option make sense for Neovim to support after #1820?

Member

justinmk commented Feb 8, 2015

@tarruda does the 'ttybuiltin' option make sense for Neovim to support after #1820?

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Feb 8, 2015

Member

merged

Member

justinmk commented Feb 8, 2015

merged

@justinmk justinmk closed this Feb 8, 2015

@justinmk justinmk removed the RFC label Feb 8, 2015

@ghost ghost deleted the rm-ttyfast branch Feb 8, 2015

@tarruda

This comment has been minimized.

Show comment
Hide comment
@tarruda

tarruda Feb 10, 2015

Member

@tarruda does the 'ttybuiltin' option make sense for Neovim to support after #1820?

No, it doesn't. After #1820 I will create a PR to remove remaining dead code

Member

tarruda commented Feb 10, 2015

@tarruda does the 'ttybuiltin' option make sense for Neovim to support after #1820?

No, it doesn't. After #1820 I will create a PR to remove remaining dead code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment