Introduce IconConverter by carlos-zamora · Pull Request #7830 · microsoft/terminal

carlos-zamora · 2020-10-05T20:34:29Z

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 #7667

PR Checklist

Closes Command should expose IconPath, not IconSource #7784
Documentation updated. If checked, please file a pull request on our docs repo and link it here: #xxx
Schema updated.

Validation Steps Performed

Deploy succeeded.

carlos-zamora · 2020-10-05T20:34:42Z

Topics for Discussion

Should I move the IconPathConverter to TSM?
- We'll probably need this for Settings UI.
Emoji don't appear in jumplist
- I didn't look into this one too much. I don't know if it's possible.
- This'll be a problem if/when we allow actions in the jumplist
- Could I get some guidance here? Should I open a follow-up issue instead of figuring this out now?
- I need to update docs/schema saying that you can use emoji as profile icons. But I want this part to be resolved first before doing that.

DHowett · 2020-10-05T20:52:50Z

This is likely the difference between an Icon and an IconSource. I have no idea why there's two.

DHowett · 2020-10-05T20:53:15Z

technically IconPathConverter should live in TSE, but we can't have TerminalApp depending on it from there :|

DHowett · 2020-10-05T20:56:00Z

src/cascadia/TerminalApp/Jumplist.cpp

            // 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()) };


this will be hard to get emoji working w./

src/cascadia/TerminalSettingsModel/Profile.idl

DHowett · 2020-10-05T22:23:22Z

Oh, I see. No. Emoji will never work in the jumplist. Sorry.

src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp

src/cascadia/TerminalApp/IconPathConverter.cpp

src/cascadia/TerminalSettingsModel/Command.cpp

zadjii-msft

Overall, this is so very much better than it used to be. I suppose this counts as a nit, so

src/cascadia/inc/cppwinrt_utils.h

carlos-zamora · 2020-10-08T18:16:19Z

Verified schema now shows icon as an allowable property in command (bonus points!)

doc/cascadia/profiles.schema.json

DHowett

well ;P

## 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.

ghost · 2020-11-11T18:12:38Z

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

Handy links:

ghost added Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Oct 5, 2020

DHowett reviewed Oct 5, 2020

View reviewed changes

Base automatically changed from dev/cazamor/set/terminal-settings-model to master October 6, 2020 16:57

carlos-zamora mentioned this pull request Oct 6, 2020

Feature Request - Implement Settings UI #1564

Closed

34 tasks

This comment has been minimized.

Sign in to view

carlos-zamora added 2 commits October 6, 2020 11:02

Introduce IconConverter

fad6ba0

rename IconPath w/ Icon

09b184e

carlos-zamora force-pushed the dev/cazamor/set/tsm/cmd-icon branch from 022c018 to 09b184e Compare October 6, 2020 18:33

DHowett reviewed Oct 6, 2020

View reviewed changes

src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp Outdated Show resolved Hide resolved

DHowett reviewed Oct 6, 2020

View reviewed changes

src/cascadia/TerminalApp/IconPathConverter.cpp Outdated Show resolved Hide resolved

DHowett requested changes Oct 6, 2020

View reviewed changes

src/cascadia/TerminalApp/IconPathConverter.cpp Outdated Show resolved Hide resolved

src/cascadia/TerminalApp/IconPathConverter.cpp Outdated Show resolved Hide resolved

src/cascadia/TerminalSettingsModel/Command.cpp Outdated Show resolved Hide resolved

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Oct 6, 2020

carlos-zamora added 2 commits October 6, 2020 14:27

remove const from observable properties

de091c5

resolve a few more comments

2fe20ba

zadjii-msft self-assigned this Oct 7, 2020

zadjii-msft approved these changes Oct 7, 2020

View reviewed changes

src/cascadia/inc/cppwinrt_utils.h Show resolved Hide resolved

zadjii-msft removed their assignment Oct 7, 2020

carlos-zamora added 2 commits October 8, 2020 10:46

remove const_cast

d4231ac

update schema

c09cfc1

carlos-zamora mentioned this pull request Oct 8, 2020

Update icon support MicrosoftDocs/terminal#157

Merged

carlos-zamora assigned DHowett Oct 8, 2020

DHowett reviewed Oct 8, 2020

View reviewed changes

doc/cascadia/profiles.schema.json Show resolved Hide resolved

DHowett requested changes Oct 8, 2020

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 Oct 8, 2020

DHowett approved these changes Oct 8, 2020

View reviewed changes

carlos-zamora merged commit 4fc607a into master Oct 8, 2020

carlos-zamora deleted the dev/cazamor/set/tsm/cmd-icon branch October 8, 2020 18:29

zadjii-msft mentioned this pull request Oct 30, 2020

property icon of profiles/list[] is not working, while the value is emoji #8111

Closed

ghost mentioned this pull request Nov 11, 2020

Command should expose IconPath, not IconSource #7784

Closed

skyline75489 mentioned this pull request Jan 8, 2021

Customize profile appearance using emoji #8722

Closed

Conversation

carlos-zamora commented Oct 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of the Pull Request

References

PR Checklist

Validation Steps Performed

Uh oh!

carlos-zamora commented Oct 5, 2020

Topics for Discussion

Uh oh!

DHowett commented Oct 5, 2020

Uh oh!

DHowett commented Oct 5, 2020

Uh oh!

DHowett Oct 5, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DHowett commented Oct 5, 2020

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

carlos-zamora commented Oct 8, 2020

Uh oh!

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Nov 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

carlos-zamora commented Oct 5, 2020 •

edited

Loading