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

Expose API method for writing to application side (#4948) #4953

Merged
merged 11 commits into from Feb 2, 2024

Conversation

arencoskun
Copy link
Contributor

@arencoskun arencoskun commented Feb 1, 2024

This PR adds an input method which takes data as the parameter as described in the issue. If there are any issues, you can point it out and I'll fix it.

(Sorry if there's an issue, I am a beginner.)

Fixes #4948.

Copy link
Member

@jerch jerch left a comment

Choose a reason for hiding this comment

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

Welcome and thank you for looking into this.

Looks really good already, only a few my remarks from my side below.

src/browser/Terminal.ts Outdated Show resolved Hide resolved
src/browser/Terminal.ts Outdated Show resolved Hide resolved
src/browser/public/Terminal.ts Outdated Show resolved Hide resolved
typings/xterm.d.ts Outdated Show resolved Hide resolved
@arencoskun
Copy link
Contributor Author

Thanks for the feedback, working on it

@arencoskun arencoskun marked this pull request as draft February 1, 2024 10:53
@arencoskun arencoskun marked this pull request as ready for review February 1, 2024 11:11
Copy link
Member

@jerch jerch left a comment

Choose a reason for hiding this comment

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

@arencoskun With fixing the remaining linter errors, this LGTM 👍

@jerch jerch added this to the 5.4.0 milestone Feb 1, 2024
@arencoskun
Copy link
Contributor Author

@arencoskun With fixing the remaining linter errors, this LGTM 👍

Oops, forgot to fix those. Will work on it in a few moments

@jerch
Copy link
Member

jerch commented Feb 1, 2024

@arencoskun The linter is still nagging about a whitespace in xterm.d.ts. (guess you were faster 😸 )

Oh and btw - the declaration also needs to be added to xterm-headless.d.ts (sorry forgot about that before).

@arencoskun
Copy link
Contributor Author

@arencoskun The linter is still nagging about a whitespace in xterm.d.ts. (guess you were faster 😸 )

Oh and btw - the declaration also needs to be added to xterm-headless.d.ts (sorry forgot about that before).

No problem, also adding that

@arencoskun
Copy link
Contributor Author

Sorry, forgot to mark as draft

@jerch
Copy link
Member

jerch commented Feb 1, 2024

Whoopsie - the headless API-bundling class in src/headless/public/Terminal.ts would also need the impl forwarding similar to browser/public/Terminal.ts.

(Why? - We basically maintain 2 versions of xterm.js Terminal class - one for browser and for headless nodejs usage. So parts of the API bundling is doubled here.)

@arencoskun
Copy link
Contributor Author

Whoopsie - the headless API-bundling class in src/headless/public/Terminal.ts would also need the impl forwarding similar to browser/public/Terminal.ts.

(Why? - We basically maintain 2 versions of xterm.js Terminal class - one for browser and for headless nodejs usage. So parts of the API bundling is doubled here.)

Thanks for the info, I've been busy tonight but most probably tomorrow I'll finish it off.
(Also, seems like you can't mark PRs as drafts from the mobile app. That's weird 😀 )

@arencoskun arencoskun marked this pull request as draft February 2, 2024 06:05
@arencoskun arencoskun marked this pull request as ready for review February 2, 2024 06:06
@jerch
Copy link
Member

jerch commented Feb 2, 2024

@Tyriar Could you also review this PR? Because of the API addition I dont want to merge it w'o being reviewed by you.

@jerch jerch requested a review from Tyriar February 2, 2024 07:32
@Tyriar Tyriar self-assigned this Feb 2, 2024
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.

Thanks! I just tweaked the docs and removed the doc strings from the internal files to reduce duplication/bitrot

@Tyriar Tyriar enabled auto-merge February 2, 2024 20:12
@Tyriar Tyriar merged commit 6a281bd into xtermjs:master Feb 2, 2024
16 checks passed
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.

expose API method for writing to application side
3 participants