-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Add support for OSC777 - send notification #14425
base: main
Are you sure you want to change the base?
Changes from 13 commits
ce375fa
65bc163
67d8548
006da6a
f0f75dc
e9b2e51
1fd87fe
054f173
0f339d2
8e170eb
3d83cc3
c4f623a
8b67ed7
9208222
cae6f04
654416c
7b524b0
6ac5137
75ea5f3
dc448b4
3b02c96
1726176
015c5e8
8309901
4ab628d
d234049
d3a98b3
eac27db
9a6ded2
70d905b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,6 +39,9 @@ using namespace ::Microsoft::Console; | |
using namespace ::Microsoft::Terminal::Core; | ||
using namespace std::chrono_literals; | ||
|
||
using namespace winrt::Windows::UI::Notifications; | ||
using namespace winrt::Windows::Data::Xml::Dom; | ||
|
||
#define HOOKUP_ACTION(action) _actionDispatch->action({ this, &TerminalPage::_Handle##action }); | ||
|
||
namespace winrt | ||
|
@@ -1508,6 +1511,7 @@ namespace winrt::TerminalApp::implementation | |
}); | ||
|
||
term.ShowWindowChanged({ get_weak(), &TerminalPage::_ShowWindowChangedHandler }); | ||
term.SendNotification({ get_weak(), &TerminalPage::_SendNotificationHandler }); | ||
} | ||
|
||
// Method Description: | ||
|
@@ -2419,6 +2423,81 @@ namespace winrt::TerminalApp::implementation | |
_ShowWindowChangedHandlers(*this, args); | ||
} | ||
|
||
winrt::fire_and_forget TerminalPage::_SendNotificationHandler(const IInspectable sender, | ||
const Microsoft::Terminal::Control::SendNotificationArgs args) | ||
{ | ||
auto weakThis = get_weak(); | ||
|
||
co_await resume_foreground(Dispatcher()); | ||
auto page{ weakThis.get() }; | ||
if (page) | ||
{ | ||
// If the window is inactive, we alway want to send the notification. | ||
zadjii-msft marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// | ||
// Otherwise, we only want to send the notification for panes in inactive tabs. | ||
if (_activated) | ||
{ | ||
auto foundControl = false; | ||
if (const auto activeTab{ _GetFocusedTabImpl() }) | ||
{ | ||
activeTab->GetRootPane()->WalkTree([&](auto&& pane) { | ||
if (const auto& term{ pane->GetTerminalControl() }) | ||
{ | ||
if (term == sender) | ||
zadjii-msft marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
foundControl = true; | ||
return; | ||
} | ||
} | ||
}); | ||
} | ||
|
||
// The control that sent this is in the active tab. We | ||
// should only send the notification if the window was | ||
// inactive. | ||
if (foundControl) | ||
{ | ||
co_return; | ||
} | ||
} | ||
|
||
_sendNotification(args.Title(), args.Body()); | ||
} | ||
} | ||
|
||
void TerminalPage::_sendNotification(const std::wstring_view title, | ||
const std::wstring_view body) | ||
{ | ||
static winrt::hstring xmlTemplate{ L"\ | ||
<toast>\ | ||
<visual>\ | ||
<binding template=\"ToastGeneric\">\ | ||
<text></text>\ | ||
<text></text>\ | ||
</binding>\ | ||
</visual>\ | ||
</toast>" }; | ||
Comment on lines
+3041
to
+3048
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lemme tell you, it's a disaster. At least the sample has like, 25% c++winrt code these days. The original sample was "Well, the API it hard to use, so go get this third party nuget package, and do some C#" 🤦 |
||
|
||
XmlDocument doc; | ||
doc.LoadXml(xmlTemplate); | ||
// Populate with text and values | ||
auto payload{ fmt::format(L"window={}&tabIndex=0", WindowId()) }; | ||
doc.DocumentElement().SetAttribute(L"launch", payload); | ||
doc.SelectSingleNode(L"//text[1]").InnerText(title); | ||
doc.SelectSingleNode(L"//text[2]").InnerText(body); | ||
|
||
// Construct the notification | ||
winrt::Windows::UI::Notifications::ToastNotification notif{ doc }; | ||
zadjii-msft marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
if (!_toastNotifier) | ||
{ | ||
_toastNotifier = ToastNotificationManager::CreateToastNotifier(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (not blocking) If a user has multiple windows open, we'll have a toast notifier for each window. Is it worth it at all to have a shared toast notifier across all windows? Like, the monarch acts as the notifier and the rest just ping it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean, theoretically, yea. My gut says the overhead though is not worth the plumbing. That feel right? 🤷 |
||
} | ||
|
||
// And show it! | ||
_toastNotifier.Show(notif); | ||
|
||
} | ||
|
||
// Method Description: | ||
// - Paste text from the Windows Clipboard to the focused terminal | ||
void TerminalPage::_PasteText() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what exactly is expected of this, but it doesn't seem to focus the Terminal window for me. If I've minimized it, it will be restored, but it's still hidden behind whatever other applications I've got open.
Also, if I've switched to another Terminal tab, I would have expected it to switch to the tab that triggered the notification, but that doesn't seem to be the case.
If that's not expected to work, though, that's fine. Just wanted to let you know in case there is a bug here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is based off the same
globalSummon
code, which doesn't always work depending on the FG application. I know for a fact that Task Manager and OneNote don't allow you to summon the window on top of them - what did you have focused?That's something we should definitely do! I'd probably hold that for a follow-up though, to minimize the diffs. Right now I'm not really passing the
tabIndex
parameter, and there's not a good way to "globalSummon
and switch to a tab" all in one go.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried a few things now - Notepad, Firefox, Calculator - haven't yet found anything that worked. Even when I can see it being restored from a minimized state, it restores itself behind the active window.
Maybe it's just my version of Windows? (10.0.19044.2130)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huh. That's... super weird. Does
globalSummon
work for you in this scenario?We do play weird games with inserting a thread into the FG process so we can activate it (unsurprisingly, Windows does not make it easy for an app to bring itself to the FG). But if this didn't work, then I'd assume that
globalSummon
wouldn't work the same....Unless there's something extra weird where the FG application is like,
explorer.exe
itself...I'll spin this up in a VM to check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. That seems to work in most cases. If it's minimized it pops up over the foreground app, and if it's open but in the background, it'll be moved to the foreground.
Although the latter case doesn't seem work with UWP apps I think (I've tried Calculator, Skype, News). If Terminal is hidden/minimized, global summon will open it up in the foreground, but if it's already open, and I've got something like Calculator focused, Terminal doesn't seem able to move itself to the foreground.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. Finally got a chance to try this in the 10 VM. Seems like the notification will restore the Terminal from minimized, but won't bring it to the foreground. If it's open (but not in the FG), then this does seemingly nothing. Interesting.
I think this has to do with the wayglobalSummon
attempts to bring a window to the foreground, but it interacts with the Windows 10 notifications in a VERY weird way. Usually to summon, we do this weird "inject a thread, then SetForeground on that thread" trick so that weI suspect that when we get invoked from the Windows 10 notification, we can'tNow that I've typed that up, that can't be right.
In
IslandWindow::_globalActivateWindow
,ShowWindow(..., SW_RESTORE);
, thenSetWindowPos(..., SWP_NOZORDER | SWP_NOSIZE | SWP_NOACTIVATE)
(to put on the right monitor). We're NOT doing the wacky FG trick here.AttachThreadInput
+ShowWindow
+SetActiveWindow
+SetWindowPos
to take foreground, activate ourselves, and move to the right monitor.However, in neither of these cases does the Terminal get brought to the foreground from a notification in Windows 10. I'd think that the restore one should. Maybe there's a bug in Windows 10? Lemme reach out to a notifications expert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A thought - Outlook and Teams notifications never open the window in the foreground when you click on their notifications (certainly not on Windows 10). That's weird, but maybe a lead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could it perhaps have something to do with Windows trying to prevent focus stealing? I don't know anything about the architecture, so maybe this doesn't apply, but could it help if you called AllowSetForegroundWindow somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost certainly.
You know, that's not a bad thought. That might require some wacky plumbing, but that might work. We'd need to get the PID of the target window to the WindowsTerminal.exe that got spawned, before we do then windowManager activation.... Might be possible? Certainly will be easier after #14843