Introduce read-only panes #8867

Don-Vito · 2021-01-24T01:07:48Z

Summary of the Pull Request

Introduces read-only panes.
When pane is marked as read-only:

Attempt to provide user input results in a warning
Attempt to close pane - shows dialog
Attempt to close hosting tab shows dialog
The hosting tab has no close button

PR Checklist

Closes Read-only session mode to prevent accidentally killing a long-lived process #6981
CLA signed.
Tests added/passed
Documentation updated - not yet.
Schema updated.
I've discussed this with core contributors already.

Detailed Description of the Pull Request / Additional comments

The readonly logic implemented in TermControl
(and prevents any send input)
Special handling is required to allow key-bindings
The "close-readonly" protections are in TerminalPage.
The indication that the pane is readonly is done using lock glyph
The indication that the tab contains readonly pane
is done by hiding the close button of the tab
The readonly mode is enabled by keyboard shortcut
(the followup might add this to the context menu)

Validation Steps Performed

Don-Vito · 2021-01-24T01:13:30Z

Some highlights:

carlos-zamora

Some other notes:

mildly concerned if "Close anyway" should be the primary button
mildly concerned about some of the async changes to closing tabs
So, honestly, I'd want a design signoff from @cinnamon-msft and a closer look from @zadjii-msft (or @DHowett) for async tab closing.

Mostly nits, and I don't see any real issues, so I'm approving.

src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp

src/cascadia/TerminalControl/TermControl.h

src/cascadia/TerminalApp/TerminalTab.cpp

src/cascadia/TerminalApp/Pane.h

src/cascadia/TerminalApp/Resources/en-US/Resources.resw

Don-Vito · 2021-01-25T22:50:55Z

Some other notes:

* [ ]  mildly concerned if "Close anyway" should be the primary button

* [ ]  mildly concerned about some of the async changes to closing tabs
  So, honestly, I'd want a design signoff from @cinnamon-msft and a closer look from @zadjii-msft (or @DHowett) for async tab closing.

Mostly nits, and I don't see any real issues, so I'm approving.

Yeah. I am mostly worried about the async nature of remove tab. Hope I didn't miss anything.
Regarding the primary button - I absolutely agree and will change it!

zadjii-msft

This mostly seems fine to me. I'm a little worried about the race in closing a pane. Hopefully if we move that logic within the tab itself, we can mitigate that.

src/cascadia/TerminalControl/TermControl.cpp

src/cascadia/TerminalControl/TermControl.idl

src/cascadia/TerminalApp/TerminalPage.cpp

zadjii-msft

Okay I found an unrelated crash while playing with this, but this seems good to me. Thanks! @cinnamon-msft will come around later to write up a docs page for this

src/cascadia/TerminalApp/TerminalPage.cpp

zadjii-msft · 2021-02-03T13:41:30Z

src/cascadia/TerminalApp/TerminalPage.cpp

+                        ContentDialogResult warningResult = co_await _ShowCloseReadOnlyDialog();
+
+                        // The primary action is canceling the action
+                        if (warningResult == ContentDialogResult::Primary)
+                        {
+                            co_return;
+                        }
+
+                        // Clean read-only mode to prevent additional prompt if closing the pane triggers closing of a hosting tab
+                        if (control.ReadOnly())
+                        {
+                            control.ToggleReadOnly();
+                        }
+                    }
+
+                    pane->Close();


Note to self: what happens if we run sleep 5 ; exit, then during the sleep mark a pane as read-only, then try to close the pane, then wait before confirming. We'll get the dialog, then the pane will close on its own, then we'll try to close it again. Is that fine (probably)?

Yea probably, because of the atomic added in Pane

Yea, this was fine. Nothing to worry about here.

cinnamon-msft · 2021-02-03T19:19:13Z

I could see this in the Panes page of our docs, and also in our Tips and Tricks page. :)

I'm happy to write up the docs for the 1.7 release with this!

src/cascadia/TerminalControl/Resources/en-US/Resources.resw

Don-Vito · 2021-02-04T19:14:41Z

