-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
progress-addon #5251
base: master
Are you sure you want to change the base?
progress-addon #5251
Conversation
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.
|
||
/** progress object interface */ | ||
export interface IProgress { | ||
state: 0 | 1 | 2 | 3 | 4; |
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.
Could pull this into type ProgressState
?
public dispose(): void; | ||
|
||
/** register progress handler */ | ||
public register(handler: ProgressHandler): IDisposable; |
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.
Should this be an event instead like this?
xterm.js/addons/addon-webgl/typings/addon-webgl.d.ts
Lines 20 to 23 in 41e8ae3
/** | |
* An event that is fired when the texture atlas of the renderer changes. | |
*/ | |
public readonly onChangeTextureAtlas: IEvent<HTMLCanvasElement>; |
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.
Yes would be better. Is there an easy way to do that in an addon w'o pulling a whole bunch of code deps into it?
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.
Just referencing the event file will bundle only the necessary stuff, it'll include some extra stuff in the file but the situation should improve over time when tree shaking gets better.
/** getter / setter for current progress */ | ||
public progress: IProgress; |
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.
Should this not be readonly?
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.
Hmm well, currently it is the only way to reset a broken progress indicator from integration side by setting it explicitly back to initial. If you feel uneasy about that - I can make this readonly and deploy a reset
method for that special case instead. What you think?
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 guess set is fine as it means we can restore state if needed like when reloading a window in VS Code 👌
This addon implements ConEmu's progress sequence, for more context see #5250.
The addon tries to hide most of the sequence's shenanigans behind a convenient API:
Fixes #5250.