refactor(FontManager): 优化字体的显示转换逻辑 - #917
Conversation
Summary of ChangesHello @MoYingJi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 此拉取请求旨在优化应用程序中字体名称的显示和转换逻辑。通过在获取系统字体时禁用自动引用，并引入一套新的工具函数来处理字体名称的引用和格式化，确保了字体名称在用户界面和内部存储之间的一致性和准确性。这改进了字体选择的用户体验，并使相关代码更加健壮和易于维护。 Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request refactors font display and conversion logic, introducing new utility functions hasPairedQuotes, fontFamilyToDisplay, and fontDisplayToFamily. These changes streamline how font family strings are handled, especially concerning quotes and comma/space separation, and integrate these new functions into the Vue component for single and multi-font selections. Additionally, the getFonts call in ipc-system.ts now uses disableQuoting: true for consistency. The changes improve code readability and maintainability by centralizing font string manipulation.
There was a problem hiding this comment.
Pull request overview
This PR refactors the font display and conversion logic in the FontManager component by extracting inline transformation logic into reusable helper functions and improving the handling of font family strings with quotes.
Changes:
- Extracted font family string transformation logic into three helper functions (
hasPairedQuotes,fontFamilyToDisplay,fontDisplayToFamily) - Replaced
v-model:valuewith explicit:valueand@update:valuebindings for single-font-selection mode, applying conversions to properly handle quoted font names - Unified parameter naming from
valuetovalfor consistency with codebase conventions - Added
disableQuoting: trueoption togetFonts()call to receive font names without quotes from the system
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/components/Modal/Setting/FontManager.vue | Refactored font family string conversion logic into reusable functions; updated template bindings to use explicit conversions for single-font mode; improved quote handling consistency |
| electron/main/ipc/ipc-system.ts | Added disableQuoting: true option to getFonts() call to receive unquoted font names from the system |
| if (s.startsWith('"')) { | ||
| if (s.indexOf('"', 1) === l - 1) return true; | ||
| } else if (s.startsWith("'")) { | ||
| if (s.indexOf("'", 1) === l - 1) return true; | ||
| } |
There was a problem hiding this comment.
The logic for detecting paired quotes has a bug. When checking for the closing quote, indexOf with start position 1 will find the first occurrence after position 0, which could be an escaped quote or a quote in the middle of the string, not necessarily the matching closing quote at the end. For example, "Font with \" quote" would incorrectly match.
The condition should check if the last character matches the first quote character, rather than using indexOf. Consider replacing lines 284-288 with:
const firstChar = s[0];
const lastChar = s[l - 1];
if ((firstChar === '"' || firstChar === "'") && firstChar === lastChar) {
return true;
}This ensures we only match when the opening and closing quotes are the same type and at the expected positions.
| if (s.startsWith('"')) { | |
| if (s.indexOf('"', 1) === l - 1) return true; | |
| } else if (s.startsWith("'")) { | |
| if (s.indexOf("'", 1) === l - 1) return true; | |
| } | |
| const firstChar = s[0]; | |
| const lastChar = s[l - 1]; | |
| if ((firstChar === '"' || firstChar === "'") && firstChar === lastChar) { | |
| return true; | |
| } |
There was a problem hiding this comment.
"Noto Sans", "Noto Sans CJK SC" 会被错误去除为 Noto Sans", "Noto Sans CJK SC，但包含引号的字体...我没见过说实话
| const fontFamilyToArray = (fontFamily: string): string[] => { | ||
| if (!fontFamily) return []; | ||
| const regex = /"([^"]*)"|'([^']*)'|([^,]+)/g; | ||
| const matches = fontFamily.match(regex); | ||
| if (!matches) return []; | ||
|
|
||
| return matches | ||
| .map((s) => { | ||
| // 移除首尾空格 | ||
| s = s.trim(); | ||
| // 移除引号 | ||
| if (s.match(/^"|^'/)) { | ||
| s = s.substring(1, s.length - 1); | ||
| } | ||
| return s.trim(); | ||
| }) | ||
| .filter(Boolean); | ||
| return matches.map(fontFamilyToDisplay).filter(Boolean); | ||
| }; |
There was a problem hiding this comment.
This refactoring changes the quote detection logic from the previous implementation. The old code in fontFamilyToArray used if (s.match(/^"|^'/)) which only checked if the string starts with a quote, without verifying a closing quote exists. The old fontArrayToFamily used /^["'].*["']$/ which checked for quotes at both ends but didn't ensure they were the same type.
The new hasPairedQuotes function attempts to verify that opening and closing quotes match types (both " or both '), which is more correct. However, due to the bug in the hasPairedQuotes implementation (see comment on that function), this may not work as intended.
Once the hasPairedQuotes bug is fixed, this will be an improvement over the old logic, but it's worth noting that it's a behavior change that could affect how existing font family values are processed.
重构了一下代码，同时还修复了这个小问题