Replace the KB Dialog with a InfoBar #8524

zadjii-msft · 2020-12-08T18:49:09Z

This changes the keyboard warning from a dialog to an InfoBar, which
we just got in MUX 2.5. Some users were unhappy that we'd always display
the dialog. We learned from the input team that this service should
always be enabled. We're also learing from users that they don't always
want it enabled.

We're working with the Input team to help us figure out how this service
can be disabled and the Terminal work just fine. They're confident
that it shouldn't. For 99% of our users, they're right. So we don't
want to get rid of the dialog entirely, we want to understand how this
is possible. While we wait, let's make the message less aggressive.

This is instead of making a iKnowWhatImDoingDisableTheKeyboardWarning
setting to disable the dialog. Props to @cornem for suggesting the less
aggressive solution.

Validation Steps Performed

Tested manually, but by forcing the message to always display. Disabling
the service requires two full reboots, and ain't nobody got time for
that.

Closes #8228
Closes #4448, for now

src/cascadia/TerminalApp/TerminalPage.xaml

mdtauk · 2020-12-08T20:08:11Z

Couple of thoughts. Does the InfoBar follow the theme chosen for the profile/app - or does it pick Light or Dark theme based on the console's background colour's brightness so as to fit better?

Does it use the default severity colours built into the control's template - or do you map the colours to the profile's equivalent colour?

Green for Success
Yellow for Warning
Red for Critical
some kind of inverted colours for the default/informational colours?

zadjii-msft · 2020-12-08T22:00:12Z

Does it use the default severity colours built into the control's template

It's a pretty standard InfoBar, I'm really not doing anything to customize it. The coloration is based on whatever the application's theme is set to, and whatever the default styling for an InfoBar is.

Literally the only styling I've written for it is

<mux:InfoBar x:Name="KeyboardWarningInfoBar"
                     IsClosable="True"
                     IsIconVisible="True"
                     IsOpen="False"
                     Severity="Warning"
                     Message="{x:Bind KeyboardServiceDisabledText, Mode=OneWay}"/>

DHowett · 2020-12-08T22:21:45Z

src/cascadia/TerminalApp/TerminalPage.xaml

@@ -98,6 +98,7 @@ the MIT License. See LICENSE in the project root for license information. -->
            VerticalAlignment="Stretch" />

        <mux:InfoBar x:Name="KeyboardWarningInfoBar"
+                     x:Load="False"


You do unfortunately need to use the FindName trick with Load; as written it will crash i think

(this is because the member isn't populated until the control is loaded, but FindName loads it (instead of finding it. official API!)

This is really good to know. Just hit this in my own implementation haha.

mdtauk · 2020-12-10T02:17:37Z

Does it use the default severity colours built into the control's template

It's a pretty standard InfoBar, I'm really not doing anything to customize it. The coloration is based on whatever the application's theme is set to, and whatever the default styling for an InfoBar is.

Literally the only styling I've written for it is
<mux:InfoBar x:Name="KeyboardWarningInfoBar"
                     IsClosable="True"
                     IsIconVisible="True"
                     IsOpen="False"
                     Severity="Warning"
                     Message="{x:Bind KeyboardServiceDisabledText, Mode=OneWay}"/>

The control can handle itself fine in terms of matching the App's RequestedTheme / The Theme set in Settings. I suppose what I was wondering is if it should match the UI or if it should match the Profile, in terms of colour.

It seems to be in that between zone in that it could be part of the Tabs and Window Frame UI Zone, or it could be part of the Console area.

If the InfoBar persists when switching tabs, then it makes sense to match the App.

DHowett · 2020-12-10T02:28:11Z

Yeah, so this one is app-affine and not terminal pane affine.

In general, there isn’t a well-defined “warning” color in a terminal application. We get to choose red, or yellow, or green or whatever but the user can reassign those color indices to be whatever color they want. There’s no contract that index 1 must be red, nor that an application use index 1 for warnings or errors.

It’s a neat idea, but I wouldn’t want to introduce too many dependencies on ill-defined things 😄

mdtauk · 2020-12-10T02:30:34Z

Yeah, so this one is app-affine and not terminal pane affine.

In general, there isn’t a well-defined “warning” color in a terminal application. We get to choose red, or yellow, or green or whatever but the user can reassign those color indices to be whatever color they want. There’s no contract that index 1 must be red, nor that an application use index 1 for warnings or errors.

It’s a neat idea, but I wouldn’t want to introduce too many dependencies on ill-defined things 😄

Cool - I was just adding some thoughts to the discussion (or was that opening a discussion) - either way it makes sense for it to be app level!

DHowett · 2020-12-10T07:15:08Z

(That said, I do like the idea. There has been some discussion in the .NET world and presumably the general CLI community about having something closer to semantic markup or more representative of application intent in terminal emulation. Not likely to to anywhere, but interesting to consider what a world is like where we give text meaning instead of just style!)

mdtauk · 2020-12-10T11:40:48Z

(That said, I do like the idea. There has been some discussion in the .NET world and presumably the general CLI community about having something closer to semantic markup or more representative of application intent in terminal emulation. Not likely to to anywhere, but interesting to consider what a world is like where we give text meaning instead of just style!)

Quite a departure from a "dumb" terminal churning through command codes...

DHowett · 2020-12-10T20:14:38Z

Quite a departure

Eh, so is OSC 8 hyperlink support. 😄

There's a fine line, for sure.

ghost · 2020-12-11T18:27:13Z

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.

DHowett · 2020-12-11T18:27:50Z

moved from the body:

mdcclxv · 2021-01-02T22:42:33Z

If that InfoBar has to be dismissed manually then it's not any better than a popup. Is still annoying to have it in my face every time the terminal starts. This might be acceptable for light users, but not for those using the terminal heavily.

@cornem

This changes the keyboard warning from a dialog to an `InfoBar`, which we just got in MUX 2.5. Some users were unhappy that we'd always display the dialog. We learned from the input team that this service _should_ always be enabled. We're also learing from users that they don't always want it enabled. We're working with the Input team to help us figure out how this service can be disabled _and the Terminal work just fine_. They're confident that it _shouldn't_. For 99% of our users, they're right. So we don't want to get rid of the dialog entirely, we want to understand how this is possible. While we wait, let's make the message less aggressive. This is instead of making a `iKnowWhatImDoingDisableTheKeyboardWarning` setting to disable the dialog. Props to @cornem for suggesting the less aggressive solution. ## Validation Steps Performed Tested manually, but by forcing the message to always display. Disabling the service requires two full reboots, and _ain't nobody got time for that_. Closes #8228 Closes #4448, for now (cherry picked from commit ff5b2b8)

ghost · 2021-01-28T18:02:04Z

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

Handy links:

ghost · 2021-01-28T18:07:52Z

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

Handy links:

@cornem

This changes the keyboard warning from a dialog to an `InfoBar`, which we just got in MUX 2.5. Some users were unhappy that we'd always display the dialog. We learned from the input team that this service _should_ always be enabled. We're also learing from users that they don't always want it enabled. We're working with the Input team to help us figure out how this service can be disabled _and the Terminal work just fine_. They're confident that it _shouldn't_. For 99% of our users, they're right. So we don't want to get rid of the dialog entirely, we want to understand how this is possible. While we wait, let's make the message less aggressive. This is instead of making a `iKnowWhatImDoingDisableTheKeyboardWarning` setting to disable the dialog. Props to @cornem for suggesting the less aggressive solution. ## Validation Steps Performed Tested manually, but by forcing the message to always display. Disabling the service requires two full reboots, and _ain't nobody got time for that_. Closes microsoft#8228 Closes microsoft#4448, for now

zadjii-msft added 2 commits December 8, 2020 12:27

replace the dialog with a InfoBar

721f674

Cleanup of the old dialog

6511e45

ghost added Area-Input Related to input processing (key presses, mouse, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Terminal The new Windows Terminal. labels Dec 8, 2020

DHowett reviewed Dec 8, 2020

View reviewed changes

src/cascadia/TerminalApp/TerminalPage.xaml Outdated Show resolved Hide resolved

This is handy

7c39ea2

DHowett requested changes Dec 8, 2020

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 Dec 8, 2020

zadjii-msft self-assigned this Dec 10, 2020

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Dec 10, 2020

What? I always compile and test my code before committing...

f2c14a0

DHowett approved these changes Dec 11, 2020

View reviewed changes

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Dec 11, 2020

zadjii-msft removed their assignment Dec 14, 2020

zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Dec 14, 2020

ghost requested review from miniksa, carlos-zamora and leonMSFT December 14, 2020 02:38

ghost requested a review from PankajBhojwani December 14, 2020 02:38

carlos-zamora approved these changes Dec 14, 2020

View reviewed changes

ghost merged commit ff5b2b8 into main Dec 14, 2020

ghost deleted the dev/migrie/f/8228-iKnowWhatImDoingDisableTheKeyboardWarning branch December 14, 2020 17:37

skyline75489 mentioned this pull request Jan 2, 2021

I get a warning that makes no sense on a desktop PC #8692

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace the KB Dialog with a InfoBar #8524

Replace the KB Dialog with a InfoBar #8524

zadjii-msft commented Dec 8, 2020 •

edited by DHowett

Loading

mdtauk commented Dec 8, 2020

zadjii-msft commented Dec 8, 2020

DHowett Dec 8, 2020

DHowett Dec 10, 2020

carlos-zamora Dec 14, 2020

mdtauk commented Dec 10, 2020

DHowett commented Dec 10, 2020

mdtauk commented Dec 10, 2020

DHowett commented Dec 10, 2020

mdtauk commented Dec 10, 2020

DHowett commented Dec 10, 2020

ghost commented Dec 11, 2020

DHowett commented Dec 11, 2020

mdcclxv commented Jan 2, 2021 •

edited

Loading

ghost commented Jan 28, 2021

ghost commented Jan 28, 2021

Replace the KB Dialog with a InfoBar #8524

Replace the KB Dialog with a InfoBar #8524

Conversation

zadjii-msft commented Dec 8, 2020 • edited by DHowett Loading

Validation Steps Performed

mdtauk commented Dec 8, 2020

zadjii-msft commented Dec 8, 2020

DHowett Dec 8, 2020

Choose a reason for hiding this comment

DHowett Dec 10, 2020

Choose a reason for hiding this comment

carlos-zamora Dec 14, 2020

Choose a reason for hiding this comment

mdtauk commented Dec 10, 2020

DHowett commented Dec 10, 2020

mdtauk commented Dec 10, 2020

DHowett commented Dec 10, 2020

mdtauk commented Dec 10, 2020

DHowett commented Dec 10, 2020

ghost commented Dec 11, 2020

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.

DHowett commented Dec 11, 2020

mdcclxv commented Jan 2, 2021 • edited Loading

ghost commented Jan 28, 2021

ghost commented Jan 28, 2021

zadjii-msft commented Dec 8, 2020 •

edited by DHowett

Loading

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.

mdcclxv commented Jan 2, 2021 •

edited

Loading