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

Incomplete event escape sequence on OSX (escape sequences split in 2 broken events) #132

Open
tcolar opened this issue Jun 24, 2016 · 15 comments
Labels

Comments

@tcolar
Copy link

tcolar commented Jun 24, 2016

On OSX when scrolling up and down sometimes the mouse event seem to be getting cut in half,
a normal event would be \x1b[<64;55;28M, but sometimes instead I get \x1b[<64;55;28, followed by 'M', I'm guessing it might some sort of buffering issue whereas termbox is parsing as the event is flowing in ? Obviously it's very annoying as I received what looks like a M Key pressed event.

Are there some known issues with this ? Note: I have never seen it on Linux(mostly gnome terminal), but it's very frequent on OSX(iTerm 2).

This is output from _demos/raw_input.go
See the bottom where it gets x1b[<65;55;28 and then M as two (wrong) events.

"\x1b[<64;55;28M"
EventMouse: x: 54, y: 27, b: MouseWheelUp, mod:
12
Press 'q' to quit
"\x1b[<64;55;28M"
EventMouse: x: 54, y: 27, b: MouseWheelUp, mod:
12
Press 'q' to quit
"\x1b[<65;55;28M"
EventMouse: x: 54, y: 27, b: MouseWheelDown, mod:
12
Press 'q' to quit
"\x1b[<65;55;28"
EventKey: k: 0, c: 8, mod:
1
Press 'q' to quit
"M"
EventKey: k: 0, c: M, mod:
1
Press 'q' to quit
@tcolar
Copy link
Author

tcolar commented Jun 24, 2016

Note : I might be able to look into it as I have access to a Mac, but wanted to check first if it's a know issue or if you have any ideas/comments.

@nsf
Copy link
Owner

nsf commented Jun 24, 2016

Related: #120

@nsf
Copy link
Owner

nsf commented Jun 24, 2016

So, yeah, if anyone can help me solve this (if it's even possible), that'd be nice. I don't have a mac.

@tcolar
Copy link
Author

tcolar commented Jun 25, 2016

This fixed it for me:
tcolar@7744ff9

But I' working off an old fork of termbox-go so it seems that code has changed a lot since. Let me try to see if it's a problem / need a fix in termbox master.

@tcolar
Copy link
Author

tcolar commented Jun 25, 2016

The bug does happen in master too, will try to fix it there and provide a PR

@tcolar
Copy link
Author

tcolar commented Jun 25, 2016

This worked on master, same idea of keeping existing (partial) input data, and defering event parsing until more data is available.

termbox.go
454,456c454,456
<               if n, ok := parse_escape_sequence(event, inbuf); n != 0 {
<                       event.N = n
<                       return ok
---
>               n, ok := parse_escape_sequence(event, inbuf)
>               if !ok {
>                       return false
457a458
>               event.N = n

@nsf
Copy link
Owner

nsf commented Jun 25, 2016

Hm... I don't see how it solves the problem. What if the data arrives is ESC symbol, it can be a partial escape sequence or an actual ESC key. It's ambiguous. That's why I never understood how even terminals work in the first place, because their input is always ambiguous if you slice it into bytes.

@tcolar
Copy link
Author

tcolar commented Jun 25, 2016

Actually for the new code that might not work well, in the old code I would only do that if strings.HasPrefix(bufstr, "\033[<") .... yeah I suppose that could happen to NOT be mouse but actually ESC[< but that kind of unlikely and yes as far as I can it's the best you can do because yes terminal bytes are ambiguous which seems terrible.

Maybe a slightly "better" approach would be (pseudo code)

buf=read()
if buf.startsWith("\033[<") && ! (buf.Contains('M') || buf.Contains('M')) {
    // It is MOST likely a partial mouse event, but small chance it is not, wait a bit and read more data
    time.Sleep(10*time.Millisecond) // some small delay to give partial mouse event to complete
    append(buf, buf.read())
}
parseEvent() // might get a mouse events, or some key events

It's kinda lame and ugly no question, but with the terminal data being non deterministic, I'm not sure much better can be done.

@nsf
Copy link
Owner

nsf commented Jun 25, 2016

Well, I don't like that kind of solutions to be honest. I guess I'll have to take a look at other terminal-based software and what it does on mac. Btw, is it a problem only with mouse events?

@tcolar
Copy link
Author

tcolar commented Jun 25, 2016

Unless you can force the os/terminal to not send partial events (didn't find a way to do this so far), I'm not sure what else you could do.

I have only seen it happen with mouse events, but it might just be because they are the longest ones (more bytes)

@ns-cweber
Copy link

@nsf According to this page the ambiguity is frequently resolved by checking to see if another character comes quickly after. This is obviously not an ideal solution, but I think we can do a bit better by (upon hitting an ESC character) continuing to read until we've ruled out all known escape sequences, rather than simply stopping because syscall.Read() didn't read in a full known escape sequence.

@rationull
Copy link
Contributor

@nsf are you still opposed to a solution here that involves a small delay after detecting an escape key, and then re-checking for escape sequences? Granted this is nasty because

  • presumably we need to handle even the case where the escape key is the last key in the buffer when it's read (i.e. no "[<" or 'M' in the buffer yet),
  • we can't really know how long we need to wait, and
  • this will impose the same small delay even in the case where the escape key was pressed and very quickly followed with another key but not in an escape sequence (or maybe this would just be enabled on macOS?). Such conditions could already nondeterministically be detected as escape sequences though, it seems like.

I'm toying with the idea of trying to fix this but if you feel like such a fix is unacceptable due to the nature of the problem then I may not bother. In theory would you accept a PR on this?

@nsf
Copy link
Owner

nsf commented Jan 11, 2018

If that behaviour will be enabled only on OSX I will accept it.

@rationull
Copy link
Contributor

Great! I've got a proof of concept working which I should be able to clean up, test, and isolate to macOS some time in the next few days and will submit a PR.

@rationull
Copy link
Contributor

This should be resolved after PR #176. I would be interested in knowing if anyone hits further problems, as the delay can be increased if necessary (currently set to 50ms).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants