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 an indicator on terminals indicating whether they've been interacted with #127717

Closed
connor4312 opened this issue Jun 30, 2021 · 15 comments
Closed
Assignees
Labels
api api-finalization feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders on-release-notes Issue/pull request mentioned in release notes on-testplan terminal Integrated terminal issues

Comments

@connor4312
Copy link
Member

This came to mind as something I was talking about with Kai a while ago. Currently when you create a debug terminal, users can get a debug "flash" when the terminal opens: this comes from nvm executing an npm script in its initialization. While we could try to do some targeting based on the command and how it's invoked in the debug terminal, this is pretty fragile.

Instead, now that we have "interacted" detection, it would be helpful to expose that on the API so that can we can skip attaching before the user has run anything -- which should solve the generalized case of .*rc scripts nicely.

@Tyriar
Copy link
Member

Tyriar commented Jul 6, 2021

@connor4312 thoughts?

export interface Terminal {
  /**
   * Whether the terminal has received any input from the user. This value persists across window
   * reloads in the case of terminal reconnection.
   */
  readonly hadFirstInteraction: boolean;
}

namespace window {
  /**
   * An event which fires when a terminal has received input from the user for the first time.
   */
  onDidTriggerTerminalFirstInteraction: Event<Terminal>;
}

@Tyriar Tyriar added api api-proposal feature-request Request for new features or functionality terminal Integrated terminal issues labels Jul 6, 2021
@Tyriar Tyriar added this to the July 2021 milestone Jul 6, 2021
@connor4312
Copy link
Member Author

connor4312 commented Jul 6, 2021

I think the event should be named onDidTriggerFirstTerminalInteraction, since it's on the window. Looks good to me

@Tyriar
Copy link
Member

Tyriar commented Jul 6, 2021

Updated, I put Terminal before First

@Tyriar
Copy link
Member

Tyriar commented Jul 13, 2021

Feedback responses from last week:

Do we have something like this already?

What we do have is data written to the terminal which is different and that’s stuck in proposed for live share

People wondered what interaction means - clicking on the terminal, typing in it?

It may mean clicking, it depends on the terminal mode. We can clarify this in the comment.

terminal states?

Wonder what the other states would be though, we already have exitStatus which is also kind of a state (with additional info in exitCode)

@Tyriar
Copy link
Member

Tyriar commented Jul 13, 2021

After discussing in this API sync I like the state idea, there are 4 states we could potentially start with: opened (process not yet created), connected (process created), interacted (some input was sent to the pty) and exited.

@Tyriar Tyriar modified the milestones: July 2021, August 2021 Jul 26, 2021
@Tyriar
Copy link
Member

Tyriar commented Aug 10, 2021

API proposal:

	//#region Terminal state event https://github.com/microsoft/vscode/issues/127717

	export enum TerminalState {
		/**
		 * The terminal is created and has starting to connecting to the process. This state is
		 * followed by either Opened or Closed depending on whether the process is successfully
		 * connected to.
		 */
		Opening = 0,
		/**
		 * The terminal has connected to a valid process.
		 */
		Opened = 1,
		/**
		 * The terminal has connected to a valid process and has been interacted with. Interaction
		 * means that the terminal has sent data to the process which depending on the terminal's
		 * _mode_. By default input is sent when a key is pressed but based on the terminal's mode
		 * it can also happen on:
		 *
		 * - a pointer click event
		 * - a pointer scroll event
		 * - a pointer move event
		 * - terminal focus in/out
		 *
		 * For more information on events that can send data see "DEC Private Mode Set (DECSET)" on
		 * https://invisible-island.net/xterm/ctlseqs/ctlseqs.html
		 */
		InteractedWith = 2,
		/**
		 * The terminal has closed, if it had a process it is no longer connected.
		 */
		Closed = 3,
	}

	export interface Terminal {
		state: TerminalState;
	}

	export interface TerminalStateChangeEvent {
		terminal: Terminal;
		state: TerminalState;
	}

	export namespace window {
		export const onDidChangeTerminalState: Event<TerminalStateChangeEvent>;
	}

	//#endregion

@mjbvz
Copy link
Collaborator

mjbvz commented Aug 10, 2021

  • InteractedWith -> OpenedAndInteractedWith
  • { state: Opened, interactedWith: bool }?
  • readonly Terminal .state

@Tyriar
Copy link
Member

Tyriar commented Aug 12, 2021

Thinking about this a little more, we could just strip out the opened/opening/closed states as it's not part of this. If wanted in the future we could add if TerminalState is extensible, but there is already onDidCloseTerminal and Terminal.exitStatus to cover closed/opened and that is not part of this request.

New proposal:

	//#region Terminal state event https://github.com/microsoft/vscode/issues/127717

	/**
	 * Represents the state of a {@link Terminal}.
	 */
	export interface TerminalState {
		/**
		 * Whether the {@link Terminal} has been interacted with. Interaction means that the
		 * terminal has sent data to the process which depending on the terminal's _mode_. By
		 * default input is sent when a key is pressed but based on the terminal's mode it can also
		 * happen on:
		 *
		 * - a pointer click event
		 * - a pointer scroll event
		 * - a pointer move event
		 * - terminal focus in/out
		 *
		 * For more information on events that can send data see "DEC Private Mode Set (DECSET)" on
		 * https://invisible-island.net/xterm/ctlseqs/ctlseqs.html
		 */
		readonly interactedWith: boolean;
	}

	export interface Terminal {
		/**
		 * The current state of the {@link Terminal}.
		 */
		readonly state: TerminalState;
	}

	/**
	 * An event representing a change in a {@link Terminal.state terminal's state}.
	 */
	export interface TerminalStateChangeEvent {
		/**
		 * The {@link Terminal} this event occurred on.
		 */
		readonly terminal: Terminal;
		/**
		 * The {@link Terminal.state current state} of the {@link Terminal}.
		 */
		readonly state: TerminalState;
	}

	export namespace window {
		/**
		 * An {@link Event} which fires when a {@link Terminal.state terminal's state} has changed.
		 */
		export const onDidChangeTerminalState: Event<TerminalStateChangeEvent>;
	}

	//#endregion

@Tyriar
Copy link
Member

Tyriar commented Aug 13, 2021

TPI created #130775, moving to September for finalization

@connor4312 ready for trying out in js-debug

@Tyriar
Copy link
Member

Tyriar commented Aug 17, 2021

Feedback:

  • isInteractedWith/hasBeenInteractedWith as it's a boolean
  • Remove TerminalStateChangeEvent as there is now a state object: onDidChangeTerminalState: Event<Terminal>

@Tyriar
Copy link
Member

Tyriar commented Aug 24, 2021

Feedback:

@meganrogge
Copy link
Contributor

@connor4312 any feedback on API?

@Tyriar
Copy link
Member

Tyriar commented Sep 14, 2021

If we move on #133084 there will be more things we could expose on state such as:

  • cwd: string | undefined
  • acceptingInput: boolean | undefined

@Tyriar
Copy link
Member

Tyriar commented Sep 14, 2021

Good to finalize after making it isInteractedWith, not has as we use is prefix everywhere in API.

connor4312 added a commit to microsoft/vscode-js-debug that referenced this issue Sep 14, 2021
Tyriar added a commit that referenced this issue Sep 17, 2021
@Tyriar Tyriar closed this as completed in 36b0223 Sep 17, 2021
@connor4312 connor4312 assigned Tyriar and meganrogge and unassigned Tyriar and meganrogge Sep 17, 2021
@connor4312
Copy link
Member Author

sorry I'm catsitting and she walked on my laptop keyboard.

Adopted the rename in js-debug 👍

@rzhao271 rzhao271 added the verification-needed Verification of issue is requested label Sep 29, 2021
@rchiodo rchiodo added on-testplan and removed verification-needed Verification of issue is requested labels Sep 29, 2021
@meganrogge meganrogge added the on-release-notes Issue/pull request mentioned in release notes label Oct 1, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Nov 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api api-finalization feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders on-release-notes Issue/pull request mentioned in release notes on-testplan terminal Integrated terminal issues
Projects
None yet
Development

No branches or pull requests

7 participants
@Tyriar @connor4312 @rzhao271 @mjbvz @rchiodo @meganrogge and others