@zadjii-msft - what are the next steps here? I am worried about conflicts with #9024 😊

Don-Vito · 2021-02-04T19:15:31Z

I could see this in the Panes page of our docs, and also in our Tips and Tricks page. :)

I'm happy to write up the docs for the 1.7 release with this!

I have created an issue in the docs repo: MicrosoftDocs/terminal#241

PankajBhojwani · 2021-02-04T19:58:58Z

what are the next steps here? I am worried about conflicts with #9024 😊

I anticipate that #9024 is going to take a while to both be finished and get signoffs, so my opinion is that we should merge this first and I'll deal with the conflicts over there

Don-Vito · 2021-02-08T14:17:16Z

@zadjii-msft - can I somehow help moving this forward? Actually there is a conflict..

zadjii-msft · 2021-02-08T15:23:35Z

Whoops, that's my B for not slapping automerge on this. I've resolved the conflict with #9034, and hopefully that will automerge itself in ~15 minutes. Sorry about that!

ghost · 2021-02-08T15:23:48Z

Hello @zadjii-msft!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

Don-Vito · 2021-02-08T15:56:00Z

@zadjii-msft - there was some build error, so I re-merged. And now it is not auto-merge anymore 😊

Don-Vito · 2021-02-08T17:51:15Z

Seems to be building now 😛

DHowett

Well done. Thanks!

rluvaton · 2021-02-27T23:17:18Z

Great feature! In which version this will be available?

DHowett · 2021-02-27T23:18:23Z

Any feature pull request that is merged will be available in the preview build that comes out after it was merged. Usually. :)

rluvaton · 2021-02-27T23:20:42Z

Any feature pull request that is merged will be available in the preview build that comes out after it was merged. Usually. :)

Yeah, but I don't see it in the release notes

DHowett · 2021-02-27T23:21:41Z

Preview version 1.7 hasn’t been released yet, so I am not sure what release notes you’re talking about 😄

rluvaton · 2021-02-27T23:24:13Z

@DHowett Ohh, thanks! by the way, this is the BEST terminal I ever use! thank you so much for the active development and bringing cool and useful features in no time!

Can't wait for this to be the default terminal.
Don't know how I used such bad terminals before!

ghost · 2021-03-01T18:03:51Z

🎉Windows Terminal Preview v1.7.572.0 has been released which incorporates this pull request.:tada:

Handy links:

Don-Vito added 9 commits January 20, 2021 04:04

Introduce read-only panes

6416347

Merge branch 'main' into 6981-readonly-tabs

f80fc21

Introduce ReadOnl property for tab

46071db

Document ReadOnly computation in the Pane

2861a1b

Fix close read-only dialog texts

13476f5

Fix race while waiting for the read-only dialog confirmation

c4b17c5

Introduce snapshot to resolve possible races

4e2a905

Add toggle read-only pane command to documents

3926930

Introduce warning when closing a read-only pane

ed0f30e

ghost added 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. labels Jan 24, 2021

Don-Vito changed the title ~~Introduce Read-Only Panes~~ Introduce read-only panes Jan 24, 2021

carlos-zamora approved these changes Jan 25, 2021

View reviewed changes

zadjii-msft mentioned this pull request Jan 26, 2021

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

Closed

Don-Vito added 7 commits January 26, 2021 21:34

Merge branch 'main' into 6981-readonly-tabs

a69aec6

Merge branch 'main' into 6981-readonly-tabs

073065e

Change the primary button of close readonly dialog to cancel

c5ca651

Make ContainsReadOnly to be const

4fc0ec6

Change the dialog text + fix the button names

c5b75bd

Rename the action into toggleReadOnlyMode

d72ab1e

Fix command name in schema

83402fa

Don-Vito requested review from carlos-zamora and zadjii-msft January 26, 2021 21:55

zadjii-msft self-assigned this Jan 26, 2021

zadjii-msft reviewed Jan 27, 2021

View reviewed changes

zadjii-msft assigned Don-Vito and unassigned zadjii-msft Jan 27, 2021

zadjii-msft approved these changes Feb 3, 2021

