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

progress-addon #5251

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

progress-addon #5251

wants to merge 5 commits into from

Conversation

jerch
Copy link
Member

@jerch jerch commented Dec 15, 2024

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:

const progressAddon = new ProgressAddon();
terminal.loadAddon(progressAddon);
progressAddon.register((state, value) => {
  // state: 0-4 integer
  // value: 0-100 integer
  
  // do your visualisation based on state/progress here
  ...
});

Fixes #5250.

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.

Really solid 👏

Depending on app support, this could be really nice to integrate with the VS Code tab progress:

image


/** progress object interface */
export interface IProgress {
state: 0 | 1 | 2 | 3 | 4;
Copy link
Member

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;
Copy link
Member

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?

/**
* An event that is fired when the texture atlas of the renderer changes.
*/
public readonly onChangeTextureAtlas: IEvent<HTMLCanvasElement>;

Copy link
Member Author

@jerch jerch Dec 16, 2024

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?

Copy link
Member

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.

Comment on lines +18 to +19
/** getter / setter for current progress */
public progress: IProgress;
Copy link
Member

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?

Copy link
Member Author

@jerch jerch Dec 16, 2024

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?

Copy link
Member

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 👌

@Tyriar Tyriar added this to the 6.0.0 milestone Dec 16, 2024
@Tyriar Tyriar self-assigned this Dec 16, 2024
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.

ConEmu OSC progressbar support
2 participants