Skip to content

Invoke onProgress callback in SdkManager.InstallPackagesAsync #40

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

Merged
jfversluis merged 2 commits into from
Mar 30, 2026
Merged

Invoke onProgress callback in SdkManager.InstallPackagesAsync #40

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
499c768
Invoke onProgress callback in SdkManager.InstallPackagesAsync
rmarinho Mar 30, 2026
f3cbd32
Merge branch 'main' into fix/sdk-install-progress
jfversluis Mar 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions src/Client/Microsoft.Maui.Client.UnitTests/AndroidProviderTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,25 @@ public async Task InstallPackagesAsync_InstallsMultiplePackages()
Assert.Contains("platforms;android-35", installedPackages);
}

[Fact]
public async Task InstallPackagesAsync_InvokesOnProgressForEachPackage()
{
// Arrange
var provider = new FakeAndroidProvider();
var packages = new[] { "platform-tools", "build-tools;35.0.0", "platforms;android-35" };
var progressCalls = new List<(string pkg, int idx, int total)>();

// Act
await provider.InstallPackagesAsync(packages, acceptLicenses: true,
onProgress: (pkg, idx, total) => progressCalls.Add((pkg, idx, total)));

// Assert
Assert.Equal(3, progressCalls.Count);
Assert.Equal(("platform-tools", 1, 3), progressCalls[0]);
Assert.Equal(("build-tools;35.0.0", 2, 3), progressCalls[1]);
Assert.Equal(("platforms;android-35", 3, 3), progressCalls[2]);
Comment on lines +201 to +203
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test asserts 1-based idx values (1..N). However, the CLI formatting in AndroidCommands.Install uses idx + 1 when displaying ({idx + 1}/{total}), implying it expects 0-based indexing. Update the test expectations to match the finalized onProgress contract (and ensure the production caller aligns), otherwise the UI can end up off-by-one while tests still pass.

Suggested change
Assert.Equal(("platform-tools", 1, 3), progressCalls[0]);
Assert.Equal(("build-tools;35.0.0", 2, 3), progressCalls[1]);
Assert.Equal(("platforms;android-35", 3, 3), progressCalls[2]);
Assert.Equal(("platform-tools", 0, 3), progressCalls[0]);
Assert.Equal(("build-tools;35.0.0", 1, 3), progressCalls[1]);
Assert.Equal(("platforms;android-35", 2, 3), progressCalls[2]);

Copilot uses AI. Check for mistakes.
}

[Fact]
public async Task GetAvailablePackagesAsync_ReturnsAvailablePackages()
{
Expand Down
8 changes: 7 additions & 1 deletion src/Client/Microsoft.Maui.Client.UnitTests/Fakes/FakeAndroidProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,13 @@ public Task InstallPackagesAsync(IEnumerable<string> packages, bool acceptLicens

public Task InstallPackagesAsync(IEnumerable<string> packages, bool acceptLicenses, Action<string, int, int>? onProgress, CancellationToken cancellationToken = default)
{
InstalledPackageSets.Add(packages.ToList());
var pkgList = packages.ToList();
InstalledPackageSets.Add(pkgList);
if (onProgress is not null)
{
for (var i = 0; i < pkgList.Count; i++)
onProgress(pkgList[i], i + 1, pkgList.Count);
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fake invokes onProgress with idx as i + 1 (1-based). If production code/callers expect 0-based indexing (as AndroidCommands.Install currently does), this will cause tests to pass while the UI is off-by-one. Align the fake with the chosen contract (0-based vs 1-based) and keep it consistent with SdkManager.InstallPackagesAsync.

Suggested change
onProgress(pkgList[i], i + 1, pkgList.Count);
onProgress(pkgList[i], i, pkgList.Count);

Copilot uses AI. Check for mistakes.
}
return Task.CompletedTask;
}

Expand Down
17 changes: 16 additions & 1 deletion src/Client/Microsoft.Maui.Client/Providers/Android/SdkManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,9 +106,24 @@ public async Task InstallPackagesAsync(IEnumerable<string> packages, bool accept
SyncPaths();
EnsureAvailable();

var packageList = packages.ToList();

try
{
Comment on lines +109 to 112
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

packages.ToList() is executed before the try/catch. If packages is null or its enumerator throws, the exception will bypass the existing error translation (UnauthorizedAccessException/MauiToolException) and change behavior compared to the previous implementation. Consider moving the ToList() inside the try block and/or validating packages with an explicit ArgumentNullException to keep error handling consistent.

Suggested change
var packageList = packages.ToList();
try
{
if (packages is null)
throw new System.ArgumentNullException(nameof(packages));
try
{
var packageList = packages.ToList();

Copilot uses AI. Check for mistakes.
await _sdkManager.InstallAsync(packages, acceptLicenses, cancellationToken);
if (onProgress is null)
{
await _sdkManager.InstallAsync(packageList, acceptLicenses, cancellationToken);
}
else
{
// Install one at a time so we can report per-package progress
for (var i = 0; i < packageList.Count; i++)
{
cancellationToken.ThrowIfCancellationRequested();
onProgress(packageList[i], i + 1, packageList.Count);
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onProgress is currently invoked with idx as i + 1 (1-based). The only in-repo caller (AndroidCommands.Install) formats ({idx + 1}/{total}), which will display 2..(total+1) and can show 4/3 on the last package. Either change the callback contract to be 0-based here (pass i) or update the caller (and document the contract) so all consumers agree on the indexing semantics.

Suggested change
onProgress(packageList[i], i + 1, packageList.Count);
// onProgress index is 0-based; callers can display (idx + 1)/total
onProgress(packageList[i], i, packageList.Count);

Copilot uses AI. Check for mistakes.
await _sdkManager.InstallAsync(new[] { packageList[i] }, acceptLicenses, cancellationToken);
}
}
}
catch (Exception ex) when (ex is not OperationCanceledException)
{
Expand Down