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.

Sign up for GitHub

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

Tray Icon PR followup #10938

Merged
4 commits merged into from
Aug 19, 2021
Merged

Tray Icon PR followup #10938

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2c2a880
accidentally kept monarch as defapp server, destroy tray icon if monarch
leonMSFT Aug 12, 2021
fdd14b3
move tray icon cleanup to somewhere that's not the destructor
leonMSFT Aug 14, 2021
b94e199
ensure we're not dead when we want to cleanup, move to the bg thread …
leonMSFT Aug 16, 2021
dd8d07e
formatting
leonMSFT Aug 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 9 additions & 8 deletions src/cascadia/WindowsTerminal/AppHost.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,11 +92,6 @@ AppHost::AppHost() noexcept :

AppHost::~AppHost()
{
if (_window->IsQuakeWindow())
{
_windowManager.RequestHideTrayIcon();
}

// destruction order is important for proper teardown here
_window = nullptr;
_app.Close();
Expand Down Expand Up @@ -324,6 +319,15 @@ void AppHost::AppTitleChanged(const winrt::Windows::Foundation::IInspectable& /*
// - <none>
void AppHost::LastTabClosed(const winrt::Windows::Foundation::IInspectable& /*sender*/, const winrt::TerminalApp::LastTabClosedEventArgs& /*args*/)
Copy link
Member

Choose a reason for hiding this comment

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

you might hate me for asking . . . but does this fire when you use the [x] on the window? I know that we try to kill all the tabs, but this might only be a side effect... unless it is the main effect 😀

Copy link
Contributor Author

@leonMSFT leonMSFT Aug 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It does in fact go off on the X button 🙂

{
if (_windowManager.IsMonarch() && _trayIcon)
{
_DestroyTrayIcon();
}
else if (_window->IsQuakeWindow())
{
_HideTrayIconRequested();
}

_window->Close();
}

Expand Down Expand Up @@ -656,9 +660,6 @@ void AppHost::_BecomeMonarch(const winrt::Windows::Foundation::IInspectable& /*s
{
_setupGlobalHotkeys();

// The monarch is just going to be THE listener for inbound connections.
_listenForInboundConnections();
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this change. Otherwise I'd approve this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh this was originally removed as part of this defapp bug fix #10823, and my bad merge left it in 😥. In short, the fix made it so that instead of making the monarch receive default terminal handoffs by default, the monarch should first figure out which peasant (or itself) should handle a default terminal handoff and tell that window to receive the connection.


if (_windowManager.DoesQuakeWindowExist() ||
_window->IsQuakeWindow() ||
(_logic.GetAlwaysShowTrayIcon() || _logic.GetMinimizeToTray()))
Expand Down