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

vi-style paren blinking causes unwanted "echo" in emacs #864

Closed
hvesalai opened this issue Sep 11, 2023 · 11 comments · Fixed by #874
Closed

vi-style paren blinking causes unwanted "echo" in emacs #864

hvesalai opened this issue Sep 11, 2023 · 11 comments · Fixed by #874

Comments

@hvesalai
Copy link
Contributor

hvesalai commented Sep 11, 2023

When using sbt from emacs (through sbt-mode), the terminal is set not to echo. However, if the input line contains parenthesis, the line is partially repeated.

scala> "foo"
val res1: String = foo

scala> "foo)bar"
"foo)val res2: String = foo()

Note on the last line the additional "foo), which is not expected.

The behavior is possibly caused by the vi-style paren blinking: https://github.com/jline/jline3/blob/master/reader/src/main/java/org/jline/reader/impl/LineReaderImpl.java#L2167

The question is, why is this functionality enabled for dumb terminals in sbt? Is there something wrong in how sbt uses jline3 or is it a bug in jline3 proper? I am available to help with a bug fix as soon as I understand which module to change.

@hvesalai
Copy link
Contributor Author

More specifically the erroneous output appears after calling redisplay() here:

https://github.com/jline/jline3/blob/jline-parent-3.18.0/reader/src/main/java/org/jline/reader/impl/LineReaderImpl.java#L2197

I wonder if that line should be removed since redisplay() is conditionally called also after applying the widgets here:

https://github.com/jline/jline3/blob/jline-parent-3.18.0/reader/src/main/java/org/jline/reader/impl/LineReaderImpl.java#L705

Or if the condition if (!dumb) { should be moved to be within redisplay so handling of dumb terminal would be localized to one place.

@hvesalai
Copy link
Contributor Author

@gnodet I would like to make a pull request to fix this, but would want some feedback on the approach before that. Would you have a moment to advice.

@gnodet
Copy link
Member

gnodet commented Oct 17, 2023

@gnodet I would like to make a pull request to fix this, but would want some feedback on the approach before that. Would you have a moment to advice.

With a dumb terminal, I'm even wondering if the read line loop which read bindings and evaluates widgets should be called at all. Given there's no line edition possible, I think the LineReader should simply read the full line and return it without any processing. Thoughts ?

@hvesalai
Copy link
Contributor Author

That's true. However, history should be stored because sbt provides even dump terminals ways to access the history.

@hvesalai
Copy link
Contributor Author

OTOH. Things such as history expansion would work equally on DUMP terminals (i.e. !!)

@hvesalai
Copy link
Contributor Author

How about if we make a keymap for DUMP that only has a few things that will work in that environment?

@gnodet
Copy link
Member

gnodet commented Oct 17, 2023

How about if we make a keymap for DUMP that only has a few things that will work in that environment?

Yes, sounds a good idea.

@hvesalai
Copy link
Contributor Author

Actually we already have there the safe() keybindings. Couldn't we use just that for DUMP?

@hvesalai
Copy link
Contributor Author

ping @gnodet. I would appreciate feedback on my fix (I have tested it on the repl and it works). Besides the master, I would also like it to be ported to the next release if that is not from master

@hvesalai
Copy link
Contributor Author

This wasn't completed after all. The reason is that https://github.com/jline/jline3/blob/jline-parent-3.24.1/reader/src/main/java/org/jline/reader/impl/InputRC.java#L47 undoes what we implemented in LineReaderImpl.

The line in

@hvesalai
Copy link
Contributor Author

@gnodet can you reopen the issue, it needs more fixing, see above comment

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 a pull request may close this issue.

2 participants