Add experimental feature for font list command #4886

ryfu-msft · 2024-10-16T16:58:51Z

Related to:

Changes:

Adds experimental feature flag for font
Adds initial winget font command with list subcommand.
Adds functions for retrieving the installed font familes along with face names for each font.
Adds basic unit tests to verify that font enumeration works as expected.

winget font list --family-name "Vivaldi" will display all of the face names for the font family Vivalidi. At this point, this must be an exact match, but eventually this will allow partial search queries once the font index is created (separate PR).

src/AppInstallerCLICore/Commands/FontCommand.cpp

mdanish-kh · 2024-10-16T20:09:44Z

src/AppInstallerCommonCore/Fonts.cpp

+{
+    namespace
+    {
+        std::vector<std::filesystem::path> GetFontFilePaths(const wil::com_ptr<IDWriteFontFace>& fontFace)


Seeing all upper case for returned file paths, can we update the casing for readability?

~ wingetdev font list --family-name "CaskaydiaCove Nerd Font Mono" Face Family Paths ----------------------------------------------------------------------------------------------------------------------------------------------------------- book CaskaydiaCove Nerd Font Mono C:\USERS\<USER>\APPDATA\LOCAL\MICROSOFT\WINDOWS\FONTS\CASKAYDIA COVE NERD FONT COMPLETE MONO.TTF regular CaskaydiaCove Nerd Font Mono C:\USERS\<USER>\APPDATA\LOCAL\MICROSOFT\WINDOWS\FONTS\CASKAYDIA COVE NERD FONT COMPLETE MONO REGULAR.OTF italic CaskaydiaCove Nerd Font Mono C:\USERS\<USER>\APPDATA\LOCAL\MICROSOFT\WINDOWS\FONTS\CASKAYDIA COVE NERD FONT COMPLETE MONO ITALIC.OTF

May also need to consider anonymizing path...

If we are given upper case from the API, I'm not sure it is worth the effort to use the filesystem to correct it back to the original casing.

May also need to consider anonymizing path...

And respecting the AnonymizeDisplayPaths setting

mdanish-kh · 2024-10-16T20:31:56Z

Found myself juggling more than once between winget font / winget fonts. Would it make sense to add an alias fonts for subcommand font? Though aliases are generally shorter and this one isn't so idk...

ryfu-msft · 2024-10-16T21:15:31Z

Found myself juggling more than once between winget font / winget fonts. Would it make sense to add an alias fonts for subcommand font? Though aliases are generally shorter and this one isn't so idk...

@denelon

JohnMcPMS

It would be helpful as a reviewer if the PR description contained some examples of usage so that I could give feedback on the visual representation.

I also want to ensure that the ISource implementation containing installed items is well thought through to avoid the need to have specialized COM interfaces. It doesn't even have to be a SQLite index, but if we can align the font data to the existing model we can save a lot of work exposing it.

The fun thing we could do with a custom ISource is to have the search results QI into these DWrite interfaces.

schemas/JSON/settings/settings.schema.0.2.json

JohnMcPMS · 2024-10-16T21:50:45Z

src/AppInstallerCLICore/Argument.cpp

@@ -198,6 +198,10 @@ namespace AppInstaller::CLI
        case Execution::Args::Type::IgnoreResumeLimit:
            return { type, "ignore-resume-limit"_liv, ArgTypeCategory::None };

+        // Font command
+        case Execution::Args::Type::FamilyName:
+            return { type, "family-name"_liv, ArgTypeCategory::None };


Is "family" enough? Or maybe have "family" as an alias unless you think there is going to be confusion with some future "family-X" argument.

Alternatively, why can't --name just be re-used for consistency with other winget commands? I think the spec in #4842 even calls it --name at line 178

JohnMcPMS · 2024-10-16T22:05:21Z

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

@@ -121,4 +121,10 @@
    <value>Not Localized</value>
    <comment>{Locked} This file is required to establish the basic expected subresource in the resource map. It is not used to facilitate localization with other projects.</comment>
  </data>
+  <data name="FontListCommandShortDescription" xml:space="preserve">


Wrong file.

JohnMcPMS · 2024-10-16T22:17:54Z

src/AppInstallerCommonCore/Fonts.cpp

+            UINT32 fileCount = 0;
+            THROW_IF_FAILED(fontFace->GetFiles(&fileCount, nullptr));
+
+            wil::com_ptr<IDWriteFontFile> fontFiles[8];


static_assert that sizeof(wil::com_ptr<IDWriteFontFile>) == sizeof(IDWriteFontFile*); means that it is a no-overhead smart pointer type and that you can use an array-like of them this way (regardless of the actual array-like type)

vector of wil::com_ptr<IDWriteFontFile>

resize to fileCount

JohnMcPMS · 2024-10-16T22:19:43Z

src/AppInstallerCommonCore/Fonts.cpp

+                    THROW_IF_FAILED(localLoader->GetFilePathLengthFromKey(fontFileReferenceKey, fontFileReferenceKeySize, &pathLength));
+                    pathLength += 1; // Account for the trailing null terminator during allocation.
+
+                    wchar_t path[MAX_PATH];


You have a size already, allocate some memory (preferably directly in a string so that you don't have to copy it).

JohnMcPMS · 2024-10-16T22:26:44Z

src/AppInstallerCommonCore/Fonts.cpp

+            THROW_IF_FAILED(font->GetFaceNames(faceNames.addressof()));
+
+            wchar_t localeNameBuffer[LOCALE_NAME_MAX_LENGTH];
+            const auto localeName = GetUserDefaultLocaleName(localeNameBuffer, LOCALE_NAME_MAX_LENGTH) ? localeNameBuffer : L"en-US";


We already have some locale functionality for choosing installers and settings around this. Prefer those methods over calling this function.

JohnMcPMS · 2024-10-16T22:32:48Z

src/AppInstallerCommonCore/Fonts.cpp

+
+            UINT32 faceNameIndex;
+            BOOL faceNameExists;
+            if (FAILED(faceNames->FindLocaleName(localeName, &faceNameIndex, &faceNameExists)) || !faceNameExists)


It doesn't seem like this chooses a "best" locale to use. We have the option of extracting all of the locale names and sorting them for best fit, much like installer selection. Not saying that we should, but a comment may be warranted.

JohnMcPMS · 2024-10-16T22:35:03Z

src/AppInstallerCommonCore/Fonts.cpp

+            return std::wstring(faceName);
+        }
+
+        std::wstring GetFontFamilyName(const wil::com_ptr<IDWriteFontFamily>& fontFamily)


This is exactly the same function as GetFontFaceName save for the input type and function that is called to get the IDWriteLocalizedStrings. Move all of the "get the correct localized string from an IDWriteLocalizedStrings" to a shared function.

JohnMcPMS · 2024-10-16T22:37:11Z

src/AppInstallerCommonCore/Fonts.cpp

+
+                FontFace fontFaceEntry;
+                fontFaceEntry.Name = std::wstring(faceName);
+                fontFaceEntry.FilePaths = filePaths;


Apply std::move liberally through this type of function so that you aren't making a copy of something you are about to destroy anyway.

JohnMcPMS · 2024-10-16T22:41:04Z

src/AppInstallerCommonCore/Fonts.cpp

+
+        wil::com_ptr<IDWriteFontFamily> family;
+        THROW_IF_FAILED(collection->GetFontFamily(familyIndex, family.addressof()));
+        const auto fontCount = family->GetFontCount();


Factor the "create a FontFamily from a IDWriteFontFamily" into a shared function.

Trenly

I'm not sure the way you've implemented the font families allows for Substring matching. It seems that if --family-name is present, you're expecting an exact match, which conflicts with the --exact argument. I presume the behavior should be the same as winget list where it defaults to a Substring match unless the --exact argument is specified.

Trenly · 2024-10-17T03:08:36Z

src/AppInstallerCLICore/Argument.cpp

@@ -198,6 +198,10 @@ namespace AppInstaller::CLI
        case Execution::Args::Type::IgnoreResumeLimit:
            return { type, "ignore-resume-limit"_liv, ArgTypeCategory::None };

+        // Font command
+        case Execution::Args::Type::FamilyName:
+            return { type, "family-name"_liv, ArgTypeCategory::None };


Alternatively, why can't --name just be re-used for consistency with other winget commands? I think the spec in #4842 even calls it --name at line 178

Trenly · 2024-10-17T03:10:54Z

src/AppInstallerCLICore/Commands/FontCommand.cpp

+            Argument::ForType(Args::Type::Exact),
+            Argument::ForType(Args::Type::AuthenticationMode),
+            Argument::ForType(Args::Type::AuthenticationAccount),
+            Argument::ForType(Args::Type::AcceptSourceAgreements),


-n / --count ?

--scope ?

-q / --query ?

Will FamilyName be the default positional, so if a user typed winget font list Arial it would be the same as winget font list --family-name Arial ?

src/AppInstallerCLICore/Commands/FontCommand.cpp

Trenly · 2024-10-17T03:20:46Z

src/AppInstallerCLICore/Workflows/FontFlow.cpp

+    {
+        struct InstalledFontFamiliesTableLine
+        {
+            InstalledFontFamiliesTableLine(Utility::LocIndString familyName, int faceCount)


Is LocIndString sufficient for the family name and face name? If I recall correctly, it is just a wrapper around std::string, will this appropriately handle family names that contain Unicode characters? Is it even possible for the names to contain Unicode characters?

Trenly · 2024-10-17T03:28:46Z

src/AppInstallerCLICore/Workflows/FontFlow.cpp

+                bool isFirstLine = true;
+                for (const auto& filePath : fontFace.FilePaths)
+                {
+                    if (isFirstLine)


I'm not sure that I like the idea of having table lines that have some fields being empty. While it can be appealing visually, I know that someone out there will want to script against it (even when it makes its way into the PowerShell module).

You never know what someone will do with the output either - they could pipe it to Sort-Object and then it would be really confusing since all the lines would be out of order and they wouldn't contain all the information necessary to stand independently

Trenly · 2024-10-17T03:33:13Z

src/AppInstallerCLICore/Workflows/FontFlow.cpp

+
+    void ReportInstalledFontsResult(Execution::Context& context)
+    {
+        if (context.Args.Contains(Args::Type::FamilyName))


Will there be any provision for searching by Face Name? Not sure if it provides value or not

Trenly · 2024-10-17T03:43:26Z

src/AppInstallerCLITests/Fonts.cpp

+    REQUIRE(installedFontFamilies.size() > 0);
+}
+
+TEST_CASE("GetSingleFontFamily", "[fonts]")


There should also be a test for getting multiple font families by substring, if that's planned for support?

PS C:\Users\jluedtk> [Windows.Media.Fonts]::SystemFontFamilies | Select-Object -Property Source | ? { $_ -match 'Book' } Source ------ Book Antiqua Bookman Old Style Bookshelf Symbol 7 Century Schoolbook Franklin Gothic Book

Trenly · 2024-10-17T03:56:31Z

src/AppInstallerCommonCore/Public/winget/Fonts.h

+    /// </summary>
+    /// <param name="familyName">The font family name.</param>
+    /// <returns>The specified font family if found.</returns>
+    std::optional<FontFamily> GetInstalledFontFamily(const std::wstring& familyName);


Seems a bit odd to me to have both a singular and a pluralized version of effectively the same function, with the only difference being that one takes a parameter to search for. Wouldn't it be possible to simply have GetInstalledFontFamilies with an optional parameter for the search string? Then if no families are found, you have an empty vector, if an exact match is found you'd have a vector with a single FontFamily in it, and if no familyName was specified, you'd have a vector of all the families.

github-actions · 2024-10-18T22:21:18Z

@check-spelling-bot Report

🔴 Please review

See the 📂 files view, the 📜action log, or 📝 job summary for details.

Unrecognized words (1)

Aggregrate

Previously acknowledged words that are now absent

AKV Asn azcopy clsid cobertura notmatch Peet REINSTALLMODE sas SASURL similarissues similaritytolerance templating typeparam 🫥

Some files were automatically ignored 🙈

These sample patterns would exclude them:

^src/AppInstallerCLIE2ETests/TestData/empty$

You should consider adding them to:

.github/actions/spelling/excludes.txt

File matching is via Perl regular expressions.

To check these files, more of their words need to be in the dictionary than not. You can use patterns.txt to exclude portions, add items to the dictionary (e.g. by adding them to allow.txt), or fix typos.

To accept these unrecognized words as correct and remove the previously acknowledged and now absent words and update file exclusions, you could run the following commands

... in a clone of the [email protected]:ryfu-msft/winget-cli.git repository
on the experimentalFont branch (ℹ️ how do I use this?):

curl -s -S -L 'https://raw.githubusercontent.com/check-spelling/check-spelling/v0.0.22/apply.pl' |
perl - 'https://github.com/microsoft/winget-cli/actions/runs/11411808496/attempts/1'

Warnings (1)

See the 📂 files view, the 📜action log, or 📝 job summary for details.

ℹ️ Warnings	Count
ℹ️ binary-file	1

See ℹ️ Event descriptions for more information.

If the flagged items are 🤯 false positives

If items relate to a ...

binary file (or some other file you wouldn't want to check at all).

Please add a file path to the excludes.txt file matching the containing file.

File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).
well-formed pattern.

If you can write a pattern that would match it,
try adding it to the patterns.txt file.

Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.

Note that patterns can't match multiline strings.

ryfu-msft added 9 commits October 7, 2024 14:02

add initial experimental feature

dcb7b7a

add initial fonts command

963d36e

save work

3758808

resolve mrege conflicts

28d5c89

save work

c83e58f

save work and add initial tests

ca9f296

minor fixes

a50d072

update comments

104f465

resolve merge conflicts

7e6e1c7

This comment has been minimized.

Sign in to view

mdanish-kh reviewed Oct 16, 2024

View reviewed changes

ryfu-msft marked this pull request as ready for review October 16, 2024 21:17

ryfu-msft requested a review from a team as a code owner October 16, 2024 21:17

JohnMcPMS requested changes Oct 16, 2024

View reviewed changes

microsoft-github-policy-service bot assigned ryfu-msft Oct 16, 2024

microsoft-github-policy-service bot added the Needs-Author-Feedback Issue needs attention from issue or PR author label Oct 16, 2024

Trenly reviewed Oct 17, 2024

View reviewed changes

ryfu-msft added 2 commits October 17, 2024 20:14

address initial comments

637faa5

save work

9dfbc5d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add experimental feature for font list command #4886

Add experimental feature for font list command #4886

ryfu-msft commented Oct 16, 2024 •

edited

Loading

This comment has been minimized.

mdanish-kh Oct 16, 2024

ryfu-msft Oct 16, 2024

JohnMcPMS Oct 16, 2024

Trenly Oct 17, 2024

mdanish-kh commented Oct 16, 2024

ryfu-msft commented Oct 16, 2024 •

edited

Loading

JohnMcPMS left a comment

JohnMcPMS Oct 16, 2024

Trenly Oct 17, 2024

JohnMcPMS Oct 16, 2024

JohnMcPMS Oct 16, 2024

JohnMcPMS Oct 16, 2024

JohnMcPMS Oct 16, 2024

JohnMcPMS Oct 16, 2024

JohnMcPMS Oct 16, 2024

JohnMcPMS Oct 16, 2024

JohnMcPMS Oct 16, 2024

Trenly left a comment

Trenly Oct 17, 2024

Trenly Oct 17, 2024

Trenly Oct 17, 2024

Trenly Oct 17, 2024

Trenly Oct 17, 2024

Trenly Oct 17, 2024 •

edited

Loading

Trenly Oct 17, 2024

github-actions bot commented Oct 18, 2024

Add experimental feature for font list command #4886

Are you sure you want to change the base?

Add experimental feature for font list command #4886

Conversation

ryfu-msft commented Oct 16, 2024 • edited Loading

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

Choose a reason for hiding this comment

mdanish-kh commented Oct 16, 2024

ryfu-msft commented Oct 16, 2024 • edited Loading

JohnMcPMS left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trenly left a 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

Trenly Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 18, 2024

@check-spelling-bot Report

🔴 Please review

Unrecognized words (1)

ryfu-msft commented Oct 16, 2024 •

edited

Loading

ryfu-msft commented Oct 16, 2024 •

edited

Loading

Trenly Oct 17, 2024 •

edited

Loading