add cleanup functionality and better handling of edge scenarios

src/AppInstallerCLICore/Commands/FontCommand.cpp

@@ -49,6 +49,7 @@ namespace AppInstaller::CLI
     {
         return {
             Argument::ForType(Args::Type::Manifest),
+            Argument{ Args::Type::InstallScope, Resource::String::InstallScopeDescription, ArgumentType::Standard, Argument::Visibility::Help },
         };
     }
 
@@ -66,7 +67,6 @@ namespace AppInstaller::CLI
     {
         UNREFERENCED_PARAMETER(valueType);
         context.Reporter.Error() << Resource::String::PendingWorkError << std::endl;
-        THROW_HR(E_NOTIMPL);
     }
 
     Utility::LocIndView FontInstallCommand::HelpLink() const

src/AppInstallerCLICore/FontInstaller.cpp

@@ -26,6 +26,17 @@ namespace AppInstaller::CLI::Font
         }
     }
 
+    FontFile::FontFile(std::filesystem::path filePath, DWRITE_FONT_FILE_TYPE fileType)
+        : FilePath(std::move(filePath)), FileType(fileType)
+    {
+        Title = AppInstaller::Fonts::GetFontFileTitle(FilePath);
+
+        if (IsTrueTypeFont(FileType))
+        {
+            Title += s_TrueType;
+        }
+    }
+
     FontInstaller::FontInstaller(Manifest::ScopeEnum scope) : m_scope(scope)
     {
         if (scope == Manifest::ScopeEnum::Machine)
@@ -40,28 +51,78 @@ namespace AppInstaller::CLI::Font
         }
     }
 
-    void FontInstaller::Install(const std::vector<FontFile>& fontFiles)
+    bool FontInstaller::EnsureInstall()
     {
-        for (const auto& fontFile : fontFiles)
+        for (auto& fontFile : m_fontFiles)
         {
-            const auto& filePath = fontFile.FilePath;
-            const auto& fileName = filePath.filename();
-            const auto& destinationPath = m_installLocation / fileName;
+            if (m_key[fontFile.Title].has_value())
+            {
+                if (!std::filesystem::exists(m_key[fontFile.Title]->GetValue<Registry::Value::Type::String>()))
+                {
+                    AICLI_LOG(CLI, Info, << "Removing existing font value as font file does not exist.");
+                    m_key.DeleteValue(fontFile.Title);
+                }
+                else
+                {
+                    AICLI_LOG(CLI, Info, << "Existing font value found: " << AppInstaller::Utility::ConvertToUTF8(fontFile.Title));
+                    return false;
+                }
+            }
+
+            std::filesystem::path destinationPath = m_installLocation / fontFile.FilePath.filename();
+            auto initialStem = fontFile.FilePath.stem();
+            auto extension = fontFile.FilePath.extension();
+
+            // If a file exists at the destination path, make the filename unique.
+            int index = 0;
+            while (std::filesystem::exists(destinationPath))
+            {
+                std::filesystem::path unique = { "_" + std::to_string(index) };
+                auto duplicateStem = initialStem;
+                duplicateStem += unique;
+                duplicateStem += extension;
+                destinationPath = m_installLocation / duplicateStem;
+                index++;
+            }
 
-            AICLI_LOG(CLI, Verbose, << "Getting Font title");
+            fontFile.DestinationPath = std::move(destinationPath);
+        }
 
-            std::wstring title = AppInstaller::Fonts::GetFontFileTitle(filePath);
+        return true;
+    }
+
+    void FontInstaller::Install()
+    {
+        bool isMachineScope = m_scope == Manifest::ScopeEnum::Machine;
 
-            if (IsTrueTypeFont(fontFile.FileType))
+        for (const auto& fontFile : m_fontFiles)
+        {
+            AICLI_LOG(CLI, Info, << "Creating font value with name : " << AppInstaller::Utility::ConvertToUTF8(fontFile.Title));
+            if (isMachineScope)
             {
-                title += s_TrueType;
+                m_key.SetValue(fontFile.Title, fontFile.DestinationPath.filename(), REG_SZ);
             }
+            else
+            {
+                m_key.SetValue(fontFile.Title, fontFile.DestinationPath, REG_SZ);
+            }
+        }
 
-            // If font subkey already exists, remove the font file if it exists.
-            if (m_key[title].has_value())
+        for (const auto& fontFile : m_fontFiles)
+        {
+            AICLI_LOG(CLI, Info, << "Moving font file to: " << fontFile.DestinationPath);
+            AppInstaller::Filesystem::RenameFile(fontFile.FilePath, fontFile.DestinationPath);
+        }
+    }
+
+    void FontInstaller::Uninstall()
+    {
+        for (const auto& fontFile : m_fontFiles)
+        {
+            if (m_key[fontFile.Title].has_value())
             {
-                AICLI_LOG(CLI, Info, << "Existing font subkey found:" << AppInstaller::Utility::ConvertToUTF8(title));
-                std::filesystem::path existingFontFilePath = { m_key[title]->GetValue<Registry::Value::Type::String>() };
+                AICLI_LOG(CLI, Info, << "Existing font value found:" << AppInstaller::Utility::ConvertToUTF8(fontFile.Title));
+                std::filesystem::path existingFontFilePath = { m_key[fontFile.Title]->GetValue<Registry::Value::Type::String>() };
 
                 if (m_scope == Manifest::ScopeEnum::Machine)
                 {
@@ -74,25 +135,10 @@ namespace AppInstaller::CLI::Font
                     AICLI_LOG(CLI, Info, << "Removing existing font file at:" << existingFontFilePath);
                     std::filesystem::remove(existingFontFilePath);
                 }
-            }
 
-            AICLI_LOG(CLI, Info, << "Creating font subkey with name: " << AppInstaller::Utility::ConvertToUTF8(title));
-            if (m_scope == Manifest::ScopeEnum::Machine)
-            {
-                m_key.SetValue(title, fileName, REG_SZ);
-            }
-            else
-            {
-                m_key.SetValue(title, destinationPath, REG_SZ);
+                AICLI_LOG(CLI, Info, << "Deleting registry value:" << existingFontFilePath);
+                m_key.DeleteValue(fontFile.Title);
             }
-
-            AICLI_LOG(CLI, Info, << "Moving font file to: " << destinationPath);
-            AppInstaller::Filesystem::RenameFile(filePath, destinationPath);
         }
     }
