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

Provide the CloseWindow warning experience for the 'X' button #3049

Conversation

KaiyuWang16
Copy link
Contributor

@KaiyuWang16 KaiyuWang16 commented Oct 3, 2019

This is a follow up project of #1589 , which warns users of multiple tabs opened when press ALT+F4.
Now we are going to connect this experience with the 'X' button of the window.

For now, this PR only includes the implementation for IslandWindow (client window). Non-client window experience is still under implementation.

References

PR Checklist

  • Closes #xxx
  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed
  • Requires documentation to be updated
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

To Test:

  1. Start terminal.
  2. Open multiple tabs.
  3. Click 'X' on the top right corner of the window.
  4. You should be able to see a warning. If you cancel it, then the Terminal keeps running.
  5. If you click 'Close', then the app will close.
  6. If you only have one tab opened, and click 'X', the app should close directly without any warnings.

Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I'm fine with this.

src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved
src/cascadia/WindowsTerminal/AppHost.cpp Outdated Show resolved Hide resolved
@ghost ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Oct 4, 2019
@DHowett-MSFT
Copy link
Contributor

please make sure your pull request names are reasonable ("dev/kawa/1589 do the thing" is not a great branch name)

Pretend that they are completing the sentence, "This pull request will ..."

@KaiyuWang16 KaiyuWang16 changed the title Dev/kawa/1589 follow up connect close window action with x button Dev/kawa/Provide warning experience of multiple opened tabs for 'X' button Oct 10, 2019
@KaiyuWang16
Copy link
Contributor Author

please make sure your pull request names are reasonable ("dev/kawa/1589 do the thing" is not a great branch name)
Pretend that they are completing the sentence, "This pull request will ..."

PR name changed. I wonder if I need to change the branch name ? This may require to post a new PR.

@DHowett-MSFT
Copy link
Contributor

No, you can keep the branch name.

@KaiyuWang16 KaiyuWang16 changed the title Dev/kawa/Provide warning experience of multiple opened tabs for 'X' button Provide CloseWindow warning experience for the 'X' button Oct 10, 2019
Copy link
Contributor

@DHowett-MSFT DHowett-MSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@DHowett-MSFT DHowett-MSFT changed the title Provide CloseWindow warning experience for the 'X' button Provide the CloseWindow warning experience for the 'X' button Oct 11, 2019
@DHowett-MSFT DHowett-MSFT merged commit 82dd0b9 into master Oct 11, 2019
@DHowett-MSFT DHowett-MSFT deleted the dev/kawa/1589-follow-up-connect-CloseWindow-Action-with-X-button branch October 11, 2019 00:09
@ghost
Copy link

ghost commented Oct 23, 2019

🎉Windows Terminal Preview v0.6.2951.0 has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants