Show indicator of bell in tab #8637

PankajBhojwani · 2020-12-22T19:58:57Z

When we emit a BEL (visual or audible), show an indicator in the tab
header

If the tab the BEL is coming from is not focused when the BEL is raised,
the indicator in its header will be removed when the tab gains focus. If
the tab was already focused when the BEL was emitted, then the indicator
goes away after 2 seconds.

Closes #8106

skyline75489 · 2020-12-23T02:23:51Z

First of all, love the work, as always. Second, I think we should revisit the "hide bell after 1 second" design. In iTerm2, I observed the following behaviour:

If a bell rings in the currently focused tab, the bell indicator only disappears if there's intereactions from users, for example, typing something.
If a bell rings in another tab, the bell indicator disappears as soon as that tab gains focus.

This design should help people easily find what the actual tab is causing the bell. But if it just disappears automatically, then people might still be confused about it.

PankajBhojwani · 2020-12-23T18:21:36Z

This design should help people easily find what the actual tab is causing the bell. But if it just disappears automatically, then people might still be confused about it.

I think that makes sense, thanks for the suggestion!

If a bell rings in the currently focused tab, the bell indicator only disappears if there's intereactions from users, for example, typing something.

This will bear some thinking about what entails as an interaction

skyline75489 · 2020-12-23T23:20:12Z

My definition of “interaction” so far is just typing something. That is the most common way user would interact with a terminal, I think. But I can’t say for sure that’s the only way to make bell indicator disappears. 获取 Outlook for iOS<https://aka.ms/o0ukef>

PankajBhojwani · 2021-01-07T20:02:45Z

Had a chat with @DHowett, we will implement this as: the bell indicator goes away when the tab receives focus, and if the tab was already focused when the bell was raised then the indicator goes away after a small amount of time.

