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

Check process tree when evaluating whether the shell supporting clearing #48974

Closed
Tyriar opened this issue Apr 30, 2018 · 3 comments
Closed
Assignees
Labels
invalid Issue identified as not relevant or not valid terminal Integrated terminal issues

Comments

@Tyriar
Copy link
Member

Tyriar commented Apr 30, 2018

Follow up from #48146 (comment)

Running a simple bash script like while sleep 1; do echo 'a'; done and then pressing cmd+k will print ^L to the screen. We should check to see if any child processes are there and if so revert back to the standard clear all except the cursor line.

@Tyriar Tyriar added bug Issue identified by VS Code Team member as probable bug terminal Integrated terminal issues labels Apr 30, 2018
@Tyriar Tyriar added this to the Backlog milestone Apr 30, 2018
@Tyriar Tyriar self-assigned this Apr 30, 2018
@cedric05
Copy link
Contributor

cedric05 commented May 3, 2018

any plans? or can i work on this?

@Tyriar
Copy link
Member Author

Tyriar commented May 3, 2018

@cedric05 you can give it a try to see if the performance hit is alright. Here's how I see the implementation working.

Basically what we want to do is that when the process title changes, that is this event for macOS/Linux https://github.com/Microsoft/vscode/blob/0c5da80ab2f8af0e94f29af5ffdbfb7b10bdf653/src/vs/workbench/parts/terminal/electron-browser/terminalInstance.ts#L604, shell out to get the process tree under the shell process (this._processManager.shellProcessId). Note that the listener would need to be permanent, not just based on whether name is set.

For Windows we're already getting the process tree here https://github.com/Microsoft/vscode/blob/0c5da80ab2f8af0e94f29af5ffdbfb7b10bdf653/src/vs/workbench/parts/terminal/node/windowsShellHelper.ts#L119, we just need to inform TerminalInstance of whether the shell has children or not.

Some notes:

  • This work will also directly contribute at getting Terminal confirmOnExit should only warn when the terminal shell is running something #23808 fixed.

  • We want to check the tree before clear or shutdown happens so that we're not blocked on IO when they happen as we don't have that much time.

  • We want to scan the process tree as opposed to checking the process title (which we already have) such that cases like this are handled:

    bash -l
      bash some_script.sh
    

    I think it's a fine compromise to fallback on the old Terminal: Clear implementation if you're running a sub-shell.

@Tyriar
Copy link
Member Author

Tyriar commented May 5, 2018

I'm going to revert this feature, see #49162 (comment)

@Tyriar Tyriar closed this as completed May 5, 2018
@Tyriar Tyriar removed this from the Backlog milestone May 5, 2018
@Tyriar Tyriar added invalid Issue identified as not relevant or not valid and removed bug Issue identified by VS Code Team member as probable bug labels May 5, 2018
@vscodebot vscodebot bot locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
invalid Issue identified as not relevant or not valid terminal Integrated terminal issues
Projects
None yet
Development

No branches or pull requests

2 participants