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

"You are about to close a read-only terminal" popup does not appear if I select "Close Tab" from the menu again #9502

Closed
KalleOlaviNiemitalo opened this issue Mar 15, 2021 · 7 comments
Labels
Area-User Interface Issues pertaining to the user interface of the Console or Terminal Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) 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

Environment

Windows build number: 10.0.19042.867
Windows Terminal version (if applicable): Preview 1.7.572.0

Steps to reproduce

  1. Start a new session, e.g. Command Prompt.
  2. Press Ctrl+Shift+P to open the command palette, and choose "Toggle pane read-only mode". A lock icon appears in the tab header.
  3. Right-click the tab header. A menu opens.
  4. Select "Close Tab" from the menu. A warning pops up: "You are about to close a read-only terminal. Do you wish to continue?"
  5. Click "Cancel".
  6. Right-click the tab header again. A menu opens.
  7. Select "Close Tab" from the menu.

Expected behavior

The same warning should pop up again.

Actual behavior

The warning does not pop up and the tab does not close.

Notes

If you try to close the pane by pressing Ctrl+Shift+W or choosing "Close pane" from the command palette, then that pops up the warning each time.

Related to #6981.

@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 Mar 15, 2021
@KalleOlaviNiemitalo
Copy link
Author

KalleOlaviNiemitalo commented Mar 15, 2021

TerminalPage::_ShowCloseReadOnlyDialog is called from two functions:

TerminalPage::_RemoveTab is called from four functions:

TerminalPage::_RemoveTabViewItem, TerminalPage::_CloseFocusedTab, and TerminalPage::_CloseFocusedPane discard the IAsyncAction returned by TerminalPage::_RemoveTab, instead of awaiting it. Does that cause the coroutine to be aborted so that some cleanup is skipped?

@zadjii-msft
Copy link
Member

Huh. That's weird. Not totally sure why, but that's definitely happening. Thanks!

/cc @Don-Vito this seems like the kind of thing that interests you 😋

@zadjii-msft zadjii-msft added Area-User Interface Issues pertaining to the user interface of the Console or Terminal Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Terminal The new Windows Terminal. labels Mar 15, 2021
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Mar 15, 2021
@zadjii-msft zadjii-msft added this to the Terminal v2.0 milestone Mar 15, 2021
@KalleOlaviNiemitalo
Copy link
Author

If discarding the IAsyncAction really causes this, then can the compiler be made to warn about such usage, perhaps by making C++/WinRT emit [[nodiscard]] somewhere?

@DHowett DHowett removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Mar 15, 2021
@Don-Vito
Copy link
Contributor

@zadjii-msft - please assign to me 😄
@KalleOlaviNiemitalo - thanks for the research (congrats for the profile picture!)

@Don-Vito
Copy link
Contributor

So the problem is that click on the "close tab" goes directly for the tab->rootPane->Close(), rather than asking the TerminalPage to remove the tab (which is not very successful design decision IMHO). Unfortunately, Pane::Close sets isClosing to true, even if the closing is dismissed, and as a result the next invocations of Pane::Close do nothing:

void Pane::Close()
{
    // We should not set the Pane to closing state if Closing is dismissed.
    if (!_isClosing.exchange(true))
    {        
        _ClosedHandlers(nullptr, nullptr);
    }
}

@ghost ghost added In-PR This issue has a related PR 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 Mar 21, 2021
@ghost ghost closed this as completed in a5ff745 Mar 22, 2021
DHowett pushed a commit that referenced this issue Apr 2, 2021
## Summary of the Pull Request
Currently a repeated attempt to close a read-only tab from context menu,
will bring the terminal into invalid state if user dismisses close action.

There are two root causes for this:
1. The tab close menu triggers the closing of the root pane
(rather than invoking close tab flow in the Terminal Page).
2. Currently panes are not aware that the closing was canceled,
and thus they trigger the Closed event, putting the system in a weird state,
where the Closed handlers were invoked, but the Pane remains.

This PR mitigates #9502, by addressing the first root cause
(the fix is trivial and hopefully can be serviced).
Moreover, it addresses the only existing UI flow that can trigger the issue.

The remaining problematic flow will occur when the connection is closed.
I have created a separate Issue to track it:
#9572
as I guess the PR for it might be more complex.

<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [x] Closes #9502
* [x] CLA signed.
* [ ] Tests added/passed
* [ ] Documentation updated.
* [ ] Schema updated.
* [ ] I've discussed this with core contributors already.

(cherry picked from commit a5ff745)
@ghost
Copy link

ghost commented Apr 14, 2021

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

Handy links:

@ghost
Copy link

ghost commented Apr 14, 2021

🎉This issue was addressed in #9571, which has now been successfully released as Windows Terminal Preview v1.8.1032.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 Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) 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

No branches or pull requests

4 participants