Moving around in Jline produces weird characters #31

Closed
Raynes opened this Issue Apr 11, 2012 · 20 comments

Comments

Projects
None yet
4 participants

Raynes commented Apr 11, 2012

~/code/jline2(master) $ java -cp target/jline-2.7-SNAPSHOT.jar:target/jline-2.7-SNAPSHOT-tests.jar jline.example.Example none
prompt> foo bar baz foo bar baz foo bar baz foo bar baz foo bar baz foo bar baz foo ba;86Rr baz

Notice towards the end there are some characters: ;86R. I keep getting this whenever I move around the text with the arrow keys (left and right). It is spontaneous and doesn't happen for every key stroke, but consistently happens every 30 or so.

This is in bash on iTerm 2 in OS X. The jline version used in the above example is SHA: 5dcfc6b

Raynes commented Apr 11, 2012

Investigating this further, it happens every time my cursor moves to a lower line. In the above example, the area where that occurred was word-wrapped by iTerm.

Member

trptcolin commented Apr 11, 2012

Ah, good catch. And it happens for me on multi-line stuff as well, even with other shortcut keys (word-forward, end-of-line, etc), but only on the keypress that moves you from the first to the second line (or 2nd to 3rd, etc.)

Owner

jdillon commented Apr 11, 2012

Yup, appears to be something in the ML handling. I've seen this in both iTerm and Terminal.app

Member

trptcolin commented Apr 13, 2012

Please see 5103bac#src/main/java/jline/console/ConsoleReader.java-P107

I tracked it down, but want to cover my bases as i'm not sure how to get a true "weird wrap" terminal.

Member

trptcolin commented Apr 13, 2012

Here's what I'm thinking: trptcolin/jline2@7bdfcff

Apologies for the whitespace - git diff -w works, or I can push a new branch without the whitespace removals if needed.

Owner

jdillon commented Apr 13, 2012

Please do push a new change w/o the superfluous whitespace changes.

Member

trptcolin commented Apr 13, 2012

Sure: trptcolin/jline2@baf839e

This also deletes the related and no-longer-used private method getCurrentPosition.

Member

trptcolin commented Apr 13, 2012

The other direction I see is for someone familiar with hasWeirdWrap() to limit it to only return true results for consoles that actually do suffer from this mysterious disease.

Owner

jdillon commented Apr 14, 2012

IMO hasWeirdWrap is a turd in the api which should not exist. I'm not even sure wtf "weird-wrap" means

Contributor

huynhjl commented Apr 15, 2012

Really sorry for hasWeirdWrap. I came up with that name while doing some investigation and my pull request was merged without much review. It was an indicator for terminals where when a character is entered in the last column, the cursor does not move to the next line (typically staying in the last column and character is printed in reverse video). I needed a way to identify that we are dealing with that type terminal and in that case possibly use more complex logic. Ubuntu's default terminal has this behavior. May be hasAutoWrapInLastColumn would be better (and it would be the opposite of hasWeirdWrap)?

Member

trptcolin commented Apr 16, 2012

Please see the latest notes in 5103bac#src/main/java/jline/console/ConsoleReader.java-P107 - looks like my commit fixing this issue should be OK.

@jdillon let me know if you want a separate pull request, or if the branch I pasted the commit from earlier is ok: https://github.com/trptcolin/jline2/tree/multiline_less_whitespace

Owner

jdillon commented Apr 16, 2012

Thats not a pull request :-P

Member

trptcolin commented Apr 16, 2012

I guess that means you want a pull request. I meant a "pull request separate from the linked branch".

Owner

jdillon commented Apr 16, 2012

Pull request integration in github makes it nice and easy to review whats changed, so I prefer to use that when possible (yes I'm lazy).

Member

trptcolin commented Apr 16, 2012

Cool deal. Pull request opened, thanks!

Owner

jdillon commented Apr 22, 2012

What is the impact of this change? I'm included to merge it but I'm not neck deep in jline2 stuff atm so I'm not sure what the effect of this change is.

Owner

jdillon commented Apr 22, 2012

for ref #34

Member

trptcolin commented Apr 22, 2012

The impact is to display normally when moving the cursor across multiple lines, and avoid printing these extra characters. I have not seen any downsides/tradeoffs. @huynhjl has also done some testing: see 5103bac#commitcomment-1213878

Contributor

huynhjl commented Apr 22, 2012

I think that removing sending ansi sequence to query terminal properties like 6n (here to get cursor position) is beneficial. I believe this code was introduced when I was trying to avoid outputting a newline when the cursor is past the last column. Since now we are output the newline, therefore the check is no longer necessary. This particular piece of code would detect if the cursor would be in the last line of the terminal and scroll up. I tested trptcolin branch and found it to behave as good as the current behavior.

Member

trptcolin commented Jun 13, 2012

Works great in 2.7 - this can be closed.

jdillon closed this Jun 13, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment