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

Buffer API #2074

Merged
merged 15 commits into from
May 12, 2019
Merged

Buffer API #2074

merged 15 commits into from
May 12, 2019

Conversation

Tyriar
Copy link
Member

@Tyriar Tyriar commented May 11, 2019

This exposes the primary parts of the buffer to APIs, see changes to xterm.d.ts for the actual public API. It works by having a "view" object in public/ that owns the actual object and exposes friendly aliases for the properties, for example viewportY instead of ydisp, cursorX over x, getLine over lines.get(x), etc.

The first usage of this will be moving the search addon into the new addon API #2065, built 100% on stable API! The buffer API is currently marked experimental to make sure it meets the needs of the search addon, we can remove experimental at or before v4 #2075

Fixes #1994

@Tyriar Tyriar added this to the 3.14.0 milestone May 11, 2019
@Tyriar Tyriar self-assigned this May 11, 2019
@Tyriar Tyriar mentioned this pull request May 11, 2019
jerch
jerch previously requested changes May 11, 2019
Copy link
Member

@jerch jerch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice 👍 , just two concerns from my side.

src/public/Terminal.ts Outdated Show resolved Hide resolved
src/public/Terminal.ts Outdated Show resolved Hide resolved
@jerch
Copy link
Member

jerch commented May 11, 2019

@Tyriar
The way you currently create the BufferView by referencing is not "stable" for buffer lines between data updates of the terminal due to recycling and line movements. Not sure yet if thats a drawback/issue at all, still it means that every action from an addon will have to re-request the view to see the real terminal buffer state with the correct buffer line order.

I think this is not an issue if we clearly document that ppl should avoid holding refs to members on nonlocal scope (rule of thumb - no event loop in between), instead always refer to them by the full descent like terminal.buffer().getLine(y).getCell(x)..... If they really have to hold stuff, they will have to copy the parts over to some own container. Local refs are not affected by this, so doing this to avoid recreating the intermediate objects is perfectly doable:

function someFunc() {
    const line = terminal.buffer().getLine(y);
    // iterate over cells
    for (let x=0; 0 < line.length; ++x) {
        someAction(line.getCell(x));
    }
}

While this might lead to unexpected results:

function someFunc() {
    const line = terminal.buffer().getLine(y);
    // iterate over cells
    for (let x=0; 0 < line.length; ++x) {
        // by the time doSomethingWithCell is called
        // line does not point to buffer[y] anymore
        setTimeout(() => someAction(line.getCell(x)), timeout);
    }
}

The latter can be fixed like this (but leads to the intermediate object overhead):

function someFunc() {
    // iterate over cells
    for (let x=0; 0 < terminal.cols; ++x) {
        setTimeout(() => someAction(terminal.buffer().getLine(y).getCell(x)), timeout);
    }
}

@Tyriar
Copy link
Member Author

Tyriar commented May 12, 2019

@jerch I think this is fine if it's called out in the documentation as you mention, I'll add some more details to the jsdoc

@Tyriar Tyriar merged commit 0fae9bb into xtermjs:master May 12, 2019
@Tyriar Tyriar deleted the 1994_buffer_api branch May 30, 2019 23:06
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 this pull request may close these issues.

Buffer API
2 participants