Use vs-pwsh icons if applicable.#19990
Conversation
| wchar_t pwshPath[MAX_PATH] = { 0 }; | ||
| const auto pwshExeName = L"pwsh.exe"; | ||
| if (SearchPathW(nullptr, pwshExeName, nullptr, MAX_PATH, pwshPath, nullptr)) | ||
| isPwsh = SearchPathW(nullptr, pwshExeName, nullptr, MAX_PATH, pwshPath, nullptr); | ||
| if (isPwsh) |
There was a problem hiding this comment.
Now that this value is used outside of this function, writing this into an out-parameter is awkward. If you think about "the flow of data" for isPwsh it now looks like this:
flowchart TD
GenerateProfiles --> GetProfileCommandLine
GetProfileCommandLine --> isPwsh((isPwsh))
isPwsh --> GenerateProfiles ---> GetProfileIconPath
This looks kind of intuitively wrong, doesn't it? It would be better to write a private static bool _isPwshAvailable() noexcept member function. You can then structure it like this:
flowchart TD
GenerateProfiles --> _isPwshAvailable --> isPwsh((isPwsh))
isPwsh --> GetProfileCommandLine
isPwsh --> GetProfileIconPath
You then end up with:
const auto isPwsh = _isPwshAvailable();
profile->Commandline(winrt::hstring{ GetProfileCommandLine(instance, isPwsh) });
profile->Icon(winrt::hstring{ GetProfileIconPath(isPwsh) });There was a problem hiding this comment.
Isn't it different from the old code now? Unless I'm reading it wrong, the old code would just store "pwsh.exe" in the profile. The path itself was essentially thrown away. I may be misunderstanding the old code though.
There was a problem hiding this comment.
Oops, I misread the code. Updated to your suggestion.
0c3aeb2 to
8106e55
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Thanks for the contribution! Pushed a minor change (just ran the code formatter) so the new CI run shouldn't give any issues. Enabled auto-merge and cleaned up the PR body. So we just need another ✅ from @lhecker and this should be in. |
## Summary of the Pull Request This PR updates `VsDevShellGenerator` to use the `vs-pwsh` icon in generated profiles, if modern PowerShell has been detected. ## References and Relevant Issues The icons were added in #17706, but are not used anywhere. ## Detailed Description of the Pull Request / Additional comments * Updated `VsDevShellGenerator::GetProfileCommandLine` to accept a `bool& isPwsh` parameter, which is set to whether the generated profile command line is using modern PowerShell. This value gets passed to `VsDevShellGenerator::GetProfileIconPath`'s new parameter, which determines whether to return the icon for `powershell` or `pwsh`. (cherry picked from commit e040015) Service-Card-Id: PVTI_lADOAF3p4s4BQX0-zgpBNIk Service-Version: 1.25
Summary of the Pull Request
This PR updates
VsDevShellGeneratorto use thevs-pwshicon in generated profiles, if modern PowerShell has been detected.References and Relevant Issues
The icons were added in #17706, but are not used anywhere.
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed
PR Checklist