Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Command should expose IconPath, not IconSource #7784

Closed
carlos-zamora opened this issue Sep 30, 2020 · 1 comment · Fixed by #7830
Closed

Command should expose IconPath, not IconSource #7784

carlos-zamora opened this issue Sep 30, 2020 · 1 comment · Fixed by #7830
Assignees
@carlos-zamora
Labels
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. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
Terminal v1.5

Comments

@carlos-zamora
Copy link
Member

CommandPalette data binds itself to the IconSource of Command. Command should instead operate more like Profile in that it only exposes the IconPath.

CommandPalette can still be bound to the command's icon by using a converter like this:

            <local:HasNestedCommandsVisibilityConverter x:Key="HasNestedCommandsVisibilityConverter"/>

or

                                    Visibility="{x:Bind HasNestedCommands,
                                                 Mode=OneWay,
                                                 Converter={StaticResource HasNestedCommandsVisibilityConverter}}"

(also see CommandKeyChordVisibilityConverter)

This'll also be very useful for Profile too because...

  1. Profile icons will now be able to be set to non-path images (i.e. SegoeUI icons, emoji, etc.)
  2. Settings UI will need a similar kind of data binding in the very near future.
@carlos-zamora carlos-zamora added Product-Terminal The new Windows Terminal. Issue-Task It's a feature request, but it doesn't really need a major design. Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. labels Sep 30, 2020
@carlos-zamora carlos-zamora self-assigned this Sep 30, 2020
@ghost ghost added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Sep 30, 2020
@carlos-zamora carlos-zamora mentioned this issue Sep 30, 2020
Merged
@carlos-zamora carlos-zamora removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Sep 30, 2020
@carlos-zamora carlos-zamora added this to the Terminal v1.5 milestone Sep 30, 2020
This was referenced Oct 2, 2020
Merged
Merged
@ghost ghost added the In-PR This issue has a related PR label Oct 5, 2020
@ghost ghost added the Needs-Tag-Fix Doesn't match tag requirements label Oct 8, 2020
carlos-zamora added a commit that referenced this issue Oct 8, 2020
@carlos-zamora
Introduce IconConverter (#7830)
4fc607a 
## 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
* [X] Closes #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 ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Oct 8, 2020
skyline75489 pushed a commit to skyline75489/terminal that referenced this issue Oct 10, 2020
@carlos-zamora @skyline75489
Introduce IconConverter (microsoft#7830)
84f0fba 
## 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
Copy link

ghost commented Nov 11, 2020

🎉This issue was addressed in #7830, which has now been successfully released as Windows Terminal Preview v1.5.3142.0.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@carlos-zamora carlos-zamora

Labels
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. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
Terminal v1.5
Development

Successfully merging a pull request may close this issue.

Introduce IconConverter microsoft/terminal
1 participant
@carlos-zamora