-
-    void FontInstaller::Uninstall(const std::wstring& familyName)
-    {
-        UNREFERENCED_PARAMETER(familyName);
-    }
 }

src/AppInstallerCLICore/FontInstaller.h

@@ -8,11 +8,12 @@ namespace AppInstaller::CLI::Font
 {
     struct FontFile
     {
-        FontFile(std::filesystem::path filePath, DWRITE_FONT_FILE_TYPE fileType)
-            : FilePath(std::move(filePath)), FileType(fileType) {}
+        FontFile(std::filesystem::path filePath, DWRITE_FONT_FILE_TYPE fileType);
 
         std::filesystem::path FilePath;
         DWRITE_FONT_FILE_TYPE FileType;
+        std::wstring Title;
+        std::filesystem::path DestinationPath;
     };
 
     struct FontInstaller
@@ -21,13 +22,23 @@ namespace AppInstaller::CLI::Font
 
         std::filesystem::path FontFileLocation;
 
-        void Install(const std::vector<FontFile>& fontFiles);
+        void SetFontFiles(const std::vector<FontFile>& fontFiles)
+        {
+            m_fontFiles = fontFiles;
+        }
 
-        void Uninstall(const std::wstring& familyName);
+        // Checks if all expected registry values and font files can be installed prior to installation.
+        bool EnsureInstall();
+
+        void Install();
+
+        void Uninstall();
 
     private:
         Manifest::ScopeEnum m_scope;
         std::filesystem::path m_installLocation;
         Registry::Key m_key;
+
+        std::vector<FontFile> m_fontFiles;
     };
 }

src/AppInstallerCLICore/Resources.h

@@ -250,6 +250,7 @@ namespace AppInstaller::CLI::Resource
         WINGET_DEFINE_RESOURCE_STRINGID(FileNotFound);
         WINGET_DEFINE_RESOURCE_STRINGID(FilesRemainInInstallDirectory);
         WINGET_DEFINE_RESOURCE_STRINGID(FlagContainAdjoinedError);
+        WINGET_DEFINE_RESOURCE_STRINGID(FontAlreadyInstalled);
         WINGET_DEFINE_RESOURCE_STRINGID(FontCommandLongDescription);
         WINGET_DEFINE_RESOURCE_STRINGID(FontCommandShortDescription);
         WINGET_DEFINE_RESOURCE_STRINGID(FontFace);
@@ -260,6 +261,7 @@ namespace AppInstaller::CLI::Resource
         WINGET_DEFINE_RESOURCE_STRINGID(FontFilePaths);
         WINGET_DEFINE_RESOURCE_STRINGID(FontInstallCommandLongDescription);
         WINGET_DEFINE_RESOURCE_STRINGID(FontInstallCommandShortDescription);