We are choosing this implementation for now because of the dubiousness of what an interaction means (aside from typing there's also clicking a link for example) and also because of edge cases such as the tab key-up from alt tabbing into the tab being considered a key press.

skyline75489 · 2021-01-11T03:45:15Z

Thanks @PankajBhojwani for the thoughtful implementation. Personally I do not use speakers that much so I’m really looking forward to this feature.

src/cascadia/TerminalApp/Pane.cpp

DHowett

src/cascadia/TerminalApp/Pane.cpp

zadjii-msft · 2021-01-15T15:58:03Z

Okay my initial feedback is regarding this discussion:
#8133 (comment)

Should we put the bell on the right of the tab title? I don't love that it causes the whole string to jump back and forth

zadjii-msft

Okay this is all reasonable to me. I guess my biggest concern is on the bell placement. I really think it should be on the right.

  🌛
💪 💪
  👖

In a follow-up, we could have the bell "fade out", like it does in browsers, as opposed to just immediately hiding.

PankajBhojwani · 2021-01-15T17:58:45Z

I guess my biggest concern is on the bell placement. I really think it should be on the right.

In browsers if you have sound coming from a tab the bell/speaker icon shows up on the left though! Since we chose the progress ring placement to match how browsers do it, should we do the same for the bell?

zadjii-msft · 2021-01-15T18:12:04Z

ah shiz, someone caught me not selfhosting

Edgium:

Firefox

So that's why I'm confused 😬

DHowett · 2021-01-19T23:01:18Z

left! left! left!

zadjii-msft · 2021-01-19T23:06:20Z

Yea I lost this fight

I was outvoted

skyline75489 · 2021-01-20T00:36:52Z

TBH I’m OK with both placement.

Wait, you can dismiss a review? How many features GitHub has added?...

DHowett · 2021-01-21T02:43:29Z

Moved from body

When the tab has focus:

When the tab does not have focus:

DHowett · 2021-01-21T02:46:09Z

src/cascadia/TerminalApp/TerminalTab.cpp

+        auto weakThis{ get_weak() };
+
+        if (auto tab{ weakThis.get() })


This will always pass. Since you constructed the event handler (line 290) with a weak reference, c++/winrt automatically ensures that you have a strong living reference inside this function. You can safely use this or just.. use unqualified members.

DHowett · 2021-01-21T02:47:04Z

src/cascadia/TerminalApp/TerminalTab.cpp

+        if (auto tab{ weakThis.get() })
+        {
+            tab->ShowBellIndicator(false);
+            tab->_bellIndicatorTimer.value().Stop();


Suggested change

tab->_bellIndicatorTimer.value().Stop();

tab->_bellIndicatorTimer->Stop();

DHowett · 2021-01-21T02:49:04Z

src/cascadia/TerminalApp/TerminalTab.cpp

+                    tab->_UpdateActivePane(sender);
+                    tab->_RecalculateAndApplyTabColor();
+                }
+                tab->_focusState = WUX::FocusState::Programmatic;


i hope this is right! what else is _focusState being used for?

The only other places (before this PR) _focusState was being checked/updated: TerminalTab::_RefreshVisualState and TerminalTab::Focus

However, without this addition, the tab does not know when the app itself loses focus (by alt-tab/minimize etc) so it used to still think it is focused even though the app itself and the underlying control(s) are all unfocused.

DHowett · 2021-01-21T02:49:26Z

src/cascadia/TerminalApp/TerminalTab.cpp

+            if (tab)
+            {
+                // update this tab's focus state
+                tab->_focusState = WUX::FocusState::Unfocused;


just to be sure - there will be a moment, when you move from pane A to pane B, that the tab thinks it does not have focus?

DHowett · 2021-01-21T02:50:53Z

src/cascadia/TerminalApp/TerminalTab.cpp

+        if (auto tab{ weakThis.get() })
+        {
+            DispatcherTimer bellIndicatorTimer;
+            bellIndicatorTimer.Interval(std::chrono::milliseconds(2000));
+            bellIndicatorTimer.Tick({ get_weak(), &TerminalTab::_BellIndicatorTimerTick });
+            bellIndicatorTimer.Start();
+            tab->_bellIndicatorTimer.emplace(std::move(bellIndicatorTimer));


can an application DOS us by spamming bell? it will make us start a huge number of timers, and keep overwriting the optional.

(we'll waste memory for up to 2 seconds at which point they'll start to fire)

Oh yes good catch! We now only create a new timer if the optional doesn't have a value

DHowett · 2021-01-21T04:26:15Z

In PowerShell, try this:

"`a"*1000

You will be rewarded with...

(ed8.1f90): C++ EH exception - code e06d7363 (first chance)
(ed8.1f90): C++ EH exception - code e06d7363 (first chance)
TerminalApp.dll!00007FFD2B20C922: ReturnHr(1) tid(1f90) 8007023E {Application Error}
The exception %s (0x    Msg:[std::exception: Bad optional access]

This also happens during normal belling, unfortunately. It doesn't take a thousand, that just made it quite reproable. 😄

src/cascadia/TerminalApp/TerminalTab.cpp

ghost · 2021-01-22T23:48:04Z

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

When we emit a BEL (visual or audible), show an indicator in the tab header If the tab the BEL is coming from is not focused when the BEL is raised, the indicator in its header will be removed when the tab gains focus. If the tab was already focused when the BEL was emitted, then the indicator goes away after 2 seconds. Closes microsoft#8106

first pass

f18c012

ghost 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 Dec 22, 2020

helper functions etc

8713465

PankajBhojwani added 3 commits January 7, 2021 15:19

pause

df6c157

tab focus state updated accordingly with control

98bb42b

add comments

a6fd006

PankajBhojwani changed the title ~~[DRAFT] Show indicator of bell in tab~~ Show indicator of bell in tab Jan 8, 2021

PankajBhojwani marked this pull request as ready for review January 8, 2021 19:24

format

73b4d2e

PankajBhojwani mentioned this pull request Jan 11, 2021

[Scenario] Progress Bar Follow-ups #6700

Open

4 tasks

carlos-zamora approved these changes Jan 13, 2021

View reviewed changes

DHowett reviewed Jan 13, 2021

View reviewed changes

src/cascadia/TerminalApp/Pane.cpp Outdated Show resolved Hide resolved

DHowett requested changes Jan 13, 2021

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jan 13, 2021

fix if statements

4738653

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jan 13, 2021

DHowett reviewed Jan 13, 2021

View reviewed changes

src/cascadia/TerminalApp/Pane.cpp Outdated Show resolved Hide resolved

PankajBhojwani added 3 commits January 12, 2021 18:21

fix

5736727

fix fix

ad60150

only show indicator if visual is set

26aefa2

zadjii-msft self-assigned this Jan 15, 2021

zadjii-msft previously requested changes Jan 15, 2021

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jan 15, 2021

zadjii-msft assigned PankajBhojwani and unassigned zadjii-msft Jan 15, 2021

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jan 15, 2021

Don-Vito mentioned this pull request Jan 16, 2021

bellStyle 'visual' #8807

Closed

DHowett requested changes Jan 21, 2021

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jan 21, 2021

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jan 21, 2021

fixes

68a618a

DHowett reviewed Jan 22, 2021

View reviewed changes

src/cascadia/TerminalApp/TerminalTab.cpp Outdated Show resolved Hide resolved

check has value

eac5164

DHowett approved these changes Jan 22, 2021

View reviewed changes

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jan 22, 2021

ghost merged commit 02fd7a0 into main Jan 22, 2021

ghost deleted the dev/pabhoj/bell_in_tab branch January 22, 2021 23:48

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show indicator of bell in tab #8637

Show indicator of bell in tab #8637

PankajBhojwani commented Dec 22, 2020 •

edited by DHowett

skyline75489 commented Dec 23, 2020

PankajBhojwani commented Dec 23, 2020

skyline75489 commented Dec 23, 2020 via email •

edited by ghost

PankajBhojwani commented Jan 7, 2021 •

edited

skyline75489 commented Jan 11, 2021

DHowett left a comment

zadjii-msft commented Jan 15, 2021

zadjii-msft left a comment

PankajBhojwani commented Jan 15, 2021

zadjii-msft commented Jan 15, 2021

DHowett commented Jan 19, 2021

zadjii-msft commented Jan 19, 2021

skyline75489 commented Jan 20, 2021

DHowett commented Jan 21, 2021

DHowett Jan 21, 2021

DHowett Jan 21, 2021

DHowett Jan 21, 2021

PankajBhojwani Jan 21, 2021

DHowett Jan 21, 2021

DHowett Jan 21, 2021

DHowett Jan 21, 2021

PankajBhojwani Jan 21, 2021

DHowett commented Jan 21, 2021

ghost commented Jan 22, 2021

	tab->_bellIndicatorTimer.value().Stop();
	tab->_bellIndicatorTimer->Stop();

Show indicator of bell in tab #8637

Show indicator of bell in tab #8637

Conversation

PankajBhojwani commented Dec 22, 2020 • edited by DHowett

skyline75489 commented Dec 23, 2020

PankajBhojwani commented Dec 23, 2020

skyline75489 commented Dec 23, 2020 via email • edited by ghost

PankajBhojwani commented Jan 7, 2021 • edited

skyline75489 commented Jan 11, 2021

DHowett left a comment

Choose a reason for hiding this comment

zadjii-msft commented Jan 15, 2021

zadjii-msft left a comment

Choose a reason for hiding this comment

PankajBhojwani commented Jan 15, 2021

zadjii-msft commented Jan 15, 2021

DHowett commented Jan 19, 2021

zadjii-msft commented Jan 19, 2021

skyline75489 commented Jan 20, 2021

DHowett commented Jan 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett commented Jan 21, 2021

ghost commented Jan 22, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

PankajBhojwani commented Dec 22, 2020 •

edited by DHowett

skyline75489 commented Dec 23, 2020 via email •

edited by ghost

PankajBhojwani commented Jan 7, 2021 •

edited

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.