[PR Feedback] Refactor error handling and optimize data structures

- Updated data structures for `progressWeights` to account for edge case.
- Refined progress reporting logic to handle edge cases and ensure accurate tracking.
- Replaced `std::wstring` with `hstring` in Add/Removal headers get a perf. benefits.
- Updated exception types in tests to reflect actual exceptions thrown.
- Removed unnecessary headers.

src/AppInstallerCLIE2ETests/Interop/PackageCatalogInterop.cs

@@ -77,7 +77,7 @@ public async Task AddRemovePackageCatalog()
         public void AddPackageCatalogWithInvalidOptions()
         {
             // Add package catalog with null options.
-            Assert.ThrowsAsync<ArgumentException>(async () => await this.packageManager.AddPackageCatalogAsync(null));
+            Assert.ThrowsAsync<NullReferenceException>(async () => await this.packageManager.AddPackageCatalogAsync(null));
 
             // Add package catalog with empty options.
             Assert.ThrowsAsync<ArgumentException>(async () => await this.packageManager.AddPackageCatalogAsync(this.TestFactory.CreateAddPackageCatalogOptions()));
@@ -235,7 +235,7 @@ public async Task AddRemovePackageCatalogWithPreserveDataFiledSet()
         public void RemovePackageCatalogWithInvalidOptions()
         {
             // Remove package catalog with null options.
-            Assert.ThrowsAsync<ArgumentException>(async () => await this.packageManager.RemovePackageCatalogAsync(null));
+            Assert.ThrowsAsync<NullReferenceException>(async () => await this.packageManager.RemovePackageCatalogAsync(null));
 
             // Remove package catalog with empty options.
             Assert.ThrowsAsync<ArgumentException>(async () => await this.packageManager.RemovePackageCatalogAsync(this.TestFactory.CreateRemovePackageCatalogOptions()));

src/AppInstallerCommonCore/Public/AppInstallerProgress.h

@@ -5,7 +5,6 @@
 #include <atomic>
 #include <functional>
 #include <string_view>
-#include <mutex>
 
 namespace AppInstaller
 {

src/Microsoft.Management.Deployment/AddPackageCatalogOptions.h

@@ -31,11 +31,11 @@ namespace winrt::Microsoft::Management::Deployment::implementation
 
 #if !defined(INCLUDE_ONLY_INTERFACE_METHODS)
     private:
-        std::wstring m_name = L"";
-        std::wstring m_sourceUri = L"";
-        std::wstring m_type = L"";
+        hstring m_name = L"";
+        hstring m_sourceUri = L"";
+        hstring m_type = L"";
         winrt::Microsoft::Management::Deployment::PackageCatalogTrustLevel m_trustLevel = winrt::Microsoft::Management::Deployment::PackageCatalogTrustLevel::None;
-        std::wstring m_customHeader = L"";
+        hstring m_customHeader = L"";
         bool m_explicit = false;
 #endif
     };

src/Microsoft.Management.Deployment/Converters.cpp

@@ -510,8 +510,6 @@ namespace winrt::Microsoft::Management::Deployment::implementation
         case APPINSTALLER_CLI_ERROR_SOURCE_NAME_ALREADY_EXISTS:
         case APPINSTALLER_CLI_ERROR_SOURCE_ARG_ALREADY_EXISTS:
             return AddPackageCatalogStatus::InvalidOptions;
-        case APPINSTALLER_CLI_ERROR_SOURCE_OPEN_FAILED:
-            return AddPackageCatalogStatus::CatalogError;
         default:
             return HandleCommonCatalogOperationStatus<AddPackageCatalogStatus>(hresult);
         }

src/Microsoft.Management.Deployment/PackageCatalogProgress.cpp

@@ -17,21 +17,21 @@ namespace winrt::Microsoft::Management::Deployment
             if (sourceType.empty()
                 || Utility::CaseInsensitiveEquals( Repository::Microsoft::PredefinedInstalledSourceFactory::Type(), sourceType))
             {
-                std::unordered_map<AppInstaller::ProgressType, double> progressWeights;
+                std::vector<std::pair<AppInstaller::ProgressType, double>> progressWeights;
 
-                // There is no download operation for remove operation, so remove the download progress.
+                // There is no download operation for remove operation, so use only percentage based progress to account for uninstall.
                 if (removeOperation)
                 {
                     // it is percentage based progress.
-                    progressWeights.insert_or_assign(AppInstaller::ProgressType::Bytes, 0);
+                    progressWeights.push_back(std::make_pair(AppInstaller::ProgressType::Percent, 1.0));
                 }
                 else
                 {
                     // Add/Update operation has two progress types:
                     // 1. Bytes for downloading index and
                     // 2. Percent for index installation.
-                    progressWeights.insert_or_assign(AppInstaller::ProgressType::Bytes, 0.7);
-                    progressWeights.insert_or_assign(AppInstaller::ProgressType::Percent, 0.3);
+                    progressWeights.push_back(std::make_pair(AppInstaller::ProgressType::Bytes, 0.7));
+                    progressWeights.push_back(std::make_pair(AppInstaller::ProgressType::Percent, 0.3));
                 }
 
                 return std::make_shared<PreIndexedPackageCatalogProgressSink>(progressWeights, progressReporter);
@@ -52,30 +52,25 @@ namespace winrt::Microsoft::Management::Deployment
         }
     }
 