+        WINGET_DEFINE_RESOURCE_STRINGID(FontInstallFailed);
         WINGET_DEFINE_RESOURCE_STRINGID(FontListCommandLongDescription);
         WINGET_DEFINE_RESOURCE_STRINGID(FontListCommandShortDescription);
         WINGET_DEFINE_RESOURCE_STRINGID(FontVersion);

src/AppInstallerCLICore/Workflows/FontFlow.cpp

@@ -126,6 +126,16 @@ namespace AppInstaller::CLI::Workflow
 
     void FontInstallImpl(Execution::Context& context)
     {
+        Manifest::ScopeEnum scope = Manifest::ScopeEnum::Unknown;
+        if (context.Args.Contains(Execution::Args::Type::InstallScope))
+        {
+            scope = Manifest::ConvertToScopeEnum(context.Args.GetArg(Execution::Args::Type::InstallScope));
+        }
+
+        FontInstaller fontInstaller = FontInstaller(scope);
+
+        context.Reporter.Info() << Resource::String::InstallFlowStartingPackageInstall << std::endl;
+
         try
         {
             const auto& installerPath = context.Get<Execution::Data::InstallerPath>();
@@ -145,36 +155,42 @@ namespace AppInstaller::CLI::Workflow
                 filePaths.emplace_back(installerPath);
             }
 
-            std::vector<FontFile> fontFiles;
             Fonts::FontCatalog fontCatalog;
+            std::vector<FontFile> fontFiles;
 
-            for (const auto& file : filePaths)
+            for (std::filesystem::path filePath : filePaths)
             {
                 DWRITE_FONT_FILE_TYPE fileType;
-                if (!fontCatalog.IsFontFileSupported(file, fileType))
+                if (!fontCatalog.IsFontFileSupported(filePath, fileType))
                 {
-                    AICLI_LOG(CLI, Warning, << "Font file is not supported: " << file);
+                    AICLI_LOG(CLI, Warning, << "Font file is not supported: " << filePath);
                     context.Reporter.Error() << Resource::String::FontFileNotSupported << std::endl;
                     AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_FONT_FILE_NOT_SUPPORTED);
                 }
                 else
                 {
-                    AICLI_LOG(CLI, Verbose, << "Font file is supported: " << file);
-                    fontFiles.emplace_back(FontFile(file, fileType));
+                    AICLI_LOG(CLI, Verbose, << "Font file is supported: " << filePath);
+                    fontFiles.emplace_back(FontFile(filePath, fileType));
                 }
             }
 
-            context.Reporter.Info() << Resource::String::InstallFlowStartingPackageInstall << std::endl;
+            fontInstaller.SetFontFiles(fontFiles);
 
-            // TODO: Default to installing to HKEY_LOCAL_MACHINE registry as user install is not yet fully supported. 
-            FontInstaller fontInstaller = FontInstaller(Manifest::ScopeEnum::Machine);
+            if (!fontInstaller.EnsureInstall())
+            {
+                context.Reporter.Warn() << Resource::String::FontAlreadyInstalled << std::endl;
+                AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_FONT_ALREADY_INSTALLED);
+            }
 
-            fontInstaller.Install(fontFiles);
+            fontInstaller.Install();
             context.Add<Execution::Data::OperationReturnCode>(S_OK);
         }
         catch (...)
         {
             context.Add<Execution::Data::OperationReturnCode>(Workflow::HandleException(context, std::current_exception()));
+            context.Reporter.Warn() << Resource::String::FontInstallFailed << std::endl;
+            fontInstaller.Uninstall();
+            AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_PORTABLE_INSTALL_FAILED);
         }
     }
 }

src/AppInstallerCLICore/Workflows/InstallFlow.cpp

@@ -285,7 +285,6 @@ namespace AppInstaller::CLI::Workflow
         void FontInstall(Execution::Context& context)
         {
             context <<
-                EnsureRunningAsAdmin <<
                 FontInstallImpl <<
                 ReportInstallerResult("Font"sv, APPINSTALLER_CLI_ERROR_FONT_INSTALL_FAILED, true);
         }

src/AppInstallerCLIE2ETests/FontCommand.cs

@@ -24,28 +24,27 @@ public void OneTimeSetup()
         }
 
         /// <summary>
-        /// Test install a font with machine scope.
+        /// Test install a font with user scope.
         /// </summary>
         [Test]
-        public void InstallFont_MachineScope()
+        public void InstallFont_UserScope()
         {
             var result = TestCommon.RunAICLICommand("install", "AppInstallerTest.TestFont");
             Assert.AreEqual(Constants.ErrorCode.S_OK, result.ExitCode);
             Assert.True(result.StdOut.Contains("Successfully installed"));
-            TestCommon.VerifyFontPackage(Constants.TestFontSubKeyName, Constants.FontFileName, TestCommon.Scope.Machine);
+            TestCommon.VerifyFontPackage(Constants.TestFontSubKeyName, Constants.FontFileName, TestCommon.Scope.User);
         }
 
         /// <summary>
