CmdPal: Enable dock pinning and per-profile icons for Windows Terminal#46372
CmdPal: Enable dock pinning and per-profile icons for Windows Terminal#46372
Conversation
Enable Windows Terminal profiles to be pinned to the Command Palette dock
and show per-profile icons instead of the generic Terminal app logo.
Dock pinning:
- Set stable Id on LaunchProfileCommand (terminal/{appUserModelId}/{profileName})
- Override GetCommandItem() in WindowsTerminalCommandsProvider so the dock
can resolve pinned commands on load
Per-profile icons:
- Add InstallPath to TerminalPackage for resolving ms-appx:/// URIs
- Add ResolveProfileIcon() helper that maps ms-appx:/// paths to the
Terminal package install directory with scale variant probing
- Set resolved per-profile icon on ListItem and LaunchProfileCommand
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
59f6260 to
c533c34
Compare
|
YAY |
There was a problem hiding this comment.
Pull request overview
Enables Windows Terminal profile commands in CmdPal to support dock pinning (via stable command IDs and provider-side rehydration) and improves UX by showing per-profile icons rather than the generic Terminal app icon.
Changes:
- Assigns stable IDs to
LaunchProfileCommandand addsGetCommandItem(string id)to rehydrate pinned profile commands. - Adds
InstallPathtoTerminalPackageand uses it to resolve profile icons (includingms-appx:///and GUID-based built-in profile icons). - Updates the profiles list to use resolved per-profile icons for both list items and launch commands.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.WindowsTerminal/WindowsTerminalCommandsProvider.cs | Adds GetCommandItem implementation to resolve pinned profile commands by ID. |
| src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.WindowsTerminal/TerminalPackage.cs | Extends package model with InstallPath needed for icon resolution. |
| src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.WindowsTerminal/Pages/ProfilesListPage.cs | Applies resolved per-profile icons to the rendered list items and commands. |
| src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.WindowsTerminal/Helpers/TerminalQuery.cs | Populates InstallPath from the discovered Terminal package. |
| src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.WindowsTerminal/Helpers/TerminalHelper.cs | Adds icon resolution logic and updates GUID parsing under nullable context. |
| src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.WindowsTerminal/Commands/LaunchProfileCommand.cs | Generates and assigns stable command IDs for profile launch commands. |
…ges/ProfilesListPage.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…lpers/TerminalHelper.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ndowsTerminalCommandsProvider.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@michaeljolley I've opened a new pull request, #46481, to work on those changes. Once the pull request is ready, I'll request review from you. |
…ove ambiguity (#46481) ## Summary of the Pull Request `LaunchProfileCommand` had a constructor parameter `id` and backing field `_id` that held the Terminal `AppUserModelId` used for COM activation, which was ambiguous given the base class `Id` property (set to a generated command identifier via `MakeId`). Renames the parameter and field throughout to `appUserModelId`/`_appUserModelId`. ## PR Checklist - [ ] **Communication:** I've discussed this with core contributors already. If the work hasn't been agreed, this work might be rejected - [ ] **Tests:** Added/updated and all pass - [ ] **Localization:** All end-user-facing strings can be localized - [ ] **Dev docs:** Added/updated - [ ] **New binaries:** Added on the required places - [ ] [JSON for signing](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ESRPSigning_core.json) for new binaries - [ ] [WXS for installer](https://github.com/microsoft/PowerToys/blob/main/installer/PowerToysSetup/Product.wxs) for new binaries and localization folder - [ ] [YML for CI pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ci/templates/build-powertoys-steps.yml) for new test projects - [ ] [YML for signed pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/release.yml) - [ ] **Documentation updated:** If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/windows-uwp/tree/docs/hub/powertoys) and link it here: #xxx ## Detailed Description of the Pull Request / Additional comments Addresses review feedback on #46372. Changes in `LaunchProfileCommand.cs`: - `_id` field → `_appUserModelId` - Constructor parameter `id` → `appUserModelId` - Private `Launch(string id, ...)` → `Launch(string appUserModelId, ...)` Before, both the activation ID and the command identity used `id`-derived names, making it easy to accidentally pass the wrong value. The `MakeId` static method already used `appUserModelId` as its parameter; this aligns the rest of the class with that convention. ```csharp // Before private readonly string _id; internal LaunchProfileCommand(string id, ...) { this._id = id; this.Id = MakeId(id, profile); } // After private readonly string _appUserModelId; internal LaunchProfileCommand(string appUserModelId, ...) { this._appUserModelId = appUserModelId; this.Id = MakeId(appUserModelId, profile); } ``` ## Validation Steps Performed - Code review: no issues flagged. - Caller site (`ProfilesListPage.cs`) already passes `profile.Terminal.AppUserModelId` — no functional change. <!-- START COPILOT CODING AGENT TIPS --> --- 💬 Send tasks to Copilot coding agent from [Slack](https://gh.io/cca-slack-docs) and [Teams](https://gh.io/cca-teams-docs) to turn conversations into code. Copilot posts an update in your thread when it's finished. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: michaeljolley <1228996+michaeljolley@users.noreply.github.com>
| } | ||
|
|
||
| private void Launch(string id, string profile) | ||
| internal static string MakeId(string appUserModelId, string profileName) => $"terminal/{appUserModelId}/{profileName}"; |
There was a problem hiding this comment.
Not sure how this would fit with the deep-links... I assume we'd like to end up with sth like: x-cmdpal://commands/providerId/commandId and question is if / should be a reserved char or not? In theory we can treat commandId as catch-all parameter
There was a problem hiding this comment.
@jiripolasek what is your recommendation here?
|
@copilot What's causing the CI to fail? can you sync this branch up with main? |
Co-authored-by: niels9001 <9866362+niels9001@users.noreply.github.com>
The CI failures were caused by the branch being out of sync with |
This comment has been minimized.
This comment has been minimized.
|
@copilot sync this branch with main |
…inal-dock-pinning Co-authored-by: niels9001 <9866362+niels9001@users.noreply.github.com>
Done — merged latest |
| var iconPath = TerminalHelper.ResolveProfileIcon(profile); | ||
|
|
||
| result.Add(new ListItem(new LaunchProfileCommand(profile.Terminal.AppUserModelId, profile.Name, iconPath, openNewTab, openQuake, _appSettingsManager)) |
There was a problem hiding this comment.
weird that we're resolving the icon path to a string, then constructing two different IconInfors for that string (both here, and down in the LaunchProfileCommand). Pretty sure we don't need the one up here, and the listitem will just use the command's icon
Summary of the Pull Request
Enables Windows Terminal profiles to be pinned to the Command Palette dock and shows per-profile icons instead of the generic Terminal app logo.
Dock Pinning
Root cause:
LaunchProfileCommand.Idwas never set (defaulted tostring.Empty). The context menu factory checks!string.IsNullOrEmpty(itemId)before showing "Pin to Dock", so the option was silently hidden. Additionally,GetCommandItem()was not overridden, so pinned commands could not be resolved on load.Fix:
IdonLaunchProfileCommand(format:terminal/{appUserModelId}/{profileName})id→appUserModelIdto avoid ambiguity with the generated commandIdGetCommandItem(string id)inWindowsTerminalCommandsProviderto look up profile items by IDPer-Profile Icons
Root cause: All profiles showed the same Terminal application logo. The per-profile
iconfield from Terminal settings.json was parsed intoTerminalProfile.Iconbut never used.Fix:
InstallPathtoTerminalPackage(fromPackage.InstalledPath)ResolveProfileIcon()helper toTerminalHelperthat resolvesms-appx:///URIs to the Terminal package install directory (with scale variant probing), passes through file paths and glyphs, and falls back to the Terminal logoListItem.IconandLaunchProfileCommandRobustness
Guid.TryParseinstead ofGuid.Parsewhen reading profile GUIDs from settings.json, so a malformed or empty GUID value does not throw and take down the entire profile listPR Checklist
Detailed Description of the Pull Request / Additional comments
Files changed:
Commands/LaunchProfileCommand.cs— Assigns stableId, renamesid→appUserModelIdthroughout, addsstatic MakeId()helperHelpers/TerminalHelper.cs— AddsResolveProfileIcon(),TryResolveGuidIcon(), andResolveAppxPath()helpers; hardens GUID parsing withGuid.TryParseHelpers/TerminalQuery.cs— Passesp.InstalledPathto theTerminalPackageconstructorPages/ProfilesListPage.cs— Resolves and caches icon path per profile; setsListItem.IconTerminalPackage.cs— AddsInstallPathpropertyWindowsTerminalCommandsProvider.cs— OverridesGetCommandItem(string id)for dock-pinned command rehydration; removes unusedusingdirectiveValidation Steps Performed