-
Notifications
You must be signed in to change notification settings - Fork 8.5k
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
Group font options in the json into a single object #10433
Conversation
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 looks great! Really just want...
- resolution on the "remove 'font' prefix" question
- Testing: fix the roundtrip serialization test (I guarantee you that one is failing; It's a local test)
- Testing: add a test for handling the legacy json format. Just to make sure nobody removes that code sometime in the future.
block for spec review? |
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.
Ah shoot! Don't forget to update the schema and docs!!
I thought we agreed this doesn't need a spec since its just grouping our current font options together into 1 object and doesn't add/change any current functionality (if I'm mistaken I can add it to #10457). |
Thanks for the reminder! Updated schema |
{ | ||
} | ||
|
||
winrt::com_ptr<FontConfig> FontConfig::CopyFontInfo(const winrt::com_ptr<FontConfig> source, const winrt::weak_ref<Profile> sourceProfile) |
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.
Taking those arguments by value will force the reference count to be incremented for the duration of the call. It's probably a good idea to just take source
as a pointer or reference as you aren't storing it elsewhere for later.
Especially because the code around winrt::com_ptr::copy_from
feels a bit non-obvious / roundabout to me.
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.
So this was intentionally done this way to maintain consistency with Profile::CopySettings
and AppearanceConfig::CopyAppearance
and I'd like to keep it this way because of that
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'm convinced that we'll have to start to clean up such code in the future though.
Isn't it quite seldom that you touch all places simultaneously to clean something up? I don't believe there's (in a rhetoric sense) "any better time to do it than now", as such house-keeping tasks will always be unrelated to any actual task by their nature.
Don't forget to update the docs too! |
🎉 Handy links: |
Summary of the Pull Request
Introduces
FontConfig
, an object that isolates font-related settings in our profilesUsers can now define font settings in their json as so:
Backwards compatible with the currently expected way of defining font settings in the json, note however that upon hitting 'Save' in the SUI, these settings will be rewritten to the font-object style in the json (as above).
References
#1790
PR Checklist
Validation Steps Performed
Existing functionality works, new functionality works