Skip to content

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

Merged
Tyriar merged 11 commits intoxtermjs:masterfrom
arencoskun:master
Feb 2, 2024
Merged

Expose API method for writing to application side (#4948)#4953
Tyriar merged 11 commits intoxtermjs:masterfrom
arencoskun:master

Conversation

@arencoskun
Copy link
Copy Markdown
Contributor

@arencoskun arencoskun commented Feb 1, 2024 •
edited by jerch
Loading

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.

jerch
jerch suggested changes Feb 1, 2024
View reviewed changes
Copy link
Copy Markdown
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.

@arencoskun
Copy link
Copy Markdown
Contributor Author

arencoskun commented Feb 1, 2024

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
jerch
jerch approved these changes Feb 1, 2024
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

arencoskun commented Feb 1, 2024

@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
Copy Markdown
Member

jerch commented Feb 1, 2024 •
edited
Loading

@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
Copy Markdown
Contributor Author

arencoskun 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).

No problem, also adding that

@arencoskun
Copy link
Copy Markdown
Contributor Author

arencoskun commented Feb 1, 2024

Sorry, forgot to mark as draft

@jerch
Copy link
Copy Markdown
Member

jerch commented Feb 1, 2024 •
edited
Loading

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
Copy Markdown
Contributor Author

arencoskun 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.)

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
Copy Markdown
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
Tyriar
Tyriar approved these changes Feb 2, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@Tyriar Tyriar left a comment •
edited
Loading

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Tyriar Tyriar Tyriar approved these changes

+1 more reviewer

@jerch jerch jerch approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@Tyriar Tyriar

Labels

None yet

Projects

None yet

Milestone

5.4.0

Development

Successfully merging this pull request may close these issues.

expose API method for writing to application side

3 participants

@arencoskun @jerch @Tyriar