Add shield to tab row when elevated by zadjii-msft · Pull Request #11224 · microsoft/terminal

zadjii-msft · 2021-09-14T16:06:30Z

Summary of the Pull Request

Adds a visible indicator that a Terminal window is elevated. This icon can be disabled with "showAdminShield" false in the global settings.

References

PR Checklist

Closes UI Styling to Clearly Indicate Elevated (admin) Window #1939
I work here
[n/a] Tests added/passed
Requires documentation to be updated - yea probably

Validation Steps Performed

…ion state

carlos-zamora

Love it! Idk how valid my concern is below, so I'll just approve.

carlos-zamora · 2021-09-14T21:30:51Z

src/cascadia/TerminalApp/TitlebarControl.xaml

        <ColumnDefinition Width="Auto" />
    </Grid.ColumnDefinitions>

+


nit: you can undo changes to this file

^ This. 😄

carlos-zamora · 2021-09-14T21:34:24Z

src/cascadia/TerminalApp/TerminalPage.cpp

+            // GH#2455 - Make sure to try/catch calls to Application::Current,
+            // because that _won't_ be an instance of TerminalApp::App in the
+            // LocalTests
+            try


How does this try-catch work with the magic static? If the try fails, is isElevated permanently stuck as false?

(Also, is this even a valid concern? Does Application::Current fail often?)

does Application::Current fail often

literally only in the tests, because Application::Current isn't a TerminalApp::App in the tests. With the magic static, we catch the exception as it's getting initialized, return false, and then put false in the static member to store forever

Right now AppLogic just sets a member on itself already when it is created, but also the implementation of IsUserAdmin doesn't actually rely on any part of AppLogic's state. Would it make sense to then make a free function/static function IsElevated somewhere else that could just be used directly without multiple layers of caching and indirection? As an optimization that IsElevated function could store a static member, should you choose to.

If that free function exists somewhere sufficiently up the stack, it could also be used for ApplicationState :)

FWIW there's one of my many branches hanging around that does this. I think it's going to get looped in to #11222 when I bump that next (hopefully today, likely monday tho)

carlos-zamora · 2021-09-14T21:38:07Z

Don't forget to update the docs/schema!

zadjii-msft · 2021-09-14T21:48:14Z

Don't forget to update the docs/schema!

frick I did, just didn't push lol

DHowett · 2021-09-16T00:37:39Z

I think this breaks max and min width in showTabsInTitlebar=false mode.

zadjii-msft · 2021-09-16T13:21:52Z

src/cascadia/TerminalApp/TabRowControl.xaml

-                 CanReorderTabs="True"
-                 IsAddTabButtonVisible="false"
-                 TabWidthMode="Equal">
+    <StackPanel Orientation="Horizontal">


protip: ignore whitespace for this file

zadjii-msft · 2021-09-16T13:22:33Z

I think this breaks max and min width in showTabsInTitlebar=false mode.

What the devil, this is broken even for unelevated, tabsInTitlebar windows:

zadjii-msft · 2021-09-16T14:43:16Z

WEIRD WEIRD WEIRD

If you do it with a StackPanel, the StackPanel will overflow without a care in the world.

If you do it with a Grid though, then it will obey the MaxWidth. Weird.

lhecker

LGTM. Let me know if you want to leave it as it is and I'll approve it.

src/cascadia/TerminalApp/TabRowControl.xaml

lhecker · 2021-09-20T16:34:33Z

src/cascadia/TerminalApp/TitlebarControl.xaml

        <ColumnDefinition Width="Auto" />
    </Grid.ColumnDefinitions>

+


^ This. 😄

src/cascadia/TerminalApp/TerminalPage.cpp

DHowett

In addition to the comments, the spacing feels a bit cramped to me. cc @cinnamon-msft for design oversight, but in the meantime... I wonder if it should have equal padding on the four sides compared to the tab itself.

I know -- the TabView has its own padding, which means that we need to pad the UAC shield asymmetrically to make up for it

DHowett · 2021-09-20T18:13:56Z

src/cascadia/TerminalApp/Resources/en-US/Resources.resw

    <value>Don't show again</value>
  </data>
+  <data name="ElevationShield.[using:Windows.UI.Xaml.Controls]ToolTipService.ToolTip" xml:space="preserve">
+    <value>This Terminal window is running as an Administrator</value>


Wording here feels off... but I cannot figure out why.,

"This Terminal window is running with elevated permissions"?

"This Terminal window is running in High-IL"

"This Terminal window is running as Admin"

"This Terminal window is running elevated"

"This Terminal window has elevated permissions"

I'm fond of this one.

Suggested change

<value>This Terminal window is running as an Administrator</value>

<value>This Terminal window is running as Admin</value>

src/cascadia/TerminalApp/TabRowControl.xaml

DHowett · 2021-09-20T18:15:24Z

src/cascadia/TerminalApp/TitlebarControl.xaml

        <ColumnDefinition Width="Auto" />
    </Grid.ColumnDefinitions>

+


zadjii-msft · 2021-09-20T19:39:13Z

Came up with a better solution:

Totally forgot about TabView.TabStripHeader, which is exactly what it sounds like, and does exactly what we want.

