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

Enable Terminal closing with ALT + F4 and warning of multiple open tabs #2526

Merged

Conversation

KaiyuWang16
Copy link
Contributor

@KaiyuWang16 KaiyuWang16 commented Aug 24, 2019

Summary of the Pull Request

This PR is for: #1589
We add a key binding called "CloseWindow", which will close the whole terminal, and warn the user if multiple tabs are opened. The user could choose to cancel or continue the closing action.

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

In my Page-App refactoring change: #2208, _showDialog's signature is changed. I only pass a ContentDialog to it and the contents are constructed in each specific dialog initialization methods. In this PR, I follow this pattern.

Testing:

  1. check-out this branch and build the project
  2. Start terminal app
  3. Open settings, add
    {
    "command": "closeWindow",
    "keys": [
    "alt+f4"
    ]
    }
    to the end of the "keybindings" list.
  4. Open multiple tabs by clicking "+"
  5. Press alt + F4.
  6. On the dialog, choose ""cancel", then the dialog will close and nothing happens.
  7. Press alt + F4. On the dialog, click "OK". Then the app will close.

image

@KaiyuWang16 KaiyuWang16 self-assigned this Aug 27, 2019
src/cascadia/TerminalApp/App.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/App.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/App.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/App.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 Aug 28, 2019
src/cascadia/TerminalApp/App.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/App.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 Aug 28, 2019
@DHowett-MSFT
Copy link
Contributor

I like this. We should file a followup workitem for the following additional work:

  • When the user clicks on the X button, they should receive the same prompt.
    • This must work for the IslandWindow and the NonClientIslandWindow.
    • The close must be suppressed if the user says No.

@DHowett-MSFT
Copy link
Contributor

Is there a way we can get an x in the top right corner of this dialog, like Edge has?

image

@KaiyuWang16
Copy link
Contributor Author

Is there a way we can get an x in the top right corner of this dialog, like Edge has?

Seems that ContentDialog does not have this by default. We could customize ContentDialog, but in this way we will lose the built-in title/text/buttons and will have to provide those by ourselves.

Copy link

@codendone codendone left a comment

Choose a reason for hiding this comment

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

Couple small changes suggested. Otherwise looks good.

src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

Does this work with both light and dark mode? Are the buttons visible even when the app's theme is opposite the system theme? This probably won't update if you change the theme while the dialog is open, but I don't think any of ours do ATM.

Overall this seems like a fine starting point to me. I'd love to have a lot more customization around this in the future, but this is a good enough start. I still have a couple questions so I'm not going to sign off quite yet.

src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved
@KaiyuWang16
Copy link
Contributor Author

KaiyuWang16 commented Sep 6, 2019

Does this work with both light and dark mode? Are the buttons visible even when the app's theme is opposite the system theme? This probably won't update if you change the theme while the dialog is open, but I don't think any of ours do ATM.
Overall this seems like a fine starting point to me. I'd love to have a lot more customization around this in the future, but this is a good enough start. I still have a couple questions so I'm not going to sign off quite yet.

In "dark" theme, the content dialog will change to "dark" but the buttons remain the same:

image

@zadjii-msft zadjii-msft added Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Sep 9, 2019
src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/Resources/en-US/Resources.resw Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalPage.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 Sep 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.

Before you merge this, please change the title.

Pretend that you are completing the sentence, "This commit will..."

Remember: pull request titles and commit titles are supposed to read like e-mail subject lines. They should be concise, imperative, and convey enough useful information so that somebody looking at history will be able to understand them.
They are permanent.

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 16, 2019
@KaiyuWang16 KaiyuWang16 changed the title Dev/kawa/1589 alt+f4 closing with warning of multiple tabs Enable Terminal closing with ALT + F4 and warning of multiple open tabs Sep 17, 2019
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 17, 2019
@KaiyuWang16
Copy link
Contributor Author

Before you merge this, please change the title.
Pretend that you are completing the sentence, "This commit will..."
Remember: pull request titles and commit titles are supposed to read like e-mail subject lines. They should be concise, imperative, and convey enough useful information so that somebody looking at history will be able to understand them.
They are permanent.

Title changed. Will pay attention to the PR and commit titles in the future.

@DHowett-MSFT
Copy link
Contributor

Thanks!

@KaiyuWang16 KaiyuWang16 merged commit 5806cda into master Sep 17, 2019
@ghost
Copy link

ghost commented Sep 24, 2019

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

Handy links:

@xieofxie
Copy link

xieofxie commented Sep 27, 2019

Hi, please add warning for clicking 'close' on the upper right too~ Thanks~

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-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants