-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Font Install, Uninstall, additional Font List #5566
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This comment has been minimized.
This comment has been minimized.
Fix copypasta error Co-authored-by: Muhammad Danish <mdanishkhdev@gmail.com>
This comment has been minimized.
This comment has been minimized.
Fix argument string consistency Co-authored-by: Muhammad Danish <mdanishkhdev@gmail.com>
This comment has been minimized.
This comment has been minimized.
comment typo Co-authored-by: Kaleb Luedtke <trenlymc@gmail.com>
comment typo Co-authored-by: Kaleb Luedtke <trenlymc@gmail.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still experimental, so it can be the way it is for now, but I don't think enough design thought has gone into how fonts fit into the ecosystem of existing commands / COM. We should be able to manage (enumerate and uninstall) fonts outside of those that have been installed by us. COM callers should be able to as well.
Fonts going into the main installed packages catalog is tricky, mostly because I don't think people want list
to show every single font. But maybe that isn't a big deal. I think that I would make these changes to support that and things built upon it (all of these are toward the goal of supporting fonts, but not all are required as part of "fonts deliverable"):
- Convert
PredefinedInstalledSourceFactory::Filter
to be a set of flags that indicate what to include / behaviors. It would be something like { User, Machine, ARP, MSIX, Fonts, UpdateCache, None=0, All=(-1) }. This is mostly about making the logic easier than it is now. - Consider adding these flags to COM and / or list command to make filtering easier and faster.
- Add all fonts to the installed source, with enough identification and meaning to values to make them usable.
- Add a
show
like command / option tolist
to get additional information for installed items. That can go down different paths depending on the installed item type.font list
could be removed and folded into that.
src/AppInstallerRepositoryCore/Microsoft/PredefinedInstalledSourceFactory.cpp
Show resolved
Hide resolved
|
||
void RemoveAllFontResources(std::filesystem::path filePath) | ||
{ | ||
// The recommended uninstall method of a font is to call RemoveFontResource until it fails, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bumping this again.
src/AppInstallerCommonCore/Fonts.cpp
Outdated
fileInfo.PackageVersion = context.PackageVersion; | ||
fileInfo.InstallerSource = context.InstallerSource; | ||
fileInfo.Scope = context.Scope; | ||
if (title.has_value()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bump.
This comment has been minimized.
This comment has been minimized.
Since i can't reply to the comment about AddFontResource or RemoveFontResource, I will update on that here. I did some extensive testing and investigation of using and not using AddFontResource and RemoveFontResource. Using it was the guidance of font experts but the documentation on learn seems a little off. I verified using Notepad++ and Office Word that these two things need to be used as the code uses them or the Font Resources are not properly added or removed to the current session and the apps cannot detect the new font being available without it. They are used in this code as it was recommended that I use it and is consistent with internal font install behavior. So these methods add and remove the font internally to the current active session and allows apps to use the fonts and detect them immediately. It is not tied to the current app; adding the font resource makes it available to all apps, and removing it removes it from all apps. I did not encounter issues of files-in-use when removing fonts the apps were actively using, this appears to be handled gracefully. Moreover, removing a font while it is being used is handled well by the apps. Word was particularly graceful - adding a font was immediately available to the running app, you could use it, then when the font is removed via WinGet the document updated the text using that font to a similiar font. After re-adding the font via WinGet, Word updated the text back to the desired font. |
Implements a good chunk of #166 Key features and changes:
All of the below are experimental behind the fonts experimental feature. Font install is implemented as a winget-common installer that models most of the font install behavior of the operating system in its key components. It deviates from a normal font install in that the install locations and registry key paths are a little different in order to map fonts to an identifiable version-specific deterministic location. This is so we may more easily identify font packages and remove them and avoid font collisions, torn state, or removing fonts from other packages.
Font List update
Font Install
*Font Uninstall
** Font List*
Workflows
Tested
NOT part of this PR and is likely a future PR
Font upgrade (a new version will be installed side-by-side)
Uninstall of non-packaged fonts. This is in plan, just not in the scope of this PR.
I have signed the Contributor License Agreement.
I have updated the Release Notes.
This pull request is related to an issue.
Microsoft Reviewers: Open in CodeFlow