DHowett

LES DO DIS

DHowett · 2021-09-22T19:43:22Z

src/cascadia/TerminalApp/Resources/en-US/Resources.resw

    <value>Don't show again</value>
  </data>
+  <data name="ElevationShield.[using:Windows.UI.Xaml.Controls]ToolTipService.ToolTip" xml:space="preserve">
+    <value>This Terminal window is running as an Administrator</value>


I'm fond of this one.

Suggested change

<value>This Terminal window is running as an Administrator</value>

<value>This Terminal window is running as Admin</value>

DHowett · 2021-09-22T19:48:59Z

qq, should we make it less "bright"? right now it's the primary text color which is very bright on dark displays; we could tone it down a bit to maybe the SystemAltMediumColor?

zadjii-msft · 2021-09-22T19:59:29Z

qq, should we make it less "bright"? right now it's the primary text color which is very bright on dark displays; we could tone it down a bit to maybe the SystemAltMediumColor?

ez

miniksa · 2021-09-22T20:52:45Z

WEIRD WEIRD WEIRD

If you do it with a StackPanel, the StackPanel will overflow without a care in the world.

If you do it with a Grid though, then it will obey the MaxWidth. Weird.

#justxamlthings

miniksa · 2021-09-22T20:55:39Z

In this one, the shield looks a few pixels too high:

In this one, the shield looks a few pixels too low:

I feel like it should be centered on the same vertical center line as the content of the tab...

ghost · 2021-09-22T21:27:57Z

Hello @zadjii-msft!

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 · 2021-09-22T21:30:55Z

Do tend to agree about the spacing! fix it in post?

ghost · 2021-10-20T17:07:13Z

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

Handy links:

zadjii-msft added 5 commits September 2, 2021 15:00

add shield to titlebar

04dcd2b

move where the shield is hosted, make it actually dependent on elevat…

009ef14

…ion state

Add a setting too

2acaedf

Merge branch 'main' into dev/migrie/f/uac-shield

2899222

add a tooltip too

05679bc

ghost added Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal. labels Sep 14, 2021

schema yes yes I know

b436935

carlos-zamora approved these changes Sep 14, 2021

View reviewed changes

Merge remote-tracking branch 'origin/main' into dev/migrie/f/uac-shield

5481e00

zadjii-msft commented Sep 16, 2021

View reviewed changes

zadjii-msft added 2 commits September 16, 2021 09:43

friendship ended with stackpanel ; grid is my best friend now

26aad5e

huh, this clone doesn't auto-format

71e6d62

zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Sep 20, 2021

ghost requested review from DHowett, PankajBhojwani, leonMSFT, lhecker and miniksa September 20, 2021 15:25

lhecker reviewed Sep 20, 2021

View reviewed changes

dont need this

f29882c

zadjii-msft requested a review from lhecker September 20, 2021 17:26

DHowett requested changes Sep 20, 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 Sep 20, 2021

lhecker approved these changes Sep 20, 2021

View reviewed changes

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

zadjii-msft assigned cinnamon-msft Sep 20, 2021

zadjii-msft added 2 commits September 20, 2021 14:42

way better

134a4c3

dont need this

06a081e

DHowett approved these changes Sep 22, 2021

View reviewed changes

Merge remote-tracking branch 'origin/main' into dev/migrie/f/uac-shield

425ccaa

use SystemControlForegroundBaseMediumBrush because it's more pleasant

2bc4651

zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Sep 22, 2021

DHowett removed the Needs-Second It's a PR that needs another sign-off label Sep 22, 2021

ghost merged commit 171e0a0 into main Sep 23, 2021

ghost deleted the dev/migrie/f/uac-shield branch September 23, 2021 17:44

zadjii-msft mentioned this pull request Oct 13, 2021

Add a setting to trim out Administrator: if it's the start of the title #11502

Closed

ghost mentioned this pull request Oct 20, 2021

UI Styling to Clearly Indicate Elevated (admin) Window #1939

Closed

This pull request was closed.

	<value>This Terminal window is running as an Administrator</value>
	<value>This Terminal window is running as Admin</value>

Conversation

zadjii-msft commented Sep 14, 2021

Summary of the Pull Request

References

PR Checklist

Validation Steps Performed

Uh oh!

carlos-zamora left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rosefield Sep 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlos-zamora commented Sep 14, 2021

Uh oh!

zadjii-msft commented Sep 14, 2021

Uh oh!

DHowett commented Sep 16, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zadjii-msft commented Sep 16, 2021

Uh oh!

zadjii-msft commented Sep 16, 2021

Uh oh!

lhecker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zadjii-msft commented Sep 20, 2021

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DHowett commented Sep 22, 2021

Uh oh!

zadjii-msft commented Sep 22, 2021

Uh oh!

miniksa commented Sep 22, 2021

Uh oh!

miniksa commented Sep 22, 2021

Uh oh!

ghost commented Sep 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.

Uh oh!

DHowett commented Sep 22, 2021

Uh oh!

ghost commented Oct 20, 2021

Uh oh!

Reviewers

Rosefield Sep 15, 2021 •

edited

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.