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

allow Ctrl-D to return eof #2

Closed
wants to merge 1 commit into from

Conversation

TobyGoodwin
Copy link
Contributor

Thanks for your work on editline: it's good to see this library still maintained.

I'm puzzled as to what's going on in tty_special.

if (is_ctl_map_key(c))
   return CSdispatch;

if (c == rl_eof && rl_point == 0 && rl_end == 0)
    return CSeof;

return CSdispatch;

The second test here is for the terminal's eof character at the start of a line. But this test comes after the is_ctl_map_key test. Which means that if my eof character is a control character (which it is, being Ctrl-D) I cannot type it. (Instead editline just beeps at me, since there's no character to delete.)

It seems to me that the two tests should be swapped... which then means that the is_ctl_map_key test is redundant, since we will always return CSdispatch if we reach this point. And since this is the only place that is_ctl_map_key is called, that can go too.

Any thoughts?

@TobyGoodwin
Copy link
Contributor Author

By the way, I couldn't shake the feeling that the code must once have looked like it does in my patch (without is_ctl_map_key etc), and indeed that is the case. Essentially my PR reverts one of the changes in 111fc5e, which mainly seems to be about something else. @troglobit can you recall why you did this?

@troglobit
Copy link
Owner

Hi Toby and thanks for the patch!

The original reason for 111fc5e was to fix a bug in our CLI at work where pressing Ctrl-D crashed it. Unfortunately I blindly imported that fix to the official editline repo without evaluating it first.

Thank you for cleaning up!

@troglobit
Copy link
Owner

I've merged your pull request manually, as a2bc89d, and added configure script switches
to implement the behavior I needed myself.

@troglobit troglobit closed this Apr 6, 2015
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