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

Probably incorrect horizontal cursor positioning with certain CSI sequences #2020

Closed
cubiclesoft opened this issue Apr 19, 2019 · 6 comments · Fixed by #2288
Closed

Probably incorrect horizontal cursor positioning with certain CSI sequences #2020

cubiclesoft opened this issue Apr 19, 2019 · 6 comments · Fixed by #2288
Assignees
Labels
area/parser type/bug Something is misbehaving
Milestone

Comments

@cubiclesoft
Copy link
Contributor

cubiclesoft commented Apr 19, 2019

It appears to be possible to put the buffer.x position into an odd state with certain CSI sequences.

Sample

\E[4000G\E[B

Or:

\E[4000G\E[e

Analysis

    InputHandler.prototype.cursorCharAbsolute = function (params) {
        var param = params[0];
        if (param < 1) {
            param = 1;
        }
        this._terminal.buffer.x = param - 1;
    };

    InputHandler.prototype.cursorDown = function (params) {
        var param = params[0];
        if (param < 1) {
            param = 1;
        }
        this._terminal.buffer.y += param;
        if (this._terminal.buffer.y >= this._terminal.rows) {
            this._terminal.buffer.y = this._terminal.rows - 1;
        }
        if (this._terminal.buffer.x >= this._terminal.cols) {
            this._terminal.buffer.x--;
        }
    };

    InputHandler.prototype.vPositionRelative = function (params) {
        var param = params[0];
        if (param < 1) {
            param = 1;
        }
        this._terminal.buffer.y += param;
        if (this._terminal.buffer.y >= this._terminal.rows) {
            this._terminal.buffer.y = this._terminal.rows - 1;
        }
        if (this._terminal.buffer.x >= this._terminal.cols) {
            this._terminal.buffer.x--;
        }
    };

Not quite sure what the purpose of the x-- is. If the goal is to always cap it within the max columns, then that won't necessarily work properly here.

On an unrelated note, cursorDown and vPositionRelative contain identical code. cursorForward and hPositionRelative also contain identical code. Also, cursorPosition and hVPosition appear to have logically identical code but hVPosition is shorter and more readable, IMO.

@Tyriar
Copy link
Member

Tyriar commented Apr 20, 2019

I think the x-- is because printing can push the cursor to equal this._terminal.cols and only on the next print will it move to the following line.

@jerch
Copy link
Member

jerch commented Apr 28, 2019

Yep, the x-- tries to fix the faulty cursor position handling on sequence basis atm, a more general fix is needed as pointed out by #1434.

@Tyriar
Copy link
Member

Tyriar commented Apr 30, 2019

@jerch is this a dupe of #1434?

@jerch
Copy link
Member

jerch commented Apr 30, 2019

Kinda, its a part of it.

@Tyriar
Copy link
Member

Tyriar commented Apr 30, 2019

@jerch just the code duplication part is not?

@jerch
Copy link
Member

jerch commented Apr 30, 2019

Well the code duplication part is the try to fix the wrong positioning for certain methods, while others have not this "sanity" check yet. I guess there are more that can show this overflow (not checked), some methods even try to fix it on enter with a precheck (print for example always checks this before inserting chars to the buffer).
It happens to work atm with this mixture of pre- and postcheck/corrections, still the cursor is sometimes off (top shows the cursor wrongly in first column during screen refreshs). #1434 tried to address this but I kinda got distracted from it (and the fix was more an ugly hack).

@Tyriar Tyriar added this to the 4.0.0 milestone Jul 8, 2019
@Tyriar Tyriar added the type/bug Something is misbehaving label Jul 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/parser type/bug Something is misbehaving
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants