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

Small compatibility fixes #27

Closed
wants to merge 5 commits into from
Closed

Small compatibility fixes #27

wants to merge 5 commits into from

Conversation

dwmw2
Copy link

@dwmw2 dwmw2 commented Dec 3, 2021

  • Echo input from non-tty stdin
  • Fix Ctrl-R key handling
  • Simple build fixes

@dwmw2 dwmw2 force-pushed the master branch 4 times, most recently from b81bfda to ac8b56a Compare December 3, 2021 18:17
@msteveb
Copy link
Owner

msteveb commented Dec 13, 2021

Thanks. C++ support seems fine.

What is the rationale for echoing input. If I do this I get double output. One from the terminal and one from the echo.

$ TERM=dumb ./jimsh
Welcome to Jim version 0.80
. puts hello
puts hello
hello

Regarding reverse-i-search, really - is that how it is supposed to work? How to go forward again after skipping past the one you want? I suppose we have to be compatible, but I prefer mine :-(

@dwmw2
Copy link
Author

dwmw2 commented Dec 13, 2021

What is the rationale for echoing input. If I do this I get double output. One from the terminal and one from the echo.

Ah, as noted in the commit comment, this was intended for the case where it isn't a tty. I hadn't spotted that enableRawMode() is also returning failure (and even setting errno = ENOTTY) for the case of unsupported $TERM too.

I'm a little confused by the errno setting... can I make enableRawMode() set errno = ENOTTY only when it literally isn't a tty, and set a different errno in other cases.... or in fact, is anyone even looking? Why set errno at all?

Or just do this perhaps...

--- a/linenoise.c
+++ b/linenoise.c
@@ -1851,7 +1851,7 @@ char *linenoise(const char *prompt)
         printf("%s", prompt);
         fflush(stdout);
         sb = sb_getline(stdin);
-        if (sb) {
+        if (!isatty(current.fd) && sb) {
             printf("%s\n", sb_str(sb));
             fflush(stdout);
         }

@dwmw2
Copy link
Author

dwmw2 commented Dec 13, 2021

Regarding reverse-i-search, really - is that how it is supposed to work? How to go forward again after skipping past the one you want? I suppose we have to be compatible, but I prefer mine :-(

Honestly, this is one of those cases where my fingers' muscle memory knows more than my conscious brain. They definitely want that repeated Ctrl-R to keep going backwards.

It turns out that Ctrl-S is the 'search forwards' command, which doesn't actually work in practice unless you do stty -ixon otherwise it gets consumed. But I'll go and implement that; thanks for the reminder.

@msteveb
Copy link
Owner

msteveb commented Dec 13, 2021

Or just do this perhaps...

--- a/linenoise.c
+++ b/linenoise.c
@@ -1851,7 +1851,7 @@ char *linenoise(const char *prompt)
         printf("%s", prompt);
         fflush(stdout);
         sb = sb_getline(stdin);
-        if (sb) {
+        if (!isatty(current.fd) && sb) {
             printf("%s\n", sb_str(sb));
             fflush(stdout);
         }

Yes. That will probably do. Use of errno is probably lost in the mists of time (not literally - it would have come from original linenoise).

@msteveb
Copy link
Owner

msteveb commented Dec 13, 2021

With the ^S change, I think this all looks good but will take some time before I can test thoroughly and merge (busy time leading up to Christmas) so I'll get to it in due course. Thanks.

A couple of functions should be static as they have no prototypes in
the header files.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Pressing Ctrl-R again while in reverse-i-search should search back
further, while up/down cursor should navigate to the adjacent lines
in history rather than searching for matches.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
As noted in https://superuser.com/questions/610980/ you can't actually
use this (in readline or linenoise) unless you disable XON/XOFF flow
control processing in the terminal with 'stty -ixon'.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
@dwmw2
Copy link
Author

dwmw2 commented Sep 7, 2022

Branch updated and rebased, updated the list of keybindings that you've added in the meantime. Also added @metinkaya's allocation checks from dwmw2#1

@msteveb
Copy link
Owner

msteveb commented Sep 9, 2022

Thanks. I will get to this soon but I won’t be merging the allocation checks. IMHO checking for allocation failure achieves no useful purpose on modern operating systems.

@msteveb
Copy link
Owner

msteveb commented Sep 18, 2022

I merged the earlier commits and made a few small changes to ctrl-r/s
Take a look at https://github.com/msteveb/linenoise/tree/readline-compat-ctrl-r and if ok I will merge it

@msteveb
Copy link
Owner

msteveb commented Mar 7, 2023

I didn't get a response, so I merged that branch. Any problems can be raised in a new issue.

@msteveb msteveb closed this Mar 7, 2023
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.

None yet

2 participants