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

Breaking scroll buffer #41

Closed
codingberg opened this issue Mar 30, 2016 · 2 comments · Fixed by #42
Closed

Breaking scroll buffer #41

codingberg opened this issue Mar 30, 2016 · 2 comments · Fixed by #42
Assignees

Comments

@codingberg
Copy link

The current line is splitted into multiple lines based on terminal width. Splitting is actually precalculated inside functions SplitByMultiLine and SplitByLine. Then, when printing a line, '\n' are sent to terminal after each precalculated line (lines that are a product of splitting). All of this rely on the fact that most terminals will do the same on window width change. They will wrap all lines from history and current editing line around new width boundary. Line printing, cleaining, and on width change logic relys on this idea of splitting.

First thing

I believe there is a problem when you have a line that needs to be splitted in more than two lines. The code doesn't perform well in some cases. For example, when you need to split a very long line into 5 new ones. This would be a dirty fix for such scenario:

func SplitByMultiLine(start, oldWidth, newWidth int, rs []rune) []string {
    var ret []string
    buf := bytes.NewBuffer(nil)
    currentWidth := start
    for _, r := range rs {
        w := runes.Width(r)
        currentWidth += w
        buf.WriteRune(r)
        if currentWidth%newWidth == 0 {
            ret = append(ret, buf.String())
            buf.Reset()
            if currentWidth == oldWidth {
                currentWidth = 0
            }
            continue
        }
        if currentWidth >= oldWidth {
            ret = append(ret, buf.String())
            buf.Reset()
            currentWidth = 0
        }
    }
    if buf.Len() > 0 {
        ret = append(ret, buf.String())
    }
    return ret
}

Second thing

Adding NL after each split, gives terminal application 'a permission' to send some parts of original line beyond top of window screen, which taints scroll buffer. Every line that ends with '\n' becomes a part of history. So basically, complete line splitting logic that is used in printing and cleaning inside runebuf, that is relying on prediction of how many new lines will show on screen after line wrapping (done by terminal application automatically), can affect scrollbuffer badly. Once that a line (born after splitting) goes into scroll buffer, it might be gone from the screen as well. That breaks line cleaning logic from runebuf that sends "\033[2K\r\033[A" for every line, and moving cursor beyond top of the screen just doesn't work. Expecting that every splitted line will be always on screen is just not a good prediction. I suggest to replace the whole logic behind it. Adding NL artifically is just bad for scroll buffer and it breaks behaviour of terminal application in some cases.

@chzyer
Copy link
Owner

chzyer commented Mar 30, 2016

Thanks for your suggestion!
The season that I use '\n' to separate lines is iTerm2/Terminal did a strange action when the number of chars equals to the width of terminal. As you said, that is not a perfect way, it will breaking scroll buffer.
So I dig that a lot today and fortunately I found that can be fixed by append a " \b" into the line and not don't need to split them any more.

Please checkout here. #42

@codingberg
Copy link
Author

Yes. It seems as an excellent fix. Thank you for your prompt reply.

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