-        /// Test install a font with user scope.
+        /// Test install a font with machine scope.
         /// </summary>
-        // [Test]
-        public void InstallFont_UserScope()
+        [Test]
+        public void InstallFont_MachineScope()
         {
-            // TODO: Enable this test once user scope installs are fully supported.
-            var result = TestCommon.RunAICLICommand("install", "AppInstallerTest.TestFont --scope User");
+            var result = TestCommon.RunAICLICommand("install", "AppInstallerTest.TestFont --scope Machine");
             Assert.AreEqual(Constants.ErrorCode.S_OK, result.ExitCode);
             Assert.True(result.StdOut.Contains("Successfully installed"));
-            TestCommon.VerifyFontPackage(Constants.TestFontSubKeyName, Constants.FontFileName, TestCommon.Scope.User);
+            TestCommon.VerifyFontPackage(Constants.TestFontSubKeyName, Constants.FontFileName, TestCommon.Scope.Machine);
         }
 
         /// <summary>

src/AppInstallerCLIE2ETests/Helpers/TestCommon.cs

@@ -371,7 +371,7 @@ public static string GetFontsDirectory(Scope scope)
         public static void VerifyFontPackage(
             string fontSubKeyName,
             string fontFileName,
-            Scope scope = Scope.Machine,
+            Scope scope = Scope.User,
             bool shouldExist = true)
         {
             // Expected font file path.

src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw

@@ -3196,4 +3196,10 @@ Please specify one of them using the --source option to proceed.</value>
   <data name="FontFileNotSupported" xml:space="preserve">
     <value>The font file is not supported and cannot be installed.</value>
   </data>
+  <data name="FontInstallFailed" xml:space="preserve">
+    <value>Font install failed; Cleaning up...</value>
+  </data>
+  <data name="FontAlreadyInstalled" xml:space="preserve">
+    <value>Font already installed</value>
+  </data>
 </root>

src/AppInstallerSharedLib/Errors.cpp

@@ -224,6 +224,7 @@ namespace AppInstaller
             WINGET_HRESULT_INFO(APPINSTALLER_CLI_ERROR_INSTALLER_ZERO_BYTE_FILE, "Downloaded zero byte installer; ensure that your network connection is working properly."),
             WINGET_HRESULT_INFO(APPINSTALLER_CLI_ERROR_FONT_INSTALL_FAILED, "Failed to install font package."),
             WINGET_HRESULT_INFO(APPINSTALLER_CLI_ERROR_FONT_FILE_NOT_SUPPORTED, "Font file is not supported and cannot be installed."),
+            WINGET_HRESULT_INFO(APPINSTALLER_CLI_ERROR_FONT_ALREADY_INSTALLED, "Font already installed."),
 
             // Install errors.
             WINGET_HRESULT_INFO(APPINSTALLER_CLI_ERROR_INSTALL_PACKAGE_IN_USE, "Application is currently running. Exit the application then try again."),

src/AppInstallerSharedLib/Public/AppInstallerErrors.h

@@ -154,6 +154,7 @@
 #define APPINSTALLER_CLI_ERROR_INSTALLER_ZERO_BYTE_FILE             ((HRESULT)0x8A150086)
 #define APPINSTALLER_CLI_ERROR_FONT_INSTALL_FAILED                  ((HRESULT)0x8A150087)
 #define APPINSTALLER_CLI_ERROR_FONT_FILE_NOT_SUPPORTED              ((HRESULT)0x8A150088)
+#define APPINSTALLER_CLI_ERROR_FONT_ALREADY_INSTALLED               ((HRESULT)0x8A150089)
 
 // Install errors.
 #define APPINSTALLER_CLI_ERROR_INSTALL_PACKAGE_IN_USE                   ((HRESULT)0x8A150101)

src/AppInstallerSharedLib/Public/winget/Registry.h

@@ -271,6 +271,9 @@ namespace AppInstaller::Registry
         std::optional<Value> operator[](std::string_view name) const;
         std::optional<Value> operator[](const std::wstring& name) const;
 
+        void DeleteValue(std::string_view name) const;
+        void DeleteValue(const std::wstring& name) const;
+
         std::optional<Key> SubKey(std::string_view name, DWORD options = 0) const;
         std::optional<Key> SubKey(const std::wstring& name, DWORD options = 0) const;