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

Add an API to get a terminal's selection #188173

Open
Tyriar opened this issue Jul 18, 2023 · 14 comments
Open

Add an API to get a terminal's selection #188173

Tyriar opened this issue Jul 18, 2023 · 14 comments
Assignees
Labels
api api-proposal feature-request Request for new features or functionality terminal General terminal issues that don't fall under another label
Milestone

Comments

@Tyriar
Copy link
Member

Tyriar commented Jul 18, 2023

This is would be useful information for Copilot to know about.

@Tyriar Tyriar added feature-request Request for new features or functionality api api-proposal labels Jul 18, 2023
@Tyriar Tyriar added this to the July 2023 milestone Jul 18, 2023
@Tyriar Tyriar self-assigned this Jul 18, 2023
Tyriar added a commit that referenced this issue Jul 18, 2023
@Tyriar
Copy link
Member Author

Tyriar commented Jul 24, 2023

I'll wait for this to go through the API sync before doing a test plan item.

@Tyriar Tyriar modified the milestones: July 2023, August 2023 Jul 24, 2023
@Tyriar
Copy link
Member Author

Tyriar commented Aug 7, 2023

Proposal:

https://github.com/microsoft/vscode/blob/ea490e5545df658e05d0332bfb2a04435a0ef7f0/src/vscode-dts/vscode.proposed.terminalSelection.d.ts#L6-L17

If we want a change event:

export namespace window {
	export const onDidChangeTerminalSelection: Event<TerminalSelectionChangeEvent>;
}

export interface TerminalSelectionChangeEvent {
	readonly terminal: Terminal;
	readonly selection: string | undefined;
	// No kind like TextEditorSelectionChangeKind
}

@jrieken
Copy link
Member

jrieken commented Aug 7, 2023

Consider to have Terminal#selection which allows to retrieve the selection outside of an event

Also wondering about the missing "coordinates" (range, offset, etc) that would allow to locate and set selections

@Tyriar
Copy link
Member Author

Tyriar commented Aug 7, 2023

@jrieken the part of the proposal already in main is Terminal#selection. For coordinates, we don't have these anywhere in the terminal and you wouldn't be able to use them if we did.

@jrieken
Copy link
Member

jrieken commented Aug 8, 2023

For coordinates, we don't have these anywhere in the terminal and you wouldn't be able to use them if we did.

Does that mean one isn't ever able to set a selection? How does this work internally. Does xterm not have this or do you simply not want to expose terminal rows/columns?

@Tyriar
Copy link
Member Author

Tyriar commented Aug 8, 2023

You can select in xterm.js, here are the apis:

select(column: number, row: number, length: number): void;
selectAll(): void;
selectLines(start: number, end: number): void;

When considering whether we want to expose such an API it's useful to understand how the buffer works. It's essentially a big grid of characters of size <cols> x <rows> + <scrollback>, since there grid has fixed dimensions it comes with several complications/differences with the editor:

  • Wrapped lines are indicated by boolean flags on a line indicating whether the previous line wraps into it.
  • Resizing the terminal to make it more wide or narrow will "reflow" the terminal, which attempts to get natural wrapping behavior by pushing cells to different lines, potentially deleting or creating lines in the process.
  • The buffer has a fixed height which means when the buffer is full, each new line will trim the top line off. Internally this uses a circular buffer where the top row's index is 0, and the bottom row's index is <rows> + <scrollback> - 1.
  • The suggestion of a more stable line indexing system has come up before, but it gets kind of complicated. We could keep a running tally the amount of trimmed lines and just add that value to the current buffer lines, but that means if rows get added or removed we would need to change the index due to inserted or deleted lines, all lines below the changed ones.
  • Exposing terminal coordinates in the API wouldn't be useful at all without the ability to query the value in them. Following existing APIs and the guidelines would mean these should be synchronous, this works in the editor because the buffers are synchronized across processes. This would be a bad idea in the terminal imo, consider what happens when running ls -lR /; a tonne of data will come in and immediately be discarded as it's trimmed off the top of the buffer. Trying to keep these in sync on the extension host would be a waste of CPU time, memory and like lead to misbehaving extensions as they don't understand the intricacies of how the buffer works.

To bring this all together, we could of course expose xterm.js-like APIs in the exthost with the ability to pull and select arbitrary cell data, but it would have to be asynchronous and it would differ in subtle ways from the editor's APIs. Ultimately, I think this would just add APIs that don't add that much value but are easy to misuse, and the amount of value they add wouldn't be worth the effort to implement it anyway.

@Tyriar
Copy link
Member Author

Tyriar commented Aug 15, 2023

Feedback:

  • Should it be named selectedText instead of selection in case we expose terminal buffer coordinates in the future?

@Tyriar Tyriar modified the milestones: August 2023, September 2023 Aug 15, 2023
@jrieken
Copy link
Member

jrieken commented Aug 17, 2023

Should it be named selectedText instead of selection in case we expose terminal buffer coordinates in the future?

I'd say that for the scope a proposed API this doesn't really matter. But for finalisation I wouldn't consider either, finalising an API so that it can be made obsolete in the future without hassle isn't a goal.

Ultimately, I think this would just add APIs that don't add that much value but are easy to misuse, and the amount of value they add wouldn't be worth the effort to implement it anyway.

I don't know about this but my worry is that we do add more and more concepts to the terminal API, like selection or shell but that we don't think out load about the individual parts. We need to make sure that we don't miss the point at which we separate concerns and introduce types for buffer, buffer-coords, shell and more. I am not saying all needs to happen at once but for new API we must kept this in our mind and make sure we don't block any of this happening.

This would be a bad idea in the terminal imo, consider what happens when running ls -lR /; a tonne of data will come in and immediately be discarded as it's trimmed off the top of the buffer.

Agreeing and you can simulate that with cut-paste cycles of very many lines in a document. Tho, terminal-sync can be debounced to reduce waste.

Again, not saying all of this must happen before proposing a simple selection but my worry is that we are getting closer to this inflection point at which we should have better decoupled bits.

@Tyriar
Copy link
Member Author

Tyriar commented Sep 18, 2023

I'll keep this one on hold in its current proposed state for now, at least until the idea of a Terminal.shell API has been explored in #145234

@Tyriar Tyriar modified the milestones: September 2023, Backlog Sep 18, 2023
@troynguyen8-meta
Copy link

Hey! Is there a workaround for this? Maybe executing the "copy" command and then reading clipboard?

@poornimas-iyer
Copy link

I would like to get the selected Text from Active Terminal in vscode extension
Please let me know the options that we have for the same currently to build extensions

@Tyriar
Copy link
Member Author

Tyriar commented Nov 1, 2024

@poornimas-iyer this isn't possible until there's more movement here and/or in #207504

@yuqizhou77
Copy link

yuqizhou77 commented Nov 29, 2024

I am hoping the same so that we can get selected text from active terminal. Looks like Copilot has already able to access this. Hoping there is some API can be exposed.

@Tyriar
Copy link
Member Author

Tyriar commented Dec 2, 2024

@yuqizhou77 yeah when this went in for finalization it was put on hold along with some similar APIs as what we're ultimately moving towards is full buffer access in #207504, and we don't want to finalize an API that we don't want to keep as we generally don't break APIs.

@Tyriar Tyriar added the terminal General terminal issues that don't fall under another label label Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api api-proposal feature-request Request for new features or functionality terminal General terminal issues that don't fall under another label
Projects
None yet
Development

No branches or pull requests

5 participants