-    void CompletionOnlyProgressSink::OnProgress(uint64_t current, uint64_t maximum, AppInstaller::ProgressType type)
+    void CompletionOnlyProgressSink::OnProgress(uint64_t /*current*/, uint64_t /*maximum*/, AppInstaller::ProgressType /*type*/)
     {
-        UNREFERENCED_PARAMETER(current);
-        UNREFERENCED_PARAMETER(maximum);
-        UNREFERENCED_PARAMETER(type);
     }
 
-    void CompletionOnlyProgressSink::SetProgressMessage(std::string_view message)
+    void CompletionOnlyProgressSink::SetProgressMessage(std::string_view /*message*/)
     {
-        UNREFERENCED_PARAMETER(message);
     }
 
     void CompletionOnlyProgressSink::BeginProgress()
     {
         m_progressReporter(0);
     }
 
-    void CompletionOnlyProgressSink::EndProgress(bool hideProgressWhenDone)
+    void CompletionOnlyProgressSink::EndProgress(bool /*hideProgressWhenDone*/)
     {
-        UNREFERENCED_PARAMETER(hideProgressWhenDone);
         m_progressReporter(100);
     }
 
-    PreIndexedPackageCatalogProgressSink::PreIndexedPackageCatalogProgressSink(std::unordered_map<AppInstaller::ProgressType, double> progressWeights, std::function<void(double)> progressReporter) :
+    PreIndexedPackageCatalogProgressSink::PreIndexedPackageCatalogProgressSink(std::vector<std::pair<AppInstaller::ProgressType, double>> progressWeights, std::function<void(double)> progressReporter) :
         m_progressWeights(progressWeights), m_progressReporter(progressReporter)
     {
         if (!m_progressReporter)
@@ -86,7 +81,7 @@ namespace winrt::Microsoft::Management::Deployment
         // If no weights are provided, default to percent.
         if (m_progressWeights.empty())
         {
-            m_progressWeights.insert_or_assign(AppInstaller::ProgressType::Percent, 1.0);
+            m_progressWeights.push_back(std::make_pair(AppInstaller::ProgressType::Percent, 1.0));
         }
 
         // Calculate the total weight.
@@ -117,30 +112,46 @@ namespace winrt::Microsoft::Management::Deployment
         m_progressValues[type] = progress;
 
         double totalProgress = 0.0;
+        double totalWeight = 0.0;
 
         // Calculate the total progress.
         for (const auto& [progressType, weight] : m_progressWeights)
         {
+            double progressValue = m_progressValues[progressType];
+
+            // [NOTE:] Sequential execution assumption & Handling incomplete progress reports :
+            // This progress calculation assumes that each operation is executed sequentially, meaning the download must be complete before
+            // the installation begins.If the download fails, the installation will not proceed.However, there may be cases where the previous
+            // operation completes successfully, but its onprogress callback does not report 100% completion(e.g., the last progress report for
+            // the download was at 90%, but the download is complete, and the installation has started).This can result in the total progress not
+            // reaching 100% after the last operation completes due to the gap in the previous operation's progress report.To handle this, consider
+            // the progress for the last operation as complete by assigning its full weight while computing progress for the following operation.
+            // For example, while computing progress for the installation, consider the download operation complete even if it did not report progress
+            // exactly at 100%.
+            if (progressValue != 0)
+            {
+                totalProgress = totalWeight;
+            }
+
             // Adjust the total progress value based on the weight.
-            totalProgress += m_progressValues[progressType] * weight;
+            totalWeight += weight;
+            totalProgress += progressValue * weight;
         }
 
         m_progressReporter(totalProgress * 100);
     }
 
-    void PreIndexedPackageCatalogProgressSink::SetProgressMessage(std::string_view message)
+    void PreIndexedPackageCatalogProgressSink::SetProgressMessage(std::string_view /*message*/)
     {
-        UNREFERENCED_PARAMETER(message);
     }
 
     void PreIndexedPackageCatalogProgressSink::BeginProgress()
     {
-        OnProgress(0, 1, AppInstaller::ProgressType::None);
+        m_progressReporter(0);
     }
 
-    void PreIndexedPackageCatalogProgressSink::EndProgress(bool hideProgressWhenDone)
+    void PreIndexedPackageCatalogProgressSink::EndProgress(bool /*hideProgressWhenDone*/)
     {
-        UNREFERENCED_PARAMETER(hideProgressWhenDone);
-        OnProgress(1, 1, AppInstaller::ProgressType::None);
+        m_progressReporter(100);
     }
 }

src/Microsoft.Management.Deployment/PackageCatalogProgress.h

@@ -52,7 +52,7 @@ namespace winrt::Microsoft::Management::Deployment
         /// </summary>
         /// <param name="progressWeights">ProgressType weight map.</param>
         /// <param name="progressReporter">Callback function that reports progress to caller.</param>
-        PreIndexedPackageCatalogProgressSink(std::unordered_map<AppInstaller::ProgressType, double> progressWeights, std::function<void(double)> progressReporter);
+        PreIndexedPackageCatalogProgressSink(std::vector<std::pair<AppInstaller::ProgressType, double>> progressWeights, std::function<void(double)> progressReporter);
 
         /// <summary>
         /// Reports combined progress to caller when configured for multiple progress types.
@@ -66,7 +66,7 @@ namespace winrt::Microsoft::Management::Deployment
         void EndProgress(bool hideProgressWhenDone) override;
 
     private:
-        std::unordered_map<AppInstaller::ProgressType, double> m_progressWeights;
+        std::vector<std::pair<AppInstaller::ProgressType, double>> m_progressWeights;
         std::function<void(double)> m_progressReporter;
         std::unordered_map<AppInstaller::ProgressType, double> m_progressValues;
     };

src/Microsoft.Management.Deployment/PackageManager.cpp

@@ -114,29 +114,11 @@ namespace winrt::Microsoft::Management::Deployment::implementation
                 source.SetCustomHeader(customHeader);
             }
 
-            try
-            {
-                auto sourceInfo = source.GetInformation();
+            auto sourceInfo = source.GetInformation();
 
-                if (sourceInfo.Authentication.Type == ::AppInstaller::Authentication::AuthenticationType::Unknown)
-                {
-                    throw winrt::hresult_error(APPINSTALLER_CLI_ERROR_AUTHENTICATION_TYPE_NOT_SUPPORTED);
-                }
-            }
-            catch (const winrt::hresult_error& hre)
-            {
-                if (hre.code() == APPINSTALLER_CLI_ERROR_AUTHENTICATION_TYPE_NOT_SUPPORTED)
-                {
-                    THROW_HR(APPINSTALLER_CLI_ERROR_AUTHENTICATION_TYPE_NOT_SUPPORTED);
-                }
-                else
-                {
-                    THROW_HR(APPINSTALLER_CLI_ERROR_SOURCE_OPEN_FAILED);
-                }
-            }
-            catch (...) // Catch all exceptions
+            if (sourceInfo.Authentication.Type == ::AppInstaller::Authentication::AuthenticationType::Unknown)
             {
-                THROW_HR(APPINSTALLER_CLI_ERROR_SOURCE_OPEN_FAILED);
+                THROW_HR(APPINSTALLER_CLI_ERROR_AUTHENTICATION_TYPE_NOT_SUPPORTED);
             }
 
             return source;
