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

Fix save and restore cursor behavior #822

Merged
merged 3 commits into from
Jul 28, 2017

Conversation

Tyriar
Copy link
Member

@Tyriar Tyriar commented Jul 27, 2017

This broke in #717.

Fixes #818

@Tyriar Tyriar added this to the 2.9.0 milestone Jul 27, 2017
@Tyriar Tyriar self-assigned this Jul 27, 2017
@Tyriar Tyriar requested a review from parisk July 27, 2017 23:29
@coveralls
Copy link

coveralls commented Jul 27, 2017

Coverage Status

Coverage increased (+0.3%) to 70.702% when pulling 604959a on Tyriar:818_fix_save_restore_cursor into f36bd24 on sourcelair:master.

@coveralls
Copy link

coveralls commented Jul 27, 2017

Coverage Status

Coverage increased (+0.2%) to 70.567% when pulling 6ea7758 on Tyriar:818_fix_save_restore_cursor into f36bd24 on sourcelair:master.

Copy link
Contributor

@parisk parisk left a comment

Choose a reason for hiding this comment

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

ouch.

LGTM 🚀

@AndrienkoAleksandr
Copy link
Contributor

Ok, merge it, but seems that's not a fix, this is alternative of real solution...

@AndrienkoAleksandr
Copy link
Contributor

I am working on some tests on this topic, https://github.com/AndrienkoAleksandr/xterm.js/blob/fixFullResetTerminal/src/test/test.js#L1025
So, @Tyriar thank you for a "new testcase", I will think about how to implement some solution to cover all known testcases...

@Tyriar
Copy link
Member Author

Tyriar commented Jul 28, 2017

@AndrienkoAleksandr are you sure we want to save/restore the cursor when switching to/from the alt buffer? I assume it was originally commented out for a reason and I'm not sure xterm does this https://github.com/joejulian/xterm/blob/defc6dd5684a12dc8e56cb6973ef973e7a32caa3/misc.c#L4172

@Tyriar
Copy link
Member Author

Tyriar commented Jul 28, 2017

@AndrienkoAleksandr clarification on what's wrong with this would be good too, is it just the save/restore on buffer change?

@coveralls
Copy link

coveralls commented Jul 28, 2017

Coverage Status

Coverage increased (+0.2%) to 70.539% when pulling ff165e8 on Tyriar:818_fix_save_restore_cursor into cdcbf60 on sourcelair:master.

@Tyriar Tyriar merged commit 34f075a into xtermjs:master Jul 28, 2017
@Tyriar Tyriar deleted the 818_fix_save_restore_cursor branch July 28, 2017 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants