Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 shield to tab row when elevated #11224
Add shield to tab row when elevated #11224
Changes from all commits
04dcd2b
009ef14
2acaedf
2899222
05679bc
b436935
5481e00
26aad5e
71e6d62
f29882c
134a4c3
06a081e
425ccaa
2bc4651
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this try-catch work with the magic static? If the try fails, is
isElevated
permanently stuck asfalse
?(Also, is this even a valid concern? Does Application::Current fail often?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 functionIsElevated
somewhere else that could just be used directly without multiple layers of caching and indirection? As an optimization thatIsElevated
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 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)