Skip to content

CmdPal: Wire up ILogger<T> in ShellViewModel and PowerToysRootPageService#46779

Draft
Copilot wants to merge 1 commit intomainfrom
copilot/research-full-aot-compliance
Draft

CmdPal: Wire up ILogger<T> in ShellViewModel and PowerToysRootPageService#46779
Copilot wants to merge 1 commit intomainfrom
copilot/research-full-aot-compliance

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 5, 2026

Summary of the Pull Request

Follow-up to #46768 which added the CmdPalLogger / CmdPalLoggerProvider / AddCmdPalLogging infrastructure with the explicit note: "This logging is not in use currently, but will be in a future PR."

This is that future PR — it wires up ILogger<T> injection in the two key DI-registered singleton services that previously used the old static logging patterns.

Changes

Microsoft.CmdPal.UI.ViewModels.csproj

  • Adds Microsoft.Extensions.Logging.Abstractions as a direct PackageReference (version already pinned in Directory.Packages.props) so that ILogger<T> is available in the ViewModels project.

ShellViewModel.cs

  • Removes using Microsoft.CmdPal.Common; (was only needed for CoreLogger)
  • Adds using Microsoft.Extensions.Logging;
  • Injects ILogger<ShellViewModel> as a new constructor parameter (last)
  • Replaces all 9 CoreLogger.* calls with idiomatic MEL-style _logger.* calls:
    • CoreLogger.LogError(ex.ToString())_logger.LogError(ex, "…") with descriptive messages and proper exception propagation
    • CoreLogger.LogError($"Failed to create ViewModel…")_logger.LogError("…{PageType}", page.GetType().Name) (structured)
    • CoreLogger.LogDebug(…)_logger.LogDebug("…")

PowerToysRootPageService.cs

  • Removes using ManagedCommon; (was only needed for Logger.* statics)
  • Adds using Microsoft.Extensions.Logging;
  • Injects ILogger<PowerToysRootPageService> as a new constructor parameter (last)
  • Replaces all 4 Logger.LogError / Logger.LogWarning / ManagedCommon.Logger.LogError calls with _logger.* equivalents

Both services are registered as DI singletons via services.AddSingleton<>() in App.xaml.cs. Because AddCmdPalLogging() was already registered in the DI container by #46768, the new ILogger<T> parameters are resolved automatically — no changes needed in App.xaml.cs.

PR Checklist

@jiripolasek
Copy link
Copy Markdown
Collaborator

@crutkas Hi, I pretty sure @michaeljolley has the folloup changes ready. I asked him to cheery-pick the logger registration, so could use it in a new code.

@crutkas
Copy link
Copy Markdown
Member

crutkas commented Apr 5, 2026

@jiripolasek and @michaeljolley this was me playing/learning. If it is done, 100% let’s close it out. If it can help the other PR, let’s cherry pick stuff in.

@crutkas
Copy link
Copy Markdown
Member

crutkas commented Apr 6, 2026

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants