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

Add a setting to flash the pane when BEL is emitted #9270

Merged
32 commits merged into from
May 24, 2021
Merged

Conversation

PankajBhojwani
Copy link
Contributor

@PankajBhojwani PankajBhojwani commented Feb 24, 2021

Summary of the Pull Request

Adds a new bellStyle called window. When window is set and a BEL is emitted, we flash the pane that emitted it.

Additionally, changes bellStyle in the SUI to a list of checkboxes instead of radio buttons, to match bellStyle being a flag-enum. Deprecates 'BellStyle::Visual' in the schema, but still allows it to be set in the json (it maps to Window | Taskbar)

References

#6700

PR Checklist

  • Closes #xxx
  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed
  • Documentation updated. If checked, please file a pull request on our docs repo and link it here: #xxx
  • Schema updated.
  • I work here

Validation Steps Performed

GIF in Teams

@zadjii-msft

This comment has been minimized.

@PankajBhojwani

This comment has been minimized.

@zadjii-msft zadjii-msft self-assigned this Mar 3, 2021
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.

I have no idea why this doesn't work. This should work - Terminal::SetScreenMode already does call TriggerRedrawAll().

Maybe InvertScreenColors/_InvertTimerTick need to take the write lock?

src/cascadia/TerminalControl/TermControl.cpp Outdated Show resolved Hide resolved
@zadjii-msft zadjii-msft added Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Area-UserInterface 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 Mar 17, 2021
@PankajBhojwani
Copy link
Contributor Author

PankajBhojwani commented Mar 19, 2021

Maybe InvertScreenColors/_InvertTimerTick need to take the write lock?

That seems to have fixed it, tested by repeatedly sending a bunch of BELs and the screen reverted back properly

@PankajBhojwani PankajBhojwani marked this pull request as ready for review March 19, 2021 23:01
zadjii-msft
zadjii-msft previously approved these changes Mar 23, 2021
@zadjii-msft zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Mar 23, 2021
@ghost ghost requested review from miniksa, carlos-zamora, DHowett and leonMSFT March 23, 2021 13:40
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

You have activated my trap card!

Since you updated the mapping for a setting, you are now forced to update the Settings UI. Otherwise, we'll crash when attempting to find the resw entry for these new values.

You also need to update the schema and the docs.

I see the gif in your PR description, but I don't understand what the difference between window and visual is. Also, taskbar is added, but not mentioned in the PR description above.

WI_IsFlagSet(paneProfile.BellStyle(), winrt::Microsoft::Terminal::Settings::Model::BellStyle::Taskbar);

// raise the event with the bool value corresponding to the taskbar or visual flag
_PaneRaiseBellHandlers(nullptr, flashTaskbar);
Copy link
Member

Choose a reason for hiding this comment

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

So, the settings now look like this?

  • window: flash terminal
  • taskbar: add bell icon to tab
  • visual = [ "window", "taskbar" ]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

taskbar flashes the taskbar icon at the toolbar (if the terminal window is not focused)! The others are correct

src/cascadia/TerminalControl/TermControl.cpp Outdated Show resolved Hide resolved
@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 23, 2021
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

I need to have a think about this :)

ghost pushed a commit that referenced this pull request May 20, 2021
## Summary of the Pull Request

This replaces `ThrottledFunc` with two variants:
* `ThrottledFuncLeading` invokes the callback immediately and blocks further calls for the given duration
* `ThrottledFuncTrailing` blocks calls for the given duration and then invokes the callback

## References

* #9270 - `ThrottledFuncLeading` will allow the pane to flash immediately for a BEL, but block further BELs until the animation finished

## PR Checklist
* [x] I work here
* [ ] Tests added/passed

## Validation Steps Performed

* [x] Ensured scrolling still works
_PaneRaiseBellHandlers(nullptr, WI_IsFlagSet(paneProfile.BellStyle(), winrt::Microsoft::Terminal::Settings::Model::BellStyle::Visual));
if (WI_IsFlagSet(paneProfile.BellStyle(), winrt::Microsoft::Terminal::Settings::Model::BellStyle::Window))
{
_control.BellLightOn();
Copy link
Member

Choose a reason for hiding this comment

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

this seems like the wrong abstraction-- the app should not tell the control to blink after the control tells the app that there was a bell.

Copy link
Member

Choose a reason for hiding this comment

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

Your comment about the visual bell moving up into the application resonates with me. Thank you.

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

abstraction concern. if you'd rather keep it this way, let me know!

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label May 21, 2021
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label May 24, 2021
@PankajBhojwani
Copy link
Contributor Author

PankajBhojwani commented May 24, 2021

abstraction concern. if you'd rather keep it this way, let me know!

I think I like the idea of letting the app handle the flash. Though I think it'll be much smoother to do that after the pane refactor goes in - that way we can attach the xaml light to the pane instead of the control's grid. Thoughts?

If we want the control to handle the flash, 1. it'll have to know if BellStyle::Window is set and 2. we'll be in the situation where the app handles BellStyle::Taskbar and BellStyle::Audible but the control handles BellStyle::Window, which I guess is fine but I think its nicer to have it all be in one place.

{
// Stop the timer and switch off the light
_bellLightTimer->Stop();
_bellLightTimer.reset();
Copy link
Member

Choose a reason for hiding this comment

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

Theoretically, once the timer's been initialized, we don't need to keep creating and destroying it each time. We could just re-use it, but I'm not gonna block over that.

Copy link
Member

Choose a reason for hiding this comment

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

Good notes for cleanup later. 😄

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.

Okay, I have nits & questions, but nothing to block over


// Switch on the light and animate the intensity to fade out
VisualBellLight::SetIsTarget(RootGrid(), true);
BellLight().CompositionLight().StartAnimation(L"Intensity", _bellLightAnimation);
Copy link
Member

Choose a reason for hiding this comment

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

Just to be clear, the animation runs from Intensity=2.0->1.0, then is switched off entirely at the end of the time. That's neat

@@ -164,8 +167,11 @@ namespace winrt::Microsoft::Terminal::Control::implementation
Windows::UI::Xaml::DispatcherTimer _autoScrollTimer;
std::optional<std::chrono::high_resolution_clock::time_point> _lastAutoScrollUpdateTime;

winrt::Windows::UI::Composition::ScalarKeyFrameAnimation _bellLightAnimation;
Copy link
Member

Choose a reason for hiding this comment

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

meh that seems fine to me. In the future, we could always init this as

winrt::Windows::UI::Composition::ScalarKeyFrameAnimation _bellLightAnimation{ nullptr };

and only ctor it when actually requested in the settings, but again, meh

@@ -627,6 +626,32 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
_State.Profile().ColorSchemeName(val.Name());
}

bool Profiles::IsBellStyleFlagSet(const uint32_t flag)
{
return (WI_EnumValue(_State.Profile().BellStyle()) & flag) == flag;
Copy link
Member

Choose a reason for hiding this comment

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

Wait uh, what exactly is happening here? Is this just WI_IsFlagSet?

Copy link
Member

Choose a reason for hiding this comment

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

or WI_AreAllFlagsSet?

Copy link
Member

Choose a reason for hiding this comment

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

explained offline -- this is because the WI macros require a compile-time constant. We have to do it manually.

@@ -630,7 +630,8 @@ namespace Microsoft::Terminal::Settings::Model::JsonUtils
for (const auto& pair : TBase::mappings)
{
if (pair.second != AllClear &&
(val & pair.second) == pair.second)
(val & pair.second) == pair.second &&
Copy link
Member

Choose a reason for hiding this comment

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

This again looks like a WI_IsFlagSet, which I think also does WI_IsSingleFlagSet


void VisualBellLight::OnIsTargetChanged(DependencyObject const& d, DependencyPropertyChangedEventArgs const& e)
{
const auto uielem{ d.try_as<UIElement>() };
Copy link
Member

Choose a reason for hiding this comment

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

kinda surprised that uielem resolved as a word with spellcheck

return VisualBellLight::GetIdStatic();
}

void VisualBellLight::OnIsTargetChanged(DependencyObject const& d, DependencyPropertyChangedEventArgs const& e)
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 think I totally understand what's going on with this method. This is a static function that's called when IsTarget changes on a VisualBellLight? And then d is either a UIElement, or a Brush... When is this ever a brush?

Or is this just forward thinking? All this seems like handy boilerplate around XamlLight, I'm kinda shocked (but also not surprised) that we need this much boilerplate just to add a light.

Copy link
Member

Choose a reason for hiding this comment

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

So yeah I think this is some cool stuff -- he's created an "attached property" (like Grid.Row -- a property that's about one object, stored in global storage, attached to another object). It looks like this one can be attached to either a brush or a UI Element, seamlessly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep! I've mostly just adopted the code from the xaml light docs. Lights can only target a Brush or a UIElement, and yeah in our case we never use it for a brush but I feel like its probably a good idea to have that check anyway

@DHowett
Copy link
Member

DHowett commented May 24, 2021

@carlos-zamora can you dismiss or re-review?

@carlos-zamora carlos-zamora dismissed their stale review May 24, 2021 17:13

major changes

_PaneRaiseBellHandlers(nullptr, WI_IsFlagSet(paneProfile.BellStyle(), winrt::Microsoft::Terminal::Settings::Model::BellStyle::Visual));
if (WI_IsFlagSet(paneProfile.BellStyle(), winrt::Microsoft::Terminal::Settings::Model::BellStyle::Window))
{
_control.BellLightOn();
Copy link
Member

Choose a reason for hiding this comment

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

Your comment about the visual bell moving up into the application resonates with me. Thank you.

{
// Stop the timer and switch off the light
_bellLightTimer->Stop();
_bellLightTimer.reset();
Copy link
Member

Choose a reason for hiding this comment

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

Good notes for cleanup later. 😄

return VisualBellLight::GetIdStatic();
}

void VisualBellLight::OnIsTargetChanged(DependencyObject const& d, DependencyPropertyChangedEventArgs const& e)
Copy link
Member

Choose a reason for hiding this comment

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

So yeah I think this is some cool stuff -- he's created an "attached property" (like Grid.Row -- a property that's about one object, stored in global storage, attached to another object). It looks like this one can be attached to either a brush or a UI Element, seamlessly.

@@ -627,6 +626,32 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
_State.Profile().ColorSchemeName(val.Name());
}

bool Profiles::IsBellStyleFlagSet(const uint32_t flag)
{
return (WI_EnumValue(_State.Profile().BellStyle()) & flag) == flag;
Copy link
Member

Choose a reason for hiding this comment

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

explained offline -- this is because the WI macros require a compile-time constant. We have to do it manually.

DHowett pushed a commit that referenced this pull request May 24, 2021
This replaces `ThrottledFunc` with two variants:
* `ThrottledFuncLeading` invokes the callback immediately and blocks further calls for the given duration
* `ThrottledFuncTrailing` blocks calls for the given duration and then invokes the callback

* #9270 - `ThrottledFuncLeading` will allow the pane to flash immediately for a BEL, but block further BELs until the animation finished

* [x] I work here
* [ ] Tests added/passed

* [x] Ensured scrolling still works

(cherry picked from commit 13f0b8e)
@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label May 24, 2021
@ghost
Copy link

ghost commented May 24, 2021

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.

@ghost ghost merged commit 227ec37 into main May 24, 2021
@ghost ghost deleted the dev/pabhoj/bell_flash branch May 24, 2021 22:51
@ghost ghost removed the Needs-Second It's a PR that needs another sign-off label May 25, 2021
@ghost
Copy link

ghost commented May 25, 2021

🎉Windows Terminal Preview v1.9.1445.0 has been released which incorporates this pull request.:tada:

Handy links:

ghost pushed a commit that referenced this pull request Jun 2, 2021
Just come cleanup I did not manage to get to before #9270 merged. 

Specifically:

- We only initialize the animation and timer if we need them
- We don't repeatedly destroy/create the timer

## Validation Steps Performed
It still works
DHowett pushed a commit that referenced this pull request Jul 7, 2021
Just come cleanup I did not manage to get to before #9270 merged.

Specifically:

- We only initialize the animation and timer if we need them
- We don't repeatedly destroy/create the timer

## Validation Steps Performed
It still works

(cherry picked from commit 2fed4c4)
ghost pushed a commit that referenced this pull request Aug 11, 2022
Adds a variable `_isBackgroundLight` that is updated when the background
color is changed. When it is `true`, the BEL indicator flash will darken
the screen instead of brightening.

`_isColorLight(bg)` returns `true` if the average of `r`, `g`, and `b`
is >127

I was unsure of an appropriate way to change the color of the
`CompositionLight` based on the background, so I changed it to always be
gray and adjusted the intensity values of the original animation to have
roughly the same visual effect as the white.

## Validation Steps Performed
* Tested the two flashes on the default color schemes and some custom
  background colors to see if they look consistent
* Used tracepoints and visual to check that the right animation is used
  (including multiple tabs, split windows with different themes, and
  changing settings while window is open)

References #9270
Closes #13450
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Area-UserInterface Issues pertaining to the user interface of the Console or Terminal AutoMerge Marked for automatic merge by the bot when requirements are met 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.

5 participants