- Notifications
You must be signed in to change notification settings - Fork33.8k
Applyfont-variation-settings to the suggestion widget (fix #199954)#200000
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Applyfont-variation-settings to the suggestion widget (fix #199954)#200000
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Congrats! You are the 200,000 issue on the VS Code repo! 🎉 |
font-variation-settings to the suggestion widgetfont-variation-settings to the suggestion widget (fixes #199954)font-variation-settings to the suggestion widget (fixes #199954)font-variation-settings to the suggestion widget (fix #199954)Nirmal4G commentedJul 8, 2024
I'm hitting this. It's such a simple fix. Any reason, this is not merged yet? |
Sahilcrossml left a comment
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.
🎯 Overall Assessment
- Recommendation: APPROVE
- Summary: This PR introduces a minor change to apply
font-variation-settingsto the suggestion widget, addressing issue#199954. The change enhances the rendering of the suggestion widget by utilizing variable font settings, which were previously not applied.
📋 Code Changes Analysis
📁 src/vs/editor/contrib/suggest/browser/suggestWidgetRenderer.ts
Changes: 2 additions, 0 deletions
Old vs New Code Comparison:
// Old CodeconstfontFeatureSettings=fontInfo.fontFeatureSettings;// New CodeconstfontVariationSettings=fontInfo.fontVariationSettings;
// Old Codemain.style.fontFeatureSettings=fontFeatureSettings;// New Codemain.style.fontVariationSettings=fontVariationSettings;
Specific Issues Found:
- Line 131: The addition of
const fontVariationSettings = fontInfo.fontVariationSettings;is appropriate and correctly retrieves the variable font settings. - Line 145: The line
main.style.fontVariationSettings = fontVariationSettings;correctly applies the variable font settings to the suggestion widget.
Suggested Fixes: No fixes are necessary as the changes are correct and align with the intended functionality.
🔍 Detailed Findings
Critical Issues (if any)
- None identified.
Moderate Issues (if any)
- None identified.
Minor Suggestions (if any)
- Consider adding a comment above the new lines to explain the purpose of
fontVariationSettingsfor future maintainers. For example:
// Retrieve and apply variable font settings for better typography supportconstfontVariationSettings=fontInfo.fontVariationSettings;main.style.fontVariationSettings=fontVariationSettings;
✅ Positive Aspects
- The change is straightforward and effectively addresses the issue raised in#199954.
- The code is clean and follows existing conventions.
- The addition of variable font settings enhances the visual quality of the suggestion widget.
🚀 Recommendations
- Ensure that the changes are tested across different variable fonts to confirm that the settings are applied correctly and that there are no regressions in the appearance of the suggestion widget.
- Consider documenting the new setting in the project’s documentation to inform users about the support for variable fonts in the suggestion widget.
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.
lgtm, tho I also wonder if suggest should just useICodeEditor#applyFontInfo so that all font tweaks are applied by default
b3bc713 intomicrosoft:mainUh oh!
There was an error while loading.Please reload this page.
siwenwang0803 commentedJun 29, 2025
🤖 Secure-PR-Guard AI Code ReviewAnalysis completed at: 2025-06-28 20:38:55 UTC 🟡 Risk Assessment:MEDIUM
🟢 Low Priority Issues
🔗Powered by Secure-PR-Guard | 🛡️OWASP LLM Top 10 Compliant This is an automated review. Please verify critical security findings manually. |
Hello, this is a very small change, which only modifies one line, to solve the issue raised in#199954.
The background of this PR is that, in my previous#153968, I added a new setting to support variable fonts. However, I recently found that the related setting has not been applied to the code completion list (orsuggestion widget, according to the source code). I have located the bug and raised this PR.