-
Notifications
You must be signed in to change notification settings - Fork 1.8k
headless onRowChange event #5392
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,7 +11,7 @@ import { Terminal as TerminalCore } from 'headless/Terminal'; | |
| import { AddonManager } from 'common/public/AddonManager'; | ||
| import { ITerminalOptions } from 'common/Types'; | ||
| import { Disposable } from 'vs/base/common/lifecycle'; | ||
| import type { Event } from 'vs/base/common/event'; | ||
| import { Event } from 'vs/base/common/event'; | ||
| /** | ||
| * The set of options that only have an effect when set in the Terminal constructor. | ||
| */ | ||
|
|
@@ -81,6 +81,10 @@ export class Terminal extends Disposable implements ITerminalApi { | |
| public get onScroll(): Event<number> { return this._core.onScroll; } | ||
| 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(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can skip making it proposed since it's so simple |
||
| return Event.map(this._core.onRowChange, e => ({ start: e.start, end: e.end })); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We want to avoid |
||
| } | ||
|
|
||
| public get parser(): IParser { | ||
| this._checkProposedApi(); | ||
|
|
@@ -186,7 +190,7 @@ export class Terminal extends Disposable implements ITerminalApi { | |
| } | ||
| 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); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why was this needed?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| } | ||
|
|
||
| private _verifyIntegers(...values: number[]): void { | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -748,6 +748,13 @@ declare module '@xterm/headless' { | |||||||||||
| */ | ||||||||||||
| onResize: IEvent<{ cols: number, rows: number }>; | ||||||||||||
|
|
||||||||||||
| /** | ||||||||||||
| * Adds an event listener for when buffer rows change during parsing. The event | ||||||||||||
| * value contains the range of rows that changed. | ||||||||||||
|
Comment on lines
+752
to
+753
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||
| * @returns an `IDisposable` to stop listening. | ||||||||||||
| */ | ||||||||||||
| onRowChange: IEvent<{ start: number, end: number }>; | ||||||||||||
|
Comment on lines
+751
to
+756
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's also add to xterm.d.ts as xterm-headless is a subset of that
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if instead xterm-headless should just forward |
||||||||||||
|
|
||||||||||||
| /** | ||||||||||||
| * Adds an event listener for when a scroll occurs. The event value is the | ||||||||||||
| * new position of the viewport. | ||||||||||||
|
|
||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the event also fire undefined? I think that means every row needs to be updated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.