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

Read-only session mode to prevent accidentally killing a long-lived process #6981

Closed
KalleOlaviNiemitalo opened this issue Jul 20, 2020 · 16 comments · Fixed by #8867
Closed
Labels
Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone

Comments

@KalleOlaviNiemitalo
Copy link

Description of the new feature/enhancement

I sometimes run ROBOCOPY or another long-lived process in Windows Terminal. I then fear that I might accidentally close the tab or hit Ctrl+C and have to start over. I would like to mark the session or tab as protected (important and read-only) and have Windows Terminal protect me from such mistakes.

I already mark such tabs with a distinctive color but they still feel risky.

Proposed technical implementation details (optional)

Add a "Read only" check box to the per-tab menu. When the check box is checked:

  • Do not forward any keyboard events to processes using the pseudoconsole. Instead, if the user types something, display a modeless notification that advises the user how to disable the read-only mode.
  • Do not forward any mouse events to processes using the pseudoconsole. Instead, let mouse events scroll the scrollback and select text. This should override the ENABLE_MOUSE_INPUT mode and perhaps also ENABLE_QUICK_EDIT_MODE.
  • Do not display a close button on the tab. This may also make it consume less horizontal space, depending on the "tabWidthMode" setting.
  • If the user tries to close the window, display a confirmation dialog box that includes the titles of the read-only tabs. This should override the "confirmCloseAllTabs" setting.
  • If the user tries to close the tab (with a key combination, as the close button is hidden), then either display a notification as with keyboard events, or prompt for confirmation.

There could be a key-bindable action to turn the read-only mode on and off.

@KalleOlaviNiemitalo KalleOlaviNiemitalo added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label Jul 20, 2020
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Jul 20, 2020
@KalleOlaviNiemitalo
Copy link
Author

This seems related to pinning tabs (#5969) but here I don't care about automatically restarting the long-lived processes, as the work would have been lost already.

@DHowett
Copy link
Member

DHowett commented Jul 27, 2020

You know what, I actually like this idea. Locked sessions or something.
I was going to close it in favor of the "are you sure you want to close? this is running ROBOCOPY.EXE!" prompt, but pinned locked tabs are a pretty cool idea.

@DHowett DHowett added this to Spec Needed ❓ in Specification Tracker via automation Jul 27, 2020
@DHowett DHowett added this to the Terminal Backlog milestone Jul 27, 2020
@DHowett DHowett added Area-User Interface Issues pertaining to the user interface of the Console or Terminal Product-Terminal The new Windows Terminal. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Jul 27, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jul 27, 2020
@KalleOlaviNiemitalo
Copy link
Author

#6372 is another feature request for long-lived noninteractive processes. These could be useful together but I don't think the implementations would have anything in common.

@ghost ghost mentioned this issue Oct 22, 2020
@Don-Vito
Copy link
Contributor

@DHowett, @zadjii-msft - I have a PoC. Still not pushing as it is pretty dirty and doesn't handle all scenarios. Please let me know if UX makes sense:

ReadOnlyTabs

@zadjii-msft
Copy link
Member

wow holy crap. That's like, the whole thing!

I think the only thing I'm not positive on is the "If you try to close the window while one pane is read-only, we close all the non-read-only tabs, then prompt for the closing of the read-only pane". In my (very subjective) opinion, the "close readonly tab" warning would supersede the "close all tabs" warning and we'd only display one warning.

You know what, I've watched this gif like 8 times while typing this up and I think it's great as it is. I guess in my head I was trying to reconcile that warning with all the scenarios from #6549 - in all those I was still expecting only one dialog. This scenario though kinda necessitates a secondary dialog just for closing the read-only pane, regardless of the confirmClose... settings.

Lemme cycle this through to the team and see if they have other feedback 😄.

@KalleOlaviNiemitalo
Copy link
Author

I wonder if ShutdownBlockReasonCreate would make sense here. Perhaps not, if the user has an SSH connection in the Windows Terminal session and runs tmux or screen on the server.

@zadjii-msft
Copy link
Member

@KalleOlaviNiemitalo Probably not, since there's not a reliable way for a client application to get at the HWND of the Terminal (and we're not about to provide one either). You've called out the exact reason why - it's not going to make sense in ssh sessions, or sessions where the Windows Terminal isn't the end terminal application. So I'd rather not open a door that's not going to work 90% of the time.

@KalleOlaviNiemitalo
Copy link
Author

KalleOlaviNiemitalo commented Jan 20, 2021

I meant Windows Terminal itself could call ShutdownBlockReasonCreate when the user enables read-only mode in a session. On the basis that, if the user does not want to terminate processes in the session by closing a tab in Windows Terminal, then the user probably does not want to terminate them by shutting down Windows, either. In the ssh + tmux scenario though, closing the tab or shutting down Windows is not really a problem because the remote tmux session can be resumed later, but read-only mode can still be useful for blocking Ctrl+C.

@ghost ghost added the In-PR This issue has a related PR label Jan 24, 2021
@sba923
Copy link

sba923 commented Jan 26, 2021

Very good idea. Count me in as a tester whenever there's code that implements this.

@zadjii-msft
Copy link
Member

Good thing, because there's a PR for it already:

image

See #8867

@sba923
Copy link

sba923 commented Jan 26, 2021

What would be the right route to help testing this? Wait until #8867 gets merged in?

@zadjii-msft
Copy link
Member

I mean, you could wait if you want. If you're ambitious, you could always check out the PR and build it locally. I usually do that with a commandline like:

git fetch origin pull/8867/head:pull/8867 && git checkout pull/8867

@Don-Vito
Copy link
Contributor

Don-Vito commented Feb 5, 2021

I meant Windows Terminal itself could call ShutdownBlockReasonCreate when the user enables read-only mode in a session. On the basis that, if the user does not want to terminate processes in the session by closing a tab in Windows Terminal, then the user probably does not want to terminate them by shutting down Windows, either. In the ssh + tmux scenario though, closing the tab or shutting down Windows is not really a problem because the remote tmux session can be resumed later, but read-only mode can still be useful for blocking Ctrl+C.

@KalleOlaviNiemitalo - Right now I moved forward without invoking ShutdownBlockReasonCreate. I am still hesitating if it is a good idea. We could probably add it as an option to the lock command

{ "command": { "action": "toggleReadOnlyMode", "blockShutdown": true }}

If you like the idea, probably we can create a follow up task.

@DHowett
Copy link
Member

DHowett commented Feb 5, 2021

(I'm in love with this feature. It's so cool.)

@KalleOlaviNiemitalo
Copy link
Author

KalleOlaviNiemitalo commented Feb 5, 2021

@Don-Vito I think it's best not to call ShutdownBlockReasonCreate now. Minimize the risk of surprising effects.

(I assume ShutdownBlockReasonCreate has no effect on whether Microsoft Store can forcefully terminate the app to install updates.)

@ghost ghost closed this as completed in #8867 Feb 8, 2021
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Feb 8, 2021
ghost pushed a commit that referenced this issue Feb 8, 2021
## Summary of the Pull Request
Introduces read-only panes.
When pane is marked as read-only:
1. Attempt to provide user input results in a warning
2. Attempt to close pane - shows dialog
3. Attempt to close hosting tab shows dialog
4. The hosting tab has no close button

## PR Checklist
* [x] Closes #6981
* [x] CLA signed. 
* [ ] Tests added/passed
* [ ] Documentation updated - not yet.
* [x] Schema updated.
* [ ] I've discussed this with core contributors already. 

## Detailed Description of the Pull Request / Additional comments
1. The readonly  logic implemented in `TermControl`
(and prevents any send input)
2. Special handling is required to allow key-bindings
3. The "close-readonly" protections are in TerminalPage.
4. The indication that the pane is readonly is done using lock glyph
5. The indication that the tab contains readonly pane
is done by hiding the close button of the tab
6. The readonly mode is enabled by keyboard shortcut
(the followup might add this to the context menu)

## Validation Steps Performed
@ghost
Copy link

ghost commented Mar 1, 2021

🎉This issue was addressed in #8867, which has now been successfully released as Windows Terminal Preview v1.7.572.0.:tada:

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants