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

src/AppInstallerCommonCore/Fonts.cpp

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

src/AppInstallerCLICore/Argument.cpp

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

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

src/AppInstallerCommonCore/Fonts.cpp

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.

Changed to calling AppInstaller::Locale::GetUserPreferredLanguages() for the default locale.

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.

Added a comment noting that we may need to have better locale selection

src/AppInstallerCommonCore/Fonts.cpp

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.

src/AppInstallerCLICore/Argument.cpp

src/AppInstallerCLICore/Commands/FontCommand.cpp

src/AppInstallerCLICore/Workflows/FontFlow.cpp

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

Yup, will come out in a later PR when I implement a searchable font source.

src/AppInstallerCLITests/Fonts.cpp

src/AppInstallerCommonCore/Public/winget/Fonts.h

… experimentalFont

ryfu-msft · 2024-10-23T22:30:42Z

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.

Once I create a fonts source and implement search functionality, I'll make this --exact argument work as expected.

src/AppInstallerCLICore/Commands/RootCommand.cpp

src/AppInstallerCLICore/Workflows/FontFlow.cpp

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

src/AppInstallerCommonCore/Fonts.cpp

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

JohnMcPMS · 2024-10-25T19:55:47Z

src/AppInstallerCommonCore/Fonts.cpp

-            if (FAILED(localizedStringCollection->FindLocaleName(preferredLocale.c_str(), &index, &exists)) || !exists)
+            for (const auto& locale : preferredLocales)
+            {
+                if (FAILED(localizedStringCollection->FindLocaleName(locale.c_str(), &index, &exists)) || exists)


Suggested change

if (FAILED(localizedStringCollection->FindLocaleName(locale.c_str(), &index, &exists)) || exists)

if (SUCCEEDED_LOG(localizedStringCollection->FindLocaleName(locale.c_str(), &index, &exists)) && exists)

Should we exit the search early if one of the lookups fails? I think we would better off logging and continue trying to find a different one.

Changed to suggested.

JohnMcPMS · 2024-10-25T20:02:23Z

src/AppInstallerCommonCore/Fonts.cpp

@@ -10,6 +10,8 @@ namespace AppInstaller::Fonts
 {
    namespace
    {
+        std::vector<std::wstring> preferredLocales = AppInstaller::Locale::GetUserPreferredLanguagesUTF16();


A global static is not how we should be reducing the number of calls to GetUserPreferredLanguagesUTF16. Global statics bring along several issues, made even worse when they are not statically initialized.

A better approach would be to have the font enumeration functions actually be methods of a type. The caller then controls the lifetime of anything that the font enumeration caches, like a member field of locales.

Created a struct called FontCatalog that hold all of these font enumeration methods as well as a private member field for the preferred locales.

src/AppInstallerCommonCore/Runtime.cpp

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

src/AppInstallerCLICore/Commands/FontCommand.cpp Outdated Show resolved Hide resolved

src/AppInstallerCommonCore/Fonts.cpp Show resolved Hide resolved

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

This comment has been minimized.

Sign in to view

ryfu-msft added 2 commits October 21, 2024 14:46

save work

2c73853

save work

d5cc4b6

microsoft-github-policy-service bot added Needs-Attention Issue needs attention from Microsoft and removed Needs-Author-Feedback Issue needs attention from issue or PR author labels Oct 23, 2024

ryfu-msft added 3 commits October 23, 2024 14:13

Merge branch 'master' of https://github.com/ryfu-msft/winget-cli into…

fb31e73

… experimentalFont

minor fix

0bb6228

refactor Get installed fonts code

3808a10

JohnMcPMS reviewed Oct 24, 2024

View reviewed changes

fix open type version

b1389bb

JohnMcPMS reviewed Oct 25, 2024

View reviewed changes

Trenly reviewed Oct 25, 2024

View reviewed changes

src/AppInstallerCommonCore/Runtime.cpp Show resolved Hide resolved

ryfu-msft added 2 commits October 27, 2024 18:01

address comments

3eaf6a1

resolve merge conflicts

dbae8ac

JohnMcPMS approved these changes Oct 28, 2024

View reviewed changes

ryfu-msft merged commit d60517e into microsoft:master Oct 28, 2024
9 checks passed

ryfu-msft deleted the experimentalFont branch October 28, 2024 21:31

microsoft-github-policy-service bot removed the Needs-Attention Issue needs attention from Microsoft label Oct 28, 2024

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 commented Oct 16, 2024

ryfu-msft commented Oct 16, 2024 •

edited

Loading

JohnMcPMS left a comment

JohnMcPMS Oct 16, 2024

JohnMcPMS Oct 16, 2024

ryfu-msft Oct 23, 2024

JohnMcPMS Oct 16, 2024

ryfu-msft Oct 23, 2024

Trenly left a comment

Trenly Oct 17, 2024

ryfu-msft Oct 23, 2024

This comment has been minimized.

ryfu-msft commented Oct 23, 2024

JohnMcPMS Oct 25, 2024

ryfu-msft Oct 28, 2024

JohnMcPMS Oct 25, 2024

ryfu-msft Oct 28, 2024

	if (FAILED(localizedStringCollection->FindLocaleName(locale.c_str(), &index, &exists)) \|\| exists)
	if (SUCCEEDED_LOG(localizedStringCollection->FindLocaleName(locale.c_str(), &index, &exists)) && exists)

Add experimental feature for font list command #4886

Add experimental feature for font list command #4886

Conversation

ryfu-msft commented Oct 16, 2024 • edited Loading

This comment has been minimized.

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

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

This comment has been minimized.

ryfu-msft commented Oct 23, 2024

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 Oct 16, 2024 •

edited

Loading

ryfu-msft commented Oct 16, 2024 •

edited

Loading