Conversation
Summary of ChangesHello @kazukokawagawa, 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! This pull request focuses on enhancing user customization and improving the application's internal structure. It introduces a more intuitive settings layout with new 'Appearance' and 'Network' categories, allowing users finer control over UI elements like playlist page details and mini-player visuals. Additionally, lyric display is made more flexible with bracket replacement options, and the developer mode activation process is streamlined. Highlights
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://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a major refactoring of the settings pages, organizing them into more logical categories like "Appearance" and "Network". This greatly improves the structure and maintainability of the settings code. Additionally, several new features and improvements have been added, such as customizing the playlist page elements, a more advanced lyric bracket replacement system, and the ability to toggle developer mode. The UX has also been enhanced by making the song title in the mini-player clickable to open the full player when the cover is hidden. Overall, this is a great set of changes that improves both the codebase and the user experience.
| @click.stop=" | ||
| settingStore.hideMiniPlayerCover ? (statusStore.showFullPlayer = true) : null | ||
| " |
There was a problem hiding this comment.
The ternary operator with a null else branch for a conditional action can be simplified for better readability. Using the logical AND (&&) operator as a guard is a more concise and common pattern for this scenario in template expressions.
settingStore.hideMiniPlayerCover && (statusStore.showFullPlayer = true)
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-executed refactoring of the settings pages, making them more organized and user-friendly. The addition of granular controls for hiding UI elements like covers and customizing the playlist page is a great enhancement. The improved logic for handling lyric brackets and ID3 tags also adds to the application's robustness.
I've identified a few areas for improvement, mainly concerning consistency in using the new settings and the complexity of a new function. Overall, this is a solid contribution with substantial improvements.
src/components/List/ListDetail.vue
Outdated
| <Transition name="fade" mode="out-in"> | ||
| <div v-if="detailData" class="detail"> | ||
| <div class="cover"> | ||
| <div class="cover" v-if="!settingStore.hideAllCovers"> |
There was a problem hiding this comment.
This change uses settingStore.hideAllCovers to control the visibility of the cover. However, this PR introduces a new, more granular hiddenCovers setting object, and other list components have been updated to use it (e.g., SongList.vue uses settingStore.hiddenCovers.list). The hideAllCovers setting seems to be a legacy option that is no longer exposed in the new settings UI.
For consistency and to correctly use the new feature, this should be updated to use the new settings object. The key for "歌单详情/歌曲列表" in CoverManager.vue is list.
<div class="cover" v-if="!settingStore.hiddenCovers.list">
| private applyBracketReplacement(lyricData: SongLyric): SongLyric { | ||
| const settingStore = useSettingStore(); | ||
| if (!settingStore.replaceLyricBrackets) { | ||
| return lyricData; | ||
| } | ||
|
|
||
| const newLyricData = cloneDeep(lyricData); | ||
|
|
||
| // Determine replacement strategy | ||
| const preset = settingStore.bracketReplacementPreset || "dash"; | ||
| const custom = settingStore.customBracketReplacement || "-"; | ||
|
|
||
| let startStr = " - "; | ||
| let endStr = " "; | ||
| let isEnclosure = false; | ||
|
|
||
| if (preset === "angleBrackets") { | ||
| startStr = "〔"; | ||
| endStr = "〕"; | ||
| isEnclosure = true; | ||
| } else if (preset === "cornerBrackets") { | ||
| startStr = "「"; | ||
| endStr = "」"; | ||
| isEnclosure = true; | ||
| } else if (preset === "custom") { | ||
| const trimmed = custom.trim(); | ||
| // Heuristic: if length is 2 and not just dashes, treat as pair | ||
| if (trimmed.length === 2 && trimmed[0] !== trimmed[1] && !trimmed.includes("-")) { | ||
| startStr = trimmed[0]; | ||
| endStr = trimmed[1]; | ||
| isEnclosure = true; | ||
| } else { | ||
| startStr = " " + trimmed + " "; | ||
| startStr = startStr.replace(/\s+/g, " "); | ||
| endStr = " "; | ||
| isEnclosure = false; | ||
| } | ||
| } | ||
|
|
||
| const processLines = (lines: LyricLine[] | undefined) => { | ||
| if (!lines) return; | ||
| lines.forEach((line) => { | ||
| // 1. Reconstruct full text to check for full bracket enclosure | ||
| const fullText = line.words.map((w) => w.word).join(""); | ||
|
|
||
| // Check if the line matches ^\s*[\(（][^()（）]*[\)）]\s*$ | ||
| const isFullBracket = /^\s*[\(（][^()（）]*[\)）]\s*$/.test(fullText); | ||
|
|
||
| if (isFullBracket && !isEnclosure) { | ||
| // Remove first ( or （ | ||
| let foundStart = false; | ||
| for (const word of line.words) { | ||
| if (foundStart) break; | ||
| if (/[\(（]/.test(word.word)) { | ||
| word.word = word.word.replace(/[\(（]/, ""); | ||
| foundStart = true; | ||
| } | ||
| } | ||
| // Remove last ) or ） | ||
| let foundEnd = false; | ||
| for (let i = line.words.length - 1; i >= 0; i--) { | ||
| if (foundEnd) break; | ||
| const word = line.words[i]; | ||
| if (/[\)）]/.test(word.word)) { | ||
| const lastIndex = Math.max( | ||
| word.word.lastIndexOf(")"), | ||
| word.word.lastIndexOf("）"), | ||
| ); | ||
| if (lastIndex !== -1) { | ||
| word.word = | ||
| word.word.substring(0, lastIndex) + | ||
| word.word.substring(lastIndex + 1); | ||
| foundEnd = true; | ||
| } | ||
| } | ||
| } | ||
| } else { | ||
| // Normal replacement | ||
| line.words.forEach((word, index) => { | ||
| word.word = word.word.replace(/[\(（]/g, startStr); | ||
|
|
||
| if (isEnclosure) { | ||
| word.word = word.word.replace(/[\)）]/g, endStr); | ||
| } else { | ||
| // Replace ) with endStr OR "" depending on position | ||
| word.word = word.word.replace(/[\)）]/g, (_, offset, string) => { | ||
| // Check if this ) is effectively at the end of the word | ||
| const isAtEnd = offset === string.length - 1; | ||
|
|
||
| if (isAtEnd) { | ||
| // If at end of word, check if it is the last word | ||
| if (index === line.words.length - 1) { | ||
| return ""; // Last word's last char -> remove | ||
| } else { | ||
| return endStr; // Not last word -> separator | ||
| } | ||
| } else { | ||
| // Not at end of word -> separator | ||
| return endStr; | ||
| } | ||
| }); | ||
| } | ||
| }); | ||
|
|
||
| // Cleanup double dashes (only for separator mode with dash) | ||
| if (!isEnclosure && startStr.includes("-")) { | ||
| line.words.forEach((word, index) => { | ||
| // Intra-word cleanup | ||
| word.word = word.word.replace(/(?:\s*-\s*){2,}/g, " - "); | ||
|
|
||
| // Inter-word cleanup | ||
| if (index > 0) { | ||
| const prev = line.words[index - 1]; | ||
| // Check if prev ends with dash and curr starts with dash | ||
| if (/-\s*$/.test(prev.word) && /^\s*-/.test(word.word)) { | ||
| // Remove trailing dash from prev | ||
| prev.word = prev.word.replace(/-\s*$/, ""); | ||
| // Ensure curr starts with " - " | ||
| if (!/^\s*-\s+/.test(word.word)) { | ||
| word.word = " - " + word.word.replace(/^\s*-\s*/, ""); | ||
| } | ||
| } | ||
| } | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| // Process translations and romaji | ||
| const processString = (str: string) => { | ||
| if (!str) return str; | ||
| if (!isEnclosure && /^\s*[\(（][^()（）]*[\)）]\s*$/.test(str)) { | ||
| return str | ||
| .replace(/^\s*[\(（]/, "") | ||
| .replace(/[\)）]\s*$/, "") | ||
| .trim(); | ||
| } else { | ||
| let res = str.replace(/[\(（]/g, startStr); | ||
| if (isEnclosure) { | ||
| res = res.replace(/[\)）]/g, endStr); | ||
| } else { | ||
| res = res | ||
| .replace(/[\)）](?=\s*$)/g, "") // Remove if at end | ||
| .replace(/[\)）]/g, endStr); // Else replace with separator | ||
| if (startStr.includes("-")) { | ||
| res = res.replace(/(?:\s*-\s*){2,}/g, " - "); // Cleanup double dashes | ||
| } | ||
| } | ||
| return res; | ||
| } | ||
| }; | ||
|
|
||
| if (line.translatedLyric) { | ||
| line.translatedLyric = processString(line.translatedLyric); | ||
| } | ||
| if (line.romanLyric) { | ||
| line.romanLyric = processString(line.romanLyric); | ||
| } | ||
| }); | ||
| }; | ||
|
|
||
| processLines(newLyricData.lrcData); | ||
| processLines(newLyricData.yrcData); | ||
|
|
||
| return newLyricData; | ||
| } |
There was a problem hiding this comment.
The new applyBracketReplacement function introduces significant complexity for handling bracket replacements in lyrics. While the feature is great, the implementation, especially for the non-enclosure mode and custom replacements, involves intricate logic and regular expressions that could be hard to maintain and debug.
Consider simplifying the logic if possible. For example, for custom replacements, you could require users to specify pairs explicitly (e.g., () for enclosures, - for separators) instead of using heuristics to detect them.
Adding more detailed comments explaining the different parts of the logic (e.g., the purpose of the lookarounds in regex, the end-of-word checks) would also greatly improve maintainability.
| <n-h2 class="name text-hidden">{{ | ||
| settingStore.hideLyricBrackets | ||
| ? removeBrackets(currentSong.name) | ||
| : currentSong.name | ||
| }}</n-h2> |
There was a problem hiding this comment.
The setting hideLyricBrackets is now being used to remove bracketed content from song and album titles on this page. However, this setting is located in the "Lyrics" section of the settings and its description refers to lyrics. This could be confusing for users.
To improve clarity, consider introducing a more generic setting, like hideBracketedContentInTitles, or renaming the existing setting to reflect its broader scope (e.g., hideBracketedContent). This would make the settings more intuitive.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This is a substantial pull request that refactors the application's settings into more logical categories, such as 'appearance' and 'network'. It also introduces several new features, including granular control over hiding UI elements like song covers, customization of the playlist page, and an advanced lyric bracket replacement system. The changes are extensive but have been implemented consistently and with care across the codebase, improving both maintainability and user customization options. The overall quality of the implementation is high.
| value: computed({ | ||
| get: () => settingStore.customBracketReplacement, | ||
| set: (v) => { | ||
| const trimmed = v.trim(); | ||
| if (v.length === 0 || (trimmed.length > 0 && trimmed.length <= 5)) { | ||
| settingStore.customBracketReplacement = v; | ||
| } | ||
| }, | ||
| }), |
There was a problem hiding this comment.
The validation for customBracketReplacement silently ignores inputs longer than 5 characters. This could be confusing for the user as their changes won't be saved without any feedback. Consider providing immediate feedback, for instance by showing a message, to improve the user experience.
value: computed({
get: () => settingStore.customBracketReplacement,
set: (v) => {
if (v.trim().length > 5) {
window.$message.warning("自定义替换内容不能超过5个字符");
return;
}
settingStore.customBracketReplacement = v;
},
}),
This reverts commit afc7f69.
2 participants
No description provided.