mirror of
https://github.com/microsoft/PowerToys.git
synced 2026-04-03 17:56:44 +02:00
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? --> ## Summary of the Pull Request A memory leak was identified where activating a Command Palette extension via a hotkey would lead to a linear increase in resource consumption and a corresponding decrease in search performance. Each activation created a new `PageViewModel` instance for the extension's UI. However, the `ShellViewModel` did not dispose of the previous `PageViewModel` instance when a new one was created. The fix is implemented in `Microsoft.CmdPal.UI.ViewModels/ShellViewModel.cs` by ensuring that `PageViewModel` instances are correctly disposed of when they are no longer active. <!-- Please review the items on the PR checklist before submitting--> ## PR Checklist - [x] **Closes:** #40199 - [ ] **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 <!-- Provide a more detailed description of the PR, other things fixed, or any additional comments/features here ## Detailed Description of the Pull Request / Additional comments --> <!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well --> ## Validation Steps Performed 1. Installed [Workspace Launcher for Visual Studio / Code](https://github.com/tanchekwei/WorkspaceLauncherForVSCode) 2. Configured a global hotkey for the extension (e.g., Alt + Q). 3. Ran the following AHK script. Pressing F1 simulates pressing the hotkey 10 times with a 200ms interval, followed by typing p to trigger a search and observe how many times `GetItems` is invoked. ```ahk #NoEnv SetBatchLines, -1 ; Press F1 to run the sequence F1:: Loop, 10 { Send, !q Sleep, 200 } Send, p return ``` ### Before the Fix (PowerToys v0.91.1) - Pressing the hotkey 10 times then search will triggered `GetItems` 12 times. - See video and log output below: https://github.com/user-attachments/assets/3c7d59d6-2dda-4ab4-b230-2f2472c45776 ```log [2025-06-25 00:04:58.251] [VisualStudioCodePage.UpdateSearchText] Started [2025-06-25 00:04:58.251] [VisualStudioCodePage.UpdateSearchText] SearchText: p [2025-06-25 00:04:58.251] [WorkspaceFilter.Filter] Started [2025-06-25 00:04:58.263] [WorkspaceFilter.Filter] Finished in 11ms [2025-06-25 00:04:58.269] [VisualStudioCodePage.GetItems] Started [2025-06-25 00:04:58.269] [VisualStudioCodePage.GetItems] Finished in 0ms [2025-06-25 00:04:58.373] [VisualStudioCodePage.GetItems] Started [2025-06-25 00:04:58.373] [VisualStudioCodePage.GetItems] Finished in 0ms [2025-06-25 00:04:58.408] [VisualStudioCodePage.GetItems] Started [2025-06-25 00:04:58.408] [VisualStudioCodePage.GetItems] Finished in 0ms [2025-06-25 00:04:58.445] [VisualStudioCodePage.GetItems] Started [2025-06-25 00:04:58.446] [VisualStudioCodePage.GetItems] Finished in 0ms [2025-06-25 00:04:58.485] [VisualStudioCodePage.GetItems] Started [2025-06-25 00:04:58.486] [VisualStudioCodePage.GetItems] Finished in 0ms [2025-06-25 00:04:58.524] [VisualStudioCodePage.GetItems] Started [2025-06-25 00:04:58.525] [VisualStudioCodePage.GetItems] Finished in 0ms [2025-06-25 00:04:58.563] [VisualStudioCodePage.GetItems] Started [2025-06-25 00:04:58.564] [VisualStudioCodePage.GetItems] Finished in 0ms [2025-06-25 00:04:58.604] [VisualStudioCodePage.GetItems] Started [2025-06-25 00:04:58.604] [VisualStudioCodePage.GetItems] Finished in 0ms [2025-06-25 00:04:58.645] [VisualStudioCodePage.GetItems] Started [2025-06-25 00:04:58.645] [VisualStudioCodePage.GetItems] Finished in 0ms [2025-06-25 00:04:58.685] [VisualStudioCodePage.GetItems] Started [2025-06-25 00:04:58.686] [VisualStudioCodePage.GetItems] Finished in 0ms [2025-06-25 00:04:58.755] [VisualStudioCodePage.GetItems] Started [2025-06-25 00:04:58.756] [VisualStudioCodePage.GetItems] Finished in 0ms [2025-06-25 00:04:58.798] [VisualStudioCodePage.GetItems] Started [2025-06-25 00:04:58.798] [VisualStudioCodePage.GetItems] Finished in 0ms [2025-06-25 00:04:58.843] [VisualStudioCodePage.UpdateSearchText] Finished in 591ms ``` ### After the Fix - Pressing the hotkey 10 times then search will now triggers `GetItems` only once. - See updated video and log output: https://github.com/user-attachments/assets/e38f3eb5-5353-44da-a9d9-ff83647eb6e9 ```log [2025-06-25 00:03:44.862] [VisualStudioCodePage.UpdateSearchText] Started [2025-06-25 00:03:44.863] [VisualStudioCodePage.UpdateSearchText] SearchText: p [2025-06-25 00:03:44.864] [WorkspaceFilter.Filter] Started [2025-06-25 00:03:44.866] [WorkspaceFilter.Filter] Finished in 2ms [2025-06-25 00:03:44.870] [VisualStudioCodePage.GetItems] Started [2025-06-25 00:03:44.870] [VisualStudioCodePage.GetItems] Finished in 0ms [2025-06-25 00:03:44.909] [VisualStudioCodePage.UpdateSearchText] Finished in 46ms ```
218 lines
8.3 KiB
C#
218 lines
8.3 KiB
C#
// Copyright (c) Microsoft Corporation
|
|
// The Microsoft Corporation licenses this file to you under the MIT license.
|
|
// See the LICENSE file in the project root for more information.
|
|
|
|
using System.Runtime.InteropServices;
|
|
using System.Runtime.Versioning;
|
|
using CommunityToolkit.Mvvm.ComponentModel;
|
|
using CommunityToolkit.Mvvm.Input;
|
|
using CommunityToolkit.Mvvm.Messaging;
|
|
using ManagedCommon;
|
|
using Microsoft.CmdPal.Common.Services;
|
|
using Microsoft.CmdPal.UI.ViewModels.MainPage;
|
|
using Microsoft.CmdPal.UI.ViewModels.Messages;
|
|
using Microsoft.CmdPal.UI.ViewModels.Models;
|
|
using Microsoft.CommandPalette.Extensions;
|
|
using Microsoft.Extensions.DependencyInjection;
|
|
using WinRT;
|
|
|
|
namespace Microsoft.CmdPal.UI.ViewModels;
|
|
|
|
public partial class ShellViewModel(IServiceProvider _serviceProvider, TaskScheduler _scheduler) : ObservableObject
|
|
{
|
|
[ObservableProperty]
|
|
public partial bool IsLoaded { get; set; } = false;
|
|
|
|
[ObservableProperty]
|
|
public partial DetailsViewModel? Details { get; set; }
|
|
|
|
[ObservableProperty]
|
|
public partial bool IsDetailsVisible { get; set; }
|
|
|
|
private PageViewModel _currentPage = new LoadingPageViewModel(null, _scheduler);
|
|
|
|
public PageViewModel CurrentPage
|
|
{
|
|
get => _currentPage;
|
|
set
|
|
{
|
|
var oldValue = _currentPage;
|
|
if (SetProperty(ref _currentPage, value))
|
|
{
|
|
if (oldValue is IDisposable disposable)
|
|
{
|
|
try
|
|
{
|
|
disposable.Dispose();
|
|
}
|
|
catch (Exception ex)
|
|
{
|
|
Logger.LogError(ex.ToString());
|
|
}
|
|
}
|
|
}
|
|
}
|
|
}
|
|
|
|
private MainListPage? _mainListPage;
|
|
|
|
private IExtensionWrapper? _activeExtension;
|
|
|
|
[RelayCommand]
|
|
public async Task<bool> LoadAsync()
|
|
{
|
|
var tlcManager = _serviceProvider.GetService<TopLevelCommandManager>();
|
|
await tlcManager!.LoadBuiltinsAsync();
|
|
IsLoaded = true;
|
|
|
|
// Built-ins have loaded. We can display our page at this point.
|
|
_mainListPage = new MainListPage(_serviceProvider);
|
|
WeakReferenceMessenger.Default.Send<PerformCommandMessage>(new(new ExtensionObject<ICommand>(_mainListPage)));
|
|
|
|
_ = Task.Run(async () =>
|
|
{
|
|
// After loading built-ins, and starting navigation, kick off a thread to load extensions.
|
|
tlcManager.LoadExtensionsCommand.Execute(null);
|
|
|
|
await tlcManager.LoadExtensionsCommand.ExecutionTask!;
|
|
if (tlcManager.LoadExtensionsCommand.ExecutionTask.Status != TaskStatus.RanToCompletion)
|
|
{
|
|
// TODO: Handle failure case
|
|
}
|
|
});
|
|
|
|
return true;
|
|
}
|
|
|
|
public void LoadPageViewModel(PageViewModel viewModel)
|
|
{
|
|
// Note: We removed the general loading state, extensions sometimes use their `IsLoading`, but it's inconsistently implemented it seems.
|
|
// IsInitialized is our main indicator of the general overall state of loading props/items from a page we use for the progress bar
|
|
// This triggers that load generally with the InitializeCommand asynchronously when we navigate to a page.
|
|
// We could re-track the page loading status, if we need it more granularly below again, but between the initialized and error message, we can infer some status.
|
|
// We could also maybe move this thread offloading we do for loading into PageViewModel and better communicate between the two... a few different options.
|
|
|
|
////LoadedState = ViewModelLoadedState.Loading;
|
|
if (!viewModel.IsInitialized
|
|
&& viewModel.InitializeCommand != null)
|
|
{
|
|
_ = Task.Run(async () =>
|
|
{
|
|
// You know, this creates the situation where we wait for
|
|
// both loading page properties, AND the items, before we
|
|
// display anything.
|
|
//
|
|
// We almost need to do an async await on initialize, then
|
|
// just a fire-and-forget on FetchItems.
|
|
// RE: We do set the CurrentPage in ShellPage.xaml.cs as well, so, we kind of are doing two different things here.
|
|
// Definitely some more clean-up to do, but at least its centralized to one spot now.
|
|
viewModel.InitializeCommand.Execute(null);
|
|
|
|
await viewModel.InitializeCommand.ExecutionTask!;
|
|
|
|
if (viewModel.InitializeCommand.ExecutionTask.Status != TaskStatus.RanToCompletion)
|
|
{
|
|
// TODO: Handle failure case
|
|
if (viewModel.InitializeCommand.ExecutionTask.Exception is AggregateException ex)
|
|
{
|
|
Logger.LogError(ex.ToString());
|
|
}
|
|
|
|
// TODO GH #239 switch back when using the new MD text block
|
|
// _ = _queue.EnqueueAsync(() =>
|
|
/*_queue.TryEnqueue(new(() =>
|
|
{
|
|
LoadedState = ViewModelLoadedState.Error;
|
|
}));*/
|
|
}
|
|
else
|
|
{
|
|
// TODO GH #239 switch back when using the new MD text block
|
|
// _ = _queue.EnqueueAsync(() =>
|
|
_ = Task.Factory.StartNew(
|
|
() =>
|
|
{
|
|
// bool f = await viewModel.InitializeCommand.ExecutionTask.;
|
|
// var result = viewModel.InitializeCommand.ExecutionTask.GetResultOrDefault()!;
|
|
// var result = viewModel.InitializeCommand.ExecutionTask.GetResultOrDefault<bool?>()!;
|
|
CurrentPage = viewModel; // result ? viewModel : null;
|
|
////LoadedState = result ? ViewModelLoadedState.Loaded : ViewModelLoadedState.Error;
|
|
},
|
|
CancellationToken.None,
|
|
TaskCreationOptions.None,
|
|
_scheduler);
|
|
}
|
|
});
|
|
}
|
|
else
|
|
{
|
|
CurrentPage = viewModel;
|
|
////LoadedState = ViewModelLoadedState.Loaded;
|
|
}
|
|
}
|
|
|
|
public void PerformTopLevelCommand(PerformCommandMessage message)
|
|
{
|
|
if (_mainListPage == null)
|
|
{
|
|
return;
|
|
}
|
|
|
|
if (message.Context is IListItem listItem)
|
|
{
|
|
_mainListPage.UpdateHistory(listItem);
|
|
}
|
|
}
|
|
|
|
public void SetActiveExtension(IExtensionWrapper? extension)
|
|
{
|
|
if (extension != _activeExtension)
|
|
{
|
|
// There's not really a CoDisallowSetForegroundWindow, so we don't
|
|
// need to handle that
|
|
_activeExtension = extension;
|
|
|
|
var extensionWinRtObject = _activeExtension?.GetExtensionObject();
|
|
if (extensionWinRtObject != null)
|
|
{
|
|
try
|
|
{
|
|
unsafe
|
|
{
|
|
var winrtObj = (IWinRTObject)extensionWinRtObject;
|
|
var intPtr = winrtObj.NativeObject.ThisPtr;
|
|
var hr = Native.CoAllowSetForegroundWindow(intPtr);
|
|
if (hr != 0)
|
|
{
|
|
Logger.LogWarning($"Error giving foreground rights: 0x{hr.Value:X8}");
|
|
}
|
|
}
|
|
}
|
|
catch (Exception ex)
|
|
{
|
|
Logger.LogError(ex.ToString());
|
|
}
|
|
}
|
|
}
|
|
}
|
|
|
|
public void GoHome()
|
|
{
|
|
SetActiveExtension(null);
|
|
}
|
|
|
|
// You may ask yourself, why aren't we using CsWin32 for this?
|
|
// The CsWin32 projected version includes some object marshalling, like so:
|
|
//
|
|
// HRESULT CoAllowSetForegroundWindow([MarshalAs(UnmanagedType.IUnknown)] object pUnk,...)
|
|
//
|
|
// And if you do it like that, then the IForegroundTransfer interface isn't marshalled correctly
|
|
internal sealed class Native
|
|
{
|
|
[DllImport("OLE32.dll", ExactSpelling = true)]
|
|
[DefaultDllImportSearchPaths(DllImportSearchPath.System32)]
|
|
[SupportedOSPlatform("windows5.0")]
|
|
internal static extern unsafe global::Windows.Win32.Foundation.HRESULT CoAllowSetForegroundWindow(nint pUnk, [Optional] void* lpvReserved);
|
|
}
|
|
}
|