Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Font Install #5042
base: master
Are you sure you want to change the base?
Font Install #5042
Changes from 13 commits
aae3caa
42e53bf
4efa09f
aeab647
e73e8af
8678d12
745a4ed
b5f3c16
bc8221f
8f7a47d
fa1c995
9550aed
d21f3ee
1491913
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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:
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 invokeComplete
orRevert
on all of the items, depending on success or failure. In this case,Complete
deletes the renamed file, whileRevert
renames it back to its old state.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.
Is it truly always a 1:1 registry value to file?
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.
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.
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.
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?
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.
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.
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 code has no capacity to clean up on a failure occurring in the middle. The easiest solution might be to reuse the portable database, even if only during the install itself (in memory SQLite). Then an error/exception could trigger a cleanup via the files/registry values written so far.
At a minimum, it would be better to write the registry first so that hopefully the entry shows up in
font list
and the user can "uninstall" it (which would find no file to remove and just remove the registry).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.
I implemented a basic uninstall function that will remove all registry entries and font files in the reverse order if installation happens to fail. So cleaning up files, then registry values.
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.
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.
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.
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.