@@ -1314,7 +1296,7 @@ namespace winrt::Microsoft::Management::Deployment::implementation
         LogStartupIfApplicable();
 
         // options must be set.
-        THROW_HR_IF_NULL(E_INVALIDARG, options);
+        THROW_HR_IF_NULL(E_POINTER, options);
         THROW_HR_IF(E_INVALIDARG, options.Name().empty());
         THROW_HR_IF(E_INVALIDARG, options.SourceUri().empty());
 
@@ -1352,7 +1334,7 @@ namespace winrt::Microsoft::Management::Deployment::implementation
         LogStartupIfApplicable();
 
         // options must be set.
-        THROW_HR_IF_NULL(E_INVALIDARG, options);
+        THROW_HR_IF_NULL(E_POINTER, options);
         THROW_HR_IF(E_INVALIDARG, options.Name().empty());
 
         HRESULT terminationHR = S_OK;
@@ -1379,7 +1361,6 @@ namespace winrt::Microsoft::Management::Deployment::implementation
             if (options.PreserveData())
             {
                 THROW_HR_IF(APPINSTALLER_CLI_ERROR_SOURCE_NAME_DOES_NOT_EXIST, !sourceToRemove.DropSource(matchingSource.value().Name));
-                packageCatalogProgressSink->OnProgress(100, 100, AppInstaller::ProgressType::Percent);
             }
             else
             {

src/Microsoft.Management.Deployment/PackageManager.idl

@@ -940,6 +940,11 @@ namespace Microsoft.Management.Deployment
         [contract(Microsoft.Management.Deployment.WindowsPackageManagerContract, 12)]
         {
             /// Updates the package catalog.
+            /// The progress value, represented as a double, indicates the percentage of operation completion.
+            /// For "Microsoft.PreIndexed.Package" Package Catalogs, it reflects the progress of downloading and
+            /// installing the index. When there is no meaningful progress to report for other Package Catalogs,
+            /// the progress will be set to 0% at the start and will be updated to 100% upon the operation's completion.
+            /// The progress range is from 0 to 100.
             Windows.Foundation.IAsyncOperationWithProgress<RefreshPackageCatalogResult, Double> RefreshPackageCatalogAsync();
         }
     }
@@ -1495,9 +1500,18 @@ namespace Microsoft.Management.Deployment
         [contract(Microsoft.Management.Deployment.WindowsPackageManagerContract, 12)]
         {
             /// Add a catalog to the Windows Package Catalogs.
+            /// The progress value, represented as a double, indicates the percentage of operation completion.
+            /// For "Microsoft.PreIndexed.Package" Package Catalogs, it reflects the progress of downloading and
+            /// installing the index. When there is no meaningful progress to report for other Package Catalogs,
+            /// the progress will be set to 0% at the start and will be updated to 100% upon the operation's completion.
+            /// The progress range is from 0 to 100.
             Windows.Foundation.IAsyncOperationWithProgress<AddPackageCatalogResult, Double> AddPackageCatalogAsync(AddPackageCatalogOptions options);
 
             /// Unregisters a Package Catalog from the Windows Package Catalogs and eliminates the system artifacts based on the provided options.
+            /// The progress value, represented as a double, indicates the percentage of operation completion.For "Microsoft.PreIndexed.Package"
+            /// Package Catalogs, it reflects the progress of uninstalling the index. When there is no meaningful progress to report for
+            /// other Package Catalogs, the progress will be set to 0% at the start and will be updated to 100% upon the operation's completion.
+            /// The progress range is from 0 to 100.
             Windows.Foundation.IAsyncOperationWithProgress<RemovePackageCatalogResult, Double> RemovePackageCatalogAsync(RemovePackageCatalogOptions options);
         }
 

src/Microsoft.Management.Deployment/RemovePackageCatalogOptions.h

@@ -19,7 +19,7 @@ namespace winrt::Microsoft::Management::Deployment::implementation
 
 #if !defined(INCLUDE_ONLY_INTERFACE_METHODS)
     private:
-        std::wstring m_name = L"";
+        hstring m_name = L"";
         bool m_preserveData = false;
 #endif
     };