Skip to content

Commit 82dc4cd

Browse files
moooyoYu Leng
andauthored
[CmdPal] Replace complex cancellation token mechanism with a simple task queue. (#42356)
<!-- 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 Just consider user are trying to search a long name such as "Visual Studio Code" The old mechanism: User input: V Task: V Then input: i Task cancel for V and start task i etc... The problem is: 1. I don't think we can really cancel the most time-cost part (Find packages from WinGet). 2. User cannot see anything before they really end the input and the last task complete. UX exp is so bad. 3. It's so complex to maintain. Hard to understand for the new contributor. New mechanism: User input: V Task: V Then input: i Prev Task is still running but mark the next task is i Input: s Prev Task is still running but override the next task to s etc... We can get: 1. User can see some results if prev task complete. 2. It's simple to understand 3. The extra time cost I think will not too much. Because we ignored the middle input. Compare: https://github.com/user-attachments/assets/f45f4073-efab-4f43-87f0-f47b727f36dc <!-- Please review the items on the PR checklist before submitting--> ## PR Checklist - [ ] Closes: #xxx - [ ] **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 Co-authored-by: Yu Leng <yuleng@microsoft.com>
1 parent c71fdca commit 82dc4cd

File tree

1 file changed

+59
-67
lines changed

1 file changed

+59
-67
lines changed

‎src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.WinGet/Pages/WinGetExtensionPage.cs‎

Lines changed: 59 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -28,9 +28,10 @@ internal sealed partial class WinGetExtensionPage : DynamicListPage, IDisposable
2828
public bool HasTag => !string.IsNullOrEmpty(_tag);
2929

3030
private readonly Lock _resultsLock = new();
31+
private readonly Lock _taskLock = new();
3132

32-
private CancellationTokenSource? _cancellationTokenSource;
33-
private Task<IEnumerable<CatalogPackage>>? _currentSearchTask;
33+
private string? _nextSearchQuery;
34+
private bool _isTaskRunning;
3435

3536
private List<CatalogPackage>? _results;
3637

@@ -85,7 +86,6 @@ public override IListItem[] GetItems()
8586

8687
stopwatch.Stop();
8788
Logger.LogDebug($"Building ListItems took {stopwatch.ElapsedMilliseconds}ms", memberName: nameof(GetItems));
88-
IsLoading = false;
8989
return results;
9090
}
9191
}
@@ -98,8 +98,6 @@ public override IListItem[] GetItems()
9898
Properties.Resources.winget_no_packages_found,
9999
};
100100

101-
IsLoading = false;
102-
103101
return [];
104102
}
105103

@@ -117,64 +115,70 @@ public override void UpdateSearchText(string oldSearch, string newSearch)
117115

118116
private void DoUpdateSearchText(string newSearch)
119117
{
120-
// Cancel any ongoing search
121-
if (_cancellationTokenSource is not null)
118+
lock (_taskLock)
122119
{
123-
Logger.LogDebug("Cancelling old search", memberName: nameof(DoUpdateSearchText));
124-
_cancellationTokenSource.Cancel();
125-
}
126-
127-
_cancellationTokenSource = new CancellationTokenSource();
128-
129-
var cancellationToken = _cancellationTokenSource.Token;
130-
131-
IsLoading = true;
120+
if (_isTaskRunning)
121+
{
122+
// If a task is running, queue the next search query
123+
// Keep IsLoading = true since we still have work to do
124+
Logger.LogDebug($"Task is running, queueing next search: '{newSearch}'", memberName: nameof(DoUpdateSearchText));
125+
_nextSearchQuery = newSearch;
126+
}
127+
else
128+
{
129+
// No task is running, start a new search
130+
Logger.LogDebug($"Starting new search: '{newSearch}'", memberName: nameof(DoUpdateSearchText));
131+
_isTaskRunning = true;
132+
_nextSearchQuery = null;
133+
IsLoading = true;
132134

133-
try
134-
{
135-
// Save the latest search task
136-
_currentSearchTask = DoSearchAsync(newSearch, cancellationToken);
137-
}
138-
catch (OperationCanceledException)
139-
{
140-
// DO NOTHING HERE
141-
return;
142-
}
143-
catch (Exception ex)
144-
{
145-
// Handle other exceptions
146-
ExtensionHost.LogMessage($"[WinGet] DoUpdateSearchText throw exception: {ex.Message}");
147-
return;
135+
_ = ExecuteSearchChainAsync(newSearch);
136+
}
148137
}
149-
150-
// Await the task to ensure only the latest one gets processed
151-
_ = ProcessSearchResultsAsync(_currentSearchTask, newSearch);
152138
}
153139

154-
private async Task ProcessSearchResultsAsync(
155-
Task<IEnumerable<CatalogPackage>> searchTask,
156-
string newSearch)
140+
private async Task ExecuteSearchChainAsync(string query)
157141
{
158-
try
142+
while (true)
159143
{
160-
var results = await searchTask;
144+
try
145+
{
146+
Logger.LogDebug($"Executing search for '{query}'", memberName: nameof(ExecuteSearchChainAsync));
147+
148+
var results = await DoSearchAsync(query);
161149

162-
// Ensure this is still the latest task
163-
if (_currentSearchTask == searchTask)
150+
// Update UI with results
151+
UpdateWithResults(results, query);
152+
}
153+
catch (Exception ex)
164154
{
165-
// Process the results (e.g., update UI)
166-
UpdateWithResults(results, newSearch);
155+
Logger.LogError($"Unexpected error while searching for '{query}'", ex);
167156
}
168-
}
169-
catch (OperationCanceledException)
170-
{
171-
// Handle cancellation gracefully (e.g., log or ignore)
172-
Logger.LogDebug($" Cancelled search for '{newSearch}'");
173-
}
174-
catch (Exception ex)
175-
{
176-
// Handle other exceptions
177-
Logger.LogError("Unexpected error while processing results", ex);
157+
158+
// Check if there's a next query to process
159+
string? nextQuery;
160+
lock (_taskLock)
161+
{
162+
if (_nextSearchQuery is not null)
163+
{
164+
// There's a queued search, execute it
165+
nextQuery = _nextSearchQuery;
166+
_nextSearchQuery = null;
167+
168+
Logger.LogDebug($"Found queued search, continuing with: '{nextQuery}'", memberName: nameof(ExecuteSearchChainAsync));
169+
}
170+
else
171+
{
172+
// No more searches queued, mark task as completed
173+
_isTaskRunning = false;
174+
IsLoading = false;
175+
Logger.LogDebug("No more queued searches, task chain completed", memberName: nameof(ExecuteSearchChainAsync));
176+
break;
177+
}
178+
}
179+
180+
// Continue with the next query
181+
query = nextQuery;
178182
}
179183
}
180184

@@ -189,11 +193,8 @@ private void UpdateWithResults(IEnumerable<CatalogPackage> results, string query
189193
RaiseItemsChanged();
190194
}
191195

192-
private async Task<IEnumerable<CatalogPackage>> DoSearchAsync(string query, CancellationToken ct)
196+
private async Task<IEnumerable<CatalogPackage>> DoSearchAsync(string query)
193197
{
194-
// Were we already canceled?
195-
ct.ThrowIfCancellationRequested();
196-
197198
Stopwatch stopwatch = new();
198199
stopwatch.Start();
199200

@@ -230,9 +231,6 @@ private async Task<IEnumerable<CatalogPackage>> DoSearchAsync(string query, Canc
230231
opts.Filters.Add(tagFilter);
231232
}
232233

233-
// Clean up here, then...
234-
ct.ThrowIfCancellationRequested();
235-
236234
var catalogTask = HasTag ? WinGetStatics.CompositeWingetCatalog : WinGetStatics.CompositeAllCatalog;
237235

238236
// Both these catalogs should have been instantiated by the
@@ -251,13 +249,11 @@ private async Task<IEnumerable<CatalogPackage>> DoSearchAsync(string query, Canc
251249
findPackages_stopwatch.Start();
252250
Logger.LogDebug($" Searching {catalog.Info.Name} ({query})", memberName: nameof(DoSearchAsync));
253251

254-
ct.ThrowIfCancellationRequested();
255-
256252
Logger.LogDebug($"Preface for \"{searchDebugText}\" took {stopwatch.ElapsedMilliseconds}ms", memberName: nameof(DoSearchAsync));
257253

258254
// BODGY, re: microsoft/winget-cli#5151
259255
// FindPackagesAsync isn't actually async.
260-
var internalSearchTask = Task.Run(() => catalog.FindPackages(opts), ct);
256+
var internalSearchTask = Task.Run(() => catalog.FindPackages(opts));
261257
var searchResults = await internalSearchTask;
262258

263259
findPackages_stopwatch.Stop();
@@ -271,8 +267,6 @@ private async Task<IEnumerable<CatalogPackage>> DoSearchAsync(string query, Canc
271267
return [];
272268
}
273269

274-
ct.ThrowIfCancellationRequested();
275-
276270
Logger.LogDebug($" got results for ({query})", memberName: nameof(DoSearchAsync));
277271

278272
// FYI Using .ToArray or any other kind of enumerable loop
@@ -282,8 +276,6 @@ private async Task<IEnumerable<CatalogPackage>> DoSearchAsync(string query, Canc
282276
{
283277
var match = searchResults.Matches[i];
284278

285-
ct.ThrowIfCancellationRequested();
286-
287279
var package = match.CatalogPackage;
288280
results.Add(package);
289281
}

0 commit comments

Comments
 (0)