headless onRowChange event #5392
Conversation
Tyriar
left a comment
Tyriar
left a comment
There was a problem hiding this comment.
We don't have plans to add this, the way we're handling similar things in VS Code are by tracking onCursorMove/onData/onLineFeed and then checking the buffer's cursor position.
|
The goal is to expose the _onRequestRefreshRows, so you can write a renderer on top of headless, like a real renderer would in regular xterm.js. The specific fire that is very useful in rendering is actually for a scroll event: this._onRequestRefreshRows.fire({ This event provides just what you need to for render on headless. onLineFeed and onCursorMove didn't definitely give me this fire every time the buffer scrolls. |
| public get onWriteParsed(): Event<void> { return this._core.onWriteParsed; } | ||
| public get onRowChange(): Event<{ start: number, end: number }> { | ||
| this._checkProposedApi(); | ||
| return Event.map(this._core.onRowChange, e => ({ start: e.start, end: e.end })); |
There was a problem hiding this comment.
We want to avoid Event.map here if possible, now sure why this would be needed?
| public get onTitleChange(): Event<string> { return this._core.onTitleChange; } | ||
| public get onWriteParsed(): Event<void> { return this._core.onWriteParsed; } | ||
| public get onRowChange(): Event<{ start: number, end: number }> { | ||
| this._checkProposedApi(); |
There was a problem hiding this comment.
Can skip making it proposed since it's so simple
| public loadAddon(addon: ITerminalAddon): void { | ||
| // TODO: This could cause issues if the addon calls renderer apis | ||
| this._addonManager.loadAddon(this as any, addon); | ||
| this._addonManager.loadAddon(this as any, addon as any); |
There was a problem hiding this comment.
The typescript compiler was complaining because the interfaces on headless and regular are different. I'm not sure why it ever worked without an as cast.
| this._register(this._inputHandler.onRequestRefreshRows(e => { | ||
| if (e) { | ||
| this._onRowChange.fire({ start: e.start, end: e.end }); | ||
| } | ||
| })); |
There was a problem hiding this comment.
Shouldn't the event also fire undefined? I think that means every row needs to be updated?
There was a problem hiding this comment.
I didn't love the might be undefined or might be an object as the event spec, and I never needed the undefined case so I thought it made the API cleaner. If I remember right another event gets fired on full repaint so its redundant. I'm indifferent to changing the interface to { start, end}|undefined.
| /** | ||
| * Adds an event listener for when buffer rows change during parsing. The event | ||
| * value contains the range of rows that changed. | ||
| * @returns an `IDisposable` to stop listening. | ||
| */ | ||
| onRowChange: IEvent<{ start: number, end: number }>; |
There was a problem hiding this comment.
Let's also add to xterm.d.ts as xterm-headless is a subset of that
| * Adds an event listener for when buffer rows change during parsing. The event | ||
| * value contains the range of rows that changed. |
There was a problem hiding this comment.
| * Adds an event listener for when buffer rows change during parsing. The event | |
| * value contains the range of rows that changed. | |
| * Adds an event listener for when buffer rows change during parsing. The | |
| * event value contains the range of rows that changed. This is particularly | |
| * useful when implementing a custom renderer. |
| * value contains the range of rows that changed. | ||
| * @returns an `IDisposable` to stop listening. | ||
| */ | ||
| onRowChange: IEvent<{ start: number, end: number }>; |
There was a problem hiding this comment.
I wonder if instead xterm-headless should just forward onRequestRefreshRows to the existing onRender? Would that accomplish what you're after?
2 participants
Add a onRowChange event so that watchers can do efficient inspection of buffer changes.