-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
Option to default to show icon in tab, hide tabicon or make icon in tab monochrome. #15948
Conversation
Adding enum iconstyle for hiding the icon in the tab
@microsoft-github-policy-service agree |
I will be looking into making monochrome work, but I need to understand if I need to implement grayscale features or we have a way of getting monochrome icons from iconPath. |
@@ -41,7 +41,7 @@ namespace winrt::TerminalApp::implementation | |||
std::shared_ptr<Pane> newPane); | |||
|
|||
void ToggleSplitOrientation(); | |||
void UpdateIcon(const winrt::hstring iconPath); | |||
void UpdateIcon(const winrt::hstring iconPath, const winrt::Microsoft::Terminal::Settings::Model::IconStyle& iconStyle); |
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.
IconStyle is an enum type; is it useful to pass that by const &
?
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.
By passing it const& you indicate that it is a read-only type and it should not copy the value.
You can resolve the comment if you have no further questions.
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.
What's wrong with copying the value though? It's 32-bit so nowadays smaller than a pointer. There are other methods that take bool
parameters by value.
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.
I agree with @KalleOlaviNiemitalo. const&
is most useful when passing RAII types to avoid making costly copies. An integer is much much cheaper to pass by-value.
(It can also be useful under certain circumstances when passing >8 bytes on the Windows x64 ABI due to poor MSVC optimizations, but I'm sure those few cycles are fairly irrelevant in most circumstances.)
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.
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.
I don't think it's fair to use other instances of this as proof that it isn't a mistake. Otherwise what does this code imply?
terminal/src/types/ModifierKeyState.cpp
Lines 100 to 137 in 9b70a40
// Routine Description: | |
// - Expands legacy control keys bitsets into a stl set | |
// Arguments: | |
// - flags - legacy bitset to expand | |
// Return Value: | |
// - set of ModifierKeyState values that represent flags | |
std::unordered_set<ModifierKeyState> FromConsoleControlKeyFlags(const DWORD flags) | |
{ | |
std::unordered_set<ModifierKeyState> keyStates; | |
for (const auto& mapping : ModifierKeyStateTranslationTable) | |
{ | |
if (RuntimeIsFlagSet(flags, mapping.second)) | |
{ | |
keyStates.insert(mapping.first); | |
} | |
} | |
return keyStates; | |
} | |
// Routine Description: | |
// - Converts ModifierKeyState back to the bizarre console bitflag associated with it. | |
// Arguments: | |
// - modifierKey - modifier to convert | |
// Return Value: | |
// - console bitflag associated with modifierKey | |
DWORD ToConsoleControlKeyFlag(const ModifierKeyState modifierKey) noexcept | |
{ | |
for (const auto& mapping : ModifierKeyStateTranslationTable) | |
{ | |
if (mapping.first == modifierKey) | |
{ | |
return mapping.second; | |
} | |
} | |
return 0; | |
} |
It turns a regular bitset into a std::unordered_set<>
just to then test whether a given bit is set. It creates an entire hashmap instance just to do the equivalent of a & b != 0
. It would be pretty terrific if this implied that binary operators are bad!
Passing primitive types by reference is a minuscule issue, so it's not really something I'm bothered by and I'll approve this PR regardless. But I believe if it came to a debate about this, that I could win it with fairly solid arguments.
Especially since statements like "it should not copy the value" are factually wrong. Passing parameters by value doesn't necessarily create a copy. Primitive types aren't classes after all.
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.
I will not argue that you would have more solid arguments, but entering a code base, I found it better to do as already written, as to the arguments it is wrong or not, existing examples were already written with const&
, so I found it better to copy the style.
My arguments are to the understanding of my knowledge and thank you for providing me with more information. My question would then be more along the lines of whether should we fix the existing code, or should we make it more clear that we do not need it, as now we are changing style from what is already written. I enjoy code that follows the same style.
Thanks for the contribution! We're all gonna be a little extra busy this week, so it might be a bit before we can give this a full review. Thanks for your patience!
FYI: this is probably easily doable with |
@zadjii-msft, I understand. I tried the |
Also added support for monochrome configuration.
No, no it is not 😅 There's maybe a few icons where that's actually useful, but hey, I'm here to give users options. If they want the silly XAML-powered "convert all the pixels to the text foreground color" behavior, who am I to say no 😝? |
Personally speaking, I'm not sure whether we should add this if no user has asked for it yet though... Seems quite niche to me if I'm honest. 😶 |
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.
LGTM. But you need to format your PR. You can do this in PowerShell by navigating to the root of the repository and running:
Import-Module .\tools\OpenConsole.psm1
Invoke-CodeFormat
<dustin hat> Before I merge it, can you change the title to explain the change in brief? Rather than "Fix bug 1234", it is easier to read if it is like (for example) "Evaluate the numbers before displaying them". It makes it so you can get a good idea of the code change without having to look at another bug ID |
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.
yep, this is exactly how I'd do it. Thanks!
#include <d2d1.h> | ||
#include <d2d1effects_2.h> | ||
|
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.
nit: do we need these headers?
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.
You are right, I missed removing them, it was from my idea to actually implement grayscale, but I will remove and sadly you guys have to re-approve.
One day into 1.19, and there's a LOT of hits here (**76.25%** of our ~300 crashes). A crash if the Theme doesn't have a `tab` member. Regressed in #15948 Closes MSFT:46714723
One day into 1.19, and there's a LOT of hits here (**76.25%** of our ~300 crashes). A crash if the Theme doesn't have a `tab` member. Regressed in #15948 Closes MSFT:46714723 (cherry picked from commit cf19385) Service-Card-Id: 90670731 Service-Version: 1.19
Adding enum iconstyle for hiding the icon in the tab #8157
Summary of the Pull Request
Please confirm if I am on the right track.
References and Relevant Issues
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed
PR Checklist