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

Dickey2 #2

Merged
merged 2 commits into from
May 16, 2018
Merged

Dickey2 #2

merged 2 commits into from
May 16, 2018

Conversation

ThomasDickey
Copy link
Contributor

No description provided.

ThomasDickey and others added 2 commits May 7, 2018 18:21
checksums.  Along with that, it dropped a comment which seemed to imply that
iTerm2 handled both the older/incorrect parameter order and the correct one.
This change restores the correct order for xterm.
@gnachman
Copy link
Owner

I thought the mixup of the DECRQCRA args was fixed in patch 315? Your original change to the readme in commit 2b69458 changed it to say:

--disable-xterm-checksum-bug
xterm's implementation of DECRQCRA (fixed in patch 315, early 2015) contained a
bug.  DECRQCRA is essential to these tests.  By default, a workaround for the
bug is used in case an old version of xterm is tested.

@ThomasDickey
Copy link
Contributor Author

sure - I changed the wording to indicate when it was fixed, but didn't change the logic. It's a double-negative, so if you remove the test, you still need the assignment.

@gnachman
Copy link
Owner

The spec for DECRQCRA defines it as:

CSI Pid ; Pp ; Pt ; Pl ; Pb ; Pr * y

The code with this PR will send Pp first and Pid second only for xterm. That was necessary when xterm had those values swapped, but should not be needed any longer.

Since the value of Pp passed to esccmd.DECRQCRA is always 0, things will appear to work until an exception is raised after sending DECRQCRA and the Pid becomes needed to ignore a response code that a subsequent test should ignore.

@ThomasDickey
Copy link
Contributor Author

I see the problem now: shortly after applying the fix you sent, I noticed the same issue with DECCKSR, and (incorrectly) undid your fix. So... the option is still needed :-(

1 similar comment
@ThomasDickey
Copy link
Contributor Author

I see the problem now: shortly after applying the fix you sent, I noticed the same issue with DECCKSR, and (incorrectly) undid your fix. So... the option is still needed :-(

@ThomasDickey
Copy link
Contributor Author

I'll make an update which detects the xterm patch number to handle the special case, in the next day or so.

@gnachman
Copy link
Owner

I'll merge this since it will fix the tests for now, and just send another PR once the patch is in. I'd rather not keep the flag around since it adds needless complexity; anyone who is running these tests should use the latest code of whatever terminal they're testing.

@gnachman gnachman merged commit b684c26 into gnachman:master May 16, 2018
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