View reviewed changes

zadjii-msft removed their assignment Feb 3, 2021

cinnamon-msft reviewed Feb 3, 2021

View reviewed changes

src/cascadia/TerminalControl/Resources/en-US/Resources.resw Outdated Show resolved Hide resolved

Don-Vito added 2 commits February 4, 2021 21:01

Merge branch 'main' into 6981-readonly-tabs

f38a8dd

Fix read-only pane dialog text

3e40287

Don-Vito mentioned this pull request Feb 4, 2021

Document "Locked Panes" before they go live in 1.7 MicrosoftDocs/terminal#241

Closed

Merge remote-tracking branch 'upstream/main' into 6981-readonly-tabs

21636f5

zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Feb 8, 2021

Remove redundant read-only check

e21e352

Don-Vito force-pushed the 6981-readonly-tabs branch from e750f1b to e21e352 Compare February 8, 2021 15:54

ghost removed the AutoMerge Marked for automatic merge by the bot when requirements are met label Feb 8, 2021

zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Feb 8, 2021

ghost merged commit 3230b18 into microsoft:main Feb 8, 2021

DHowett reviewed Feb 8, 2021

View reviewed changes

Don-Vito deleted the 6981-readonly-tabs branch February 28, 2021 17:13

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce read-only panes #8867

Introduce read-only panes #8867

Don-Vito commented Jan 24, 2021

Don-Vito commented Jan 24, 2021

carlos-zamora left a comment

Don-Vito commented Jan 25, 2021

zadjii-msft left a comment

zadjii-msft left a comment

zadjii-msft Feb 3, 2021

zadjii-msft Feb 3, 2021

zadjii-msft Feb 3, 2021

cinnamon-msft commented Feb 3, 2021

Don-Vito commented Feb 4, 2021 •

edited

Don-Vito commented Feb 4, 2021

PankajBhojwani commented Feb 4, 2021 •

edited

Don-Vito commented Feb 8, 2021 •

edited

zadjii-msft commented Feb 8, 2021

ghost commented Feb 8, 2021

Don-Vito commented Feb 8, 2021 •

edited

Don-Vito commented Feb 8, 2021

DHowett left a comment

rluvaton commented Feb 27, 2021

DHowett commented Feb 27, 2021

rluvaton commented Feb 27, 2021

DHowett commented Feb 27, 2021

rluvaton commented Feb 27, 2021 •

edited

ghost commented Mar 1, 2021

Introduce read-only panes #8867

Introduce read-only panes #8867

Conversation

Don-Vito commented Jan 24, 2021

Summary of the Pull Request

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Don-Vito commented Jan 24, 2021

carlos-zamora left a comment

Choose a reason for hiding this comment

Don-Vito commented Jan 25, 2021

zadjii-msft left a comment

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

zadjii-msft Feb 3, 2021

Choose a reason for hiding this comment

zadjii-msft Feb 3, 2021

Choose a reason for hiding this comment

zadjii-msft Feb 3, 2021

Choose a reason for hiding this comment

cinnamon-msft commented Feb 3, 2021

Don-Vito commented Feb 4, 2021 • edited

Don-Vito commented Feb 4, 2021

PankajBhojwani commented Feb 4, 2021 • edited

Don-Vito commented Feb 8, 2021 • edited

zadjii-msft commented Feb 8, 2021

ghost commented Feb 8, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Don-Vito commented Feb 8, 2021 • edited

Don-Vito commented Feb 8, 2021

DHowett left a comment

Choose a reason for hiding this comment

rluvaton commented Feb 27, 2021

DHowett commented Feb 27, 2021

rluvaton commented Feb 27, 2021

DHowett commented Feb 27, 2021

rluvaton commented Feb 27, 2021 • edited

ghost commented Mar 1, 2021

Don-Vito commented Feb 4, 2021 •

edited

PankajBhojwani commented Feb 4, 2021 •

edited

Don-Vito commented Feb 8, 2021 •

edited

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

Don-Vito commented Feb 8, 2021 •

edited

rluvaton commented Feb 27, 2021 •

edited