Font Install #5042

ryfu-msft · 2024-12-05T21:18:07Z

Related to: #392, #4842

Changes:
Adds Support for installing a font.

Created a FontInstaller that does two things based on the scope. place the file in the fonts directory, and writes the subkey entry in the Fonts Registry based on the title of the file.
Added a IsFontSupported function that calls upon the Dwrite Analyze function to determine if the system can support installing the font. In the flow, if this fails, we error out with no way to override.
Adds a new font install command but currently only supports installing from a local manifest. This means that the same font manifest will work for regular install.

Tests:

Added tests to verify if the installation was successful and if the correct error message is displayed if the font file is invalid.

Microsoft Reviewers: Open in CodeFlow

src/AppInstallerCLICore/Commands/FontCommand.cpp

src/AppInstallerCLICore/FontInstaller.cpp

JohnMcPMS · 2024-12-10T20:09:35Z

src/AppInstallerCLICore/FontInstaller.cpp

+            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);


Is it truly always a 1:1 registry value to file?

Yes I believe so. Whenever I do a manual install of a font file, it creates a new registry value. I haven't seen an example where that hasn't happened yet.

The value names must be unique, so we know that isn't an issue. But that leaves actual question in a more verbose form "Is it possible for multiple values to point to the same file, for instance a .ttc?". Maybe it is acceptable for us to install it as a single entry and the DWrite APIs take care of exposing the contents. But have you installed a TTC via explorer and examined the resulting registry changes?

A TTC also only has exactly one registry value when installed. Having multiple registry values point to the same font file doesn't appear to have any effect on how the font appears in the system.

src/AppInstallerCLICore/Workflows/DownloadFlow.cpp

src/AppInstallerCLICore/Workflows/FontFlow.cpp

src/AppInstallerSharedLib/Errors.cpp

src/AppInstallerCLICore/FontInstaller.cpp

JohnMcPMS · 2024-12-11T22:51:07Z

src/AppInstallerCLICore/FontInstaller.cpp

+            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);


The value names must be unique, so we know that isn't an issue. But that leaves actual question in a more verbose form "Is it possible for multiple values to point to the same file, for instance a .ttc?". Maybe it is acceptable for us to install it as a single entry and the DWrite APIs take care of exposing the contents. But have you installed a TTC via explorer and examined the resulting registry changes?

JohnMcPMS · 2024-12-11T22:54:13Z

src/AppInstallerCLICore/FontInstaller.cpp

@@ -69,6 +85,9 @@ namespace AppInstaller::CLI::Font
            {
                m_key.SetValue(title, destinationPath, REG_SZ);
            }
+
+            AICLI_LOG(CLI, Info, << "Moving font file to: " << destinationPath);
+            AppInstaller::Filesystem::RenameFile(filePath, destinationPath);


Does this handle the case where the target file already exists? If it deletes that file, this is probably bad. We should choose a new name if it does already exist.

I added a check to see if the installation will overwrite an existing font file. If that is true, I show an error message. This is only a temporary solution for now, we need a proper way to bring back those font files that we remove.

JohnMcPMS · 2024-12-11T23:05:56Z

src/AppInstallerCLICore/FontInstaller.cpp

+                if (std::filesystem::exists(existingFontFilePath))
+                {
+                    AICLI_LOG(CLI, Info, << "Removing existing font file at:" << existingFontFilePath);
+                    std::filesystem::remove(existingFontFilePath);


If the operation fails, we cannot revert this. Much like my comment on the rest of the operation being reverted on failure, we want some mechanism to put things back the way they were.

I would think that to make it maximally recoverable, one would need to:

Create a temporary portable tracking database for the file and registry changes (probably requires some amount of addition to the tracked things)

Set up a resume that can leverage the database to revert to the previous state

But I don't think we need to be that extreme (handling full on process termination). A more modest approach would have a vector of IOperationSteps that you populate as you go. On exiting the operation, you either invoke Complete or Revert on all of the items, depending on success or failure. In this case, Complete deletes the renamed file, while Revert renames it back to its old state.

JohnMcPMS · 2024-12-20T17:08:16Z

src/AppInstallerCLICore/FontInstaller.cpp

-            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>()))


Retrieve m_key[fontFile.Title] once rather than in both conditions.

JohnMcPMS · 2024-12-20T17:11:31Z

src/AppInstallerCLICore/FontInstaller.cpp

-                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>() };


You will want to assign the path from a wide string; I think there is a registry value to get that directly.

JohnMcPMS · 2024-12-20T17:14:56Z

src/AppInstallerCLICore/Workflows/FontFlow.cpp


-            for (const auto& file : filePaths)
+            for (std::filesystem::path filePath : filePaths)


Suggested change

for (std::filesystem::path filePath : filePaths)

for (const std::filesystem::path& filePath : filePaths)

Did we need a copy?

JohnMcPMS · 2024-12-20T17:19:56Z

src/AppInstallerSharedLib/Registry.cpp

@@ -132,6 +132,20 @@ namespace AppInstaller::Registry

            return true;
        }
+
+        bool TryDeleteRegistryValueData(const wil::shared_hkey& key, const std::wstring& valueName)


This should probably be called DeleteRegistryValueIfPresent. Try shouldn't throw, even on failure.

ryfu-msft added 4 commits November 26, 2024 16:13

add initial flow for installing a font

aae3caa

initial implementaiton for font installer

42e53bf

add tests

4efa09f

minor fixes

aeab647

This comment has been minimized.

Sign in to view

fix localsource.json and spelling

e73e8af

This comment has been minimized.

Sign in to view

ryfu-msft added 6 commits December 6, 2024 13:01

fix spelling again

8678d12

enable local manifests for fonts e2e tests

745a4ed

try installing font from source

b5f3c16

try again

bc8221f

try again

8f7a47d

this should work

fa1c995

ryfu-msft marked this pull request as ready for review December 10, 2024 17:30

ryfu-msft requested a review from a team as a code owner December 10, 2024 17:30

resolve merge conflicts

9550aed

JohnMcPMS reviewed Dec 10, 2024

View reviewed changes

address PR comments

d21f3ee

JohnMcPMS reviewed Dec 11, 2024

View reviewed changes

add cleanup functionality and better handling of edge scenarios

1491913

JohnMcPMS reviewed Dec 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Font Install #5042

Font Install #5042

ryfu-msft commented Dec 5, 2024 •

edited by microsoft-github-policy-service bot

Loading

This comment has been minimized.

This comment has been minimized.

JohnMcPMS Dec 10, 2024

ryfu-msft Dec 11, 2024

JohnMcPMS Dec 11, 2024

ryfu-msft Dec 19, 2024 •

edited

Loading

JohnMcPMS Dec 11, 2024

JohnMcPMS Dec 11, 2024

ryfu-msft Dec 19, 2024

JohnMcPMS Dec 11, 2024

JohnMcPMS Dec 20, 2024

JohnMcPMS Dec 20, 2024

JohnMcPMS Dec 20, 2024

JohnMcPMS Dec 20, 2024


		for (const auto& file : filePaths)
		for (std::filesystem::path filePath : filePaths)

	for (std::filesystem::path filePath : filePaths)
	for (const std::filesystem::path& filePath : filePaths)

Font Install #5042

Are you sure you want to change the base?

Font Install #5042

Conversation

ryfu-msft commented Dec 5, 2024 • edited by microsoft-github-policy-service bot Loading

Microsoft Reviewers: Open in CodeFlow

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryfu-msft Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryfu-msft commented Dec 5, 2024 •

edited by microsoft-github-policy-service bot

Loading

ryfu-msft Dec 19, 2024 •

edited

Loading