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

Allow markers on the alt buffer #3924

Merged
merged 6 commits into from
Jul 31, 2022
Merged

Conversation

silamon
Copy link
Contributor

@silamon silamon commented Jul 24, 2022

Fixes #3918

An additional event to trigger on buffer change wasn't needed, since the onWriteParsed is being triggered.

@Tyriar Tyriar added this to the 5.0.0 milestone Jul 28, 2022
@Tyriar Tyriar self-assigned this Jul 28, 2022
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

One special thing that comes to mind here is for alt buffers we want to dispose all buffers as soon as the active buffer switches back to the normal buffer. I think the fix for that would be to call clearAllMarkers() here:

// The alt buffer should always be cleared when we switch to the normal
// buffer. This frees up memory since the alt buffer should always be new
// when activated.
this._alt.clear();

@silamon
Copy link
Contributor Author

silamon commented Jul 31, 2022

Without clearAllMarkers, you can see that the alt buffer decorations persist on the screen until the normal buffer is rendered. The search addon disposes the decorations/markers then on the onWriteParsed() event. Adding clearAllMarkers disposes them before clearing the all buffer.

It might be even better to add clearAllMarkers to the clear function of the buffer (?)

EDIT: The search-addon decorations are very hard to see with the background changes.

@Tyriar
Copy link
Member

Tyriar commented Jul 31, 2022

@silamon since markers are changing to work also in the alt buffer we'll need to cover all marker use cases, this means making them invalid when alt -> normal happens. To validate which solution is best you could also run this while the alt buffer is up:

const m = term.registerMarker();

Switch the the normal buffer and make sure m is now disposed.

Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Seems to work as expected 👍

src/common/buffer/BufferSet.test.ts Outdated Show resolved Hide resolved
@Tyriar
Copy link
Member

Tyriar commented Jul 31, 2022

The search-addon decorations are very hard to see with the background changes.

Do you mean the demo theme changes? The demo's options.theme isn't tied to the search theme currently and it's probably overkill for the demo. You can always switch the theme now though:

Screen Shot 2022-07-31 at 2 38 25 pm

@Tyriar
Copy link
Member

Tyriar commented Jul 31, 2022

It might be even better to add clearAllMarkers to the clear function of the buffer (?)

Adding to clear would call it in the normal buffer as well and I think there might be a problem with that. Just doing it for the alt buffer is the safer approach.

@Tyriar Tyriar merged commit 0910e58 into xtermjs:master Jul 31, 2022
@silamon silamon deleted the allowmarkersonaltbuffer branch October 16, 2022 09:52
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.

Search highlights don't work in the alt buffer
2 participants