Introduce IconConverter #7830
Merged
Introduce IconConverter #7830
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
added
Area-CodeHealth
Topics for Discussion
|
|
This is likely the difference between an Icon and an IconSource. I have no idea why there's two. |
|
technically IconPathConverter should live in TSE, but we can't have TerminalApp depending on it from there :| |
DHowett
reviewed
Oct 5, 2020
| @@ -168,7 +169,7 @@ HRESULT Jumplist::UpdateJumplist(const CascadiaSettings& settings) noexcept | |||
|
|
|||
| // Create the shell link object for the profile | |||
| winrt::com_ptr<IShellLinkW> shLink; | |||
| const auto normalizedIconPath{ _normalizeIconPath(profile.ExpandedIconPath()) }; | |||
| const auto normalizedIconPath{ _normalizeIconPath(profile.IconPath()) }; | |||
There was a problem hiding this comment.
this will be hard to get emoji working w./
|
Oh, I see. No. Emoji will never work in the jumplist. Sorry. |
Base automatically changed from
dev/cazamor/set/terminal-settings-model
to
master
October 6, 2020 16:57
34 tasks
This comment has been minimized.
This comment has been minimized.
carlos-zamora
force-pushed
the
dev/cazamor/set/tsm/cmd-icon
branch
from
022c018
to
09b184e
Compare
DHowett
reviewed
Oct 6, 2020
DHowett
reviewed
Oct 6, 2020
DHowett
requested changes
Oct 6, 2020
ghost
added
Needs-Author-Feedback
zadjii-msft
approved these changes
Oct 7, 2020
|
Verified schema now shows icon as an allowable property in command (bonus points!) |
DHowett
reviewed
Oct 8, 2020
ghost
added
the
Needs-Author-Feedback
DHowett
approved these changes
Oct 8, 2020
skyline75489
pushed a commit
to skyline75489/terminal
that referenced
this pull request
Oct 10, 2020
## Summary of the Pull Request Introduce the `IconPathConverter` to `TerminalApp`. `Command` and `Profile` now both return the unexpanded icon path. `IconPathConverter` is responsible for expanding the icon path and retrieving the appropriate icon source. This also removes `Profile`'s expanded icon path and uses the `IconPathConverter` when necessary. This allows users to set profile icons to emoji as well. However, emoji do not appear in the jumplist. ## References Based on microsoft#7667 ## PR Checklist * [X] Closes microsoft#7784 * [x] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [x] Schema updated. ## Validation Steps Performed Deploy succeeded.
|
🎉 Handy links: |
ghost
mentioned this pull request
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary of the Pull Request
Introduce the
IconPathConvertertoTerminalApp.CommandandProfilenow both return the unexpanded icon path.IconPathConverteris responsible for expanding the icon path and retrieving the appropriate icon source.This also removes
Profile's expanded icon path and uses theIconPathConverterwhen necessary. This allows users to set profile icons to emoji as well. However, emoji do not appear in the jumplist.References
Based on #7667
PR Checklist
Validation Steps Performed
Deploy succeeded.