Allow editing font axes in the Settings UI #16104
Conversation
This comment has been minimized.
This comment has been minimized.
|
Some explanation before you review: There is no way for xaml to bind to an This means that we need to manually update the underlying map when the observable vector is changed via the UI, and whenever a new key value pair is added/removed we also need to manually propagate those changes to the map. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment has been minimized.
This comment has been minimized.
| // We cannot simply copy the font axes and features with `fontInfo->_FontAxes = source->_FontAxes;` | ||
| // since that'll just create a reference; we have to manually copy the values. | ||
| if (source->_FontAxes) | ||
| { | ||
| fontInfo->_FontAxes = winrt::single_threaded_map<winrt::hstring, float>(); | ||
| for (const auto keyValuePair : source->_FontAxes.value()) | ||
| { | ||
| fontInfo->_FontAxes.value().Insert(keyValuePair.Key(), keyValuePair.Value()); | ||
| } | ||
| } |
There was a problem hiding this comment.
This makes the MTSM_FONT_SETTINGS macro call above pointless for FontAxes, but I'd still rather not remove FontAxes from that macro since it is relevant in other places
This comment was marked as resolved.
This comment was marked as resolved.
This comment has been minimized.
This comment has been minimized.
|
Okay so I don't know how extra this would be, or what it would even look like, but: is there any way we could add like, descriptions to the axes as you add them? Little old me doesn't know enough about fonts to know what |
…/pabhoj/sui_font_parity
PankajBhojwani
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
| for (const auto tagAndName : _tagToNameMap) | ||
| { | ||
| if (i == _AxisIndex) | ||
| { | ||
| AxisKey(tagAndName.Key()); | ||
| break; | ||
| } | ||
| ++i; | ||
| } |
There was a problem hiding this comment.
This is the manual index to key mapping we have to do because xaml struggles with combo box bindings
| if (!Appearance().AreFontAxesAvailable()) | ||
| { | ||
| // if the previous font had available font axes and the expander was expanded, | ||
| // at this point the expander would be set to disabled so manually collapse it | ||
| FontAxesContainer().SetExpanded(false); | ||
| FontAxesContainer().HelpText(RS_(L"Profile_FontAxesUnavailable/Text")); | ||
| } | ||
| else | ||
| { | ||
| FontAxesContainer().HelpText(RS_(L"Profile_FontAxesAvailable/Text")); | ||
| } |
There was a problem hiding this comment.
This was something extra I added - we could just set the entire expander to invisible if the font does not support font axes, but I figured it would be more consistent for the user if we still show the expander and just set it to disabled instead. If we feel like this is more trouble than its worth I'm happy to switch to the invisible implementation instead.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
PankajBhojwani
added
the
AutoMerge
microsoft-github-policy-service
bot
deleted the
dev/pabhoj/sui_font_parity
branch
## Summary of the Pull Request **Targets #16104** Same as #16104, but for font features ## References and Relevant Issues #10000 ## Validation Steps Performed Font features are detected correctly and can be set in the settings UI   ## PR Checklist - [ ] Closes #xxx - [ ] Tests added/passed - [ ] Documentation updated - If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx - [ ] Schema updated (if necessary)
Make sure the delete button's `Tag` updates when the selected axis/feature changes, so that the correct key value gets propagated when the delete button is clicked. Refs #16678 #16104 ## Validation Steps Performed 1. Add a new feature/axis 2. Change the key 3. Click the delete button 4. Delete button works
Due to #16821 everything about #16104 broke. This PR rights the wrongs by rewriting all the `Font`-based code to not use `Font` at all. Instead we split the font spec once into font families, do a lot of complex logic to split font axes/features into used and unused ones and construct all the UI elements. So. much. boilerplate. code. Closes #16943 ## Validation Steps Performed There are more edge cases than I can list here... Some ideas: * Edit the settings.json with invalid axis/feature keys ✅ * ...out of range values ✅ * Settings UI reloads when the settings.json changes ✅ * Adding axes/features works ✅ * Removing axes/features works ✅ * Resetting axes/features works ✅ * Axes/features apply in the renderer when saving ✅
Summary of the Pull Request
Allow editing of font features and axes in the SUI to get the UI closer towards JSON parity
The allowed font axes are obtained directly from the currently selected font, and their display names are presented to the user in the user's current locale (if it exists). Otherwise, we just display the axis tag to the user.
References and Relevant Issues
#10000
Validation Steps Performed
PR Checklist