Add a setting for sending a notification on BEL#20011
Add a setting for sending a notification on BEL#20011carlos-zamora wants to merge 3 commits intodev/cazamor/toast/basefrom
Conversation
|
The only change I made was I changed the settings UI string from "Send notification" to "Desktop notification"
Separately, I noticed that the default here is
I also played around with it and I noticed this:
4. Click notification
5. Get a new Terminal window!
I double checked and we are hitting this code in terminal/src/cascadia/TerminalApp/AppCommandlineArgs.cpp Lines 1071 to 1078 in 4357c17 Shouldn't we refocus the tab/pane that sent the notification instead? @zadjii-msft thoughts? |
4357c17 to
aeb531f
Compare
324223d to
a1048d0
Compare
| // Send a desktop toast notification if requested, but only if | ||
| // the pane isn't already in the belled state. This prevents | ||
| // sending repeated toasts for repeated BEL characters. | ||
| if (bellArgs.SendNotification() && !tab->_tabStatus.BellIndicator()) | ||
| { | ||
| tab->TabToastNotificationRequested.raise(tab->Title(), L"", tab->TabViewIndex()); | ||
| } |
There was a problem hiding this comment.
Would this send a toast every 3s if I send a bell every 3s? Is it a concern to us to throttle this even further? (I'd probably lean to "no", personally.)
There was a problem hiding this comment.
We have a throttle in DesktopNotification here:
terminal/src/cascadia/TerminalApp/DesktopNotification.cpp
Lines 20 to 34 in 9df166e
It's currently set to 5 seconds:
The flow is:
TabToastNotificationRequested --> TerminalPage::_SendDesktopNotification() --> DesktopNotification::SendNotification()
So I think the 5s throttle is good, but we can adjust it if we want.
a1048d0 to
99a2976
Compare
99a2976 to
db209a1
Compare
db209a1 to
16581fe
Compare


Summary of the Pull Request
Targets #20010
Adds another
bellStyleflag option. This sends a windows toast notification to the user when a BEL is encounteredHeavily based on #19936
Co-authored by @zadjii-msft