Skip to content

针对#issues6178图标修复(概念代码)#6181

Open
2012hzy wants to merge 2 commits into
HMCL-dev:mainfrom
2012hzy:fix-icon-webp-loss
Open

针对#issues6178图标修复(概念代码)#6181
2012hzy wants to merge 2 commits into
HMCL-dev:mainfrom
2012hzy:fix-icon-webp-loss

Conversation

@2012hzy

@2012hzy 2012hzy commented Jun 15, 2026

Copy link
Copy Markdown

修复#issues6178
测试:已完成编译测试,本地通过且功能(大体正常??)
关联Fixes #6178

@2012hzy

2012hzy commented Jun 15, 2026

Copy link
Copy Markdown
Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request modifies setVersionIconFile in HMCLGameRepository.java to copy the new icon file before deleting old ones, preventing source file loss when the source is inside the version directory. The reviewer identified a critical bug where Files.isSameFile(iconFile, target) will throw a NoSuchFileException if the target file does not exist yet, and also noted code duplication. A suggestion was provided to check if the target exists and to consolidate the cleanup logic.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +287 to +312
if (Files.isSameFile(iconFile, target)) {
Path root = getVersionRoot(id);
for (String extension : FXUtils.IMAGE_EXTENSIONS) {
if (extension.equals(ext)) continue;
Path file = root.resolve("icon." + extension);
try {
Files.deleteIfExists(file);
} catch (IOException e) {
LOG.warning("Failed to delete icon file: " + file, e);
}
}
return;
}

FileUtils.copyFile(iconFile, target);

Path root = getVersionRoot(id);
for (String extension : FXUtils.IMAGE_EXTENSIONS) {
if (extension.equals(ext)) continue;
Path file = root.resolve("icon." + extension);
try {
Files.deleteIfExists(file);
} catch (IOException e) {
LOG.warning("Failed to delete icon file: " + file, e);
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

这里存在两个问题:

  1. 潜在的 NoSuchFileException 异常(Bug):当首次为版本设置图标时,target 文件尚不存在。在这种情况下直接调用 Files.isSameFile(iconFile, target) 会因为目标文件不存在而抛出 NoSuchFileException 异常,导致设置图标功能完全失效。
  2. 代码重复:删除其他格式图标的代码在 if 分支和外部被完全重复编写了两次。

我们可以通过先检查 Files.exists(target) 来避免异常,并合并重复的清理逻辑,使代码更加简洁和健壮。

Suggested change
if (Files.isSameFile(iconFile, target)) {
Path root = getVersionRoot(id);
for (String extension : FXUtils.IMAGE_EXTENSIONS) {
if (extension.equals(ext)) continue;
Path file = root.resolve("icon." + extension);
try {
Files.deleteIfExists(file);
} catch (IOException e) {
LOG.warning("Failed to delete icon file: " + file, e);
}
}
return;
}
FileUtils.copyFile(iconFile, target);
Path root = getVersionRoot(id);
for (String extension : FXUtils.IMAGE_EXTENSIONS) {
if (extension.equals(ext)) continue;
Path file = root.resolve("icon." + extension);
try {
Files.deleteIfExists(file);
} catch (IOException e) {
LOG.warning("Failed to delete icon file: " + file, e);
}
}
if (!Files.exists(target) || !Files.isSameFile(iconFile, target)) {
FileUtils.copyFile(iconFile, target);
}
Path root = getVersionRoot(id);
for (String extension : FXUtils.IMAGE_EXTENSIONS) {
if (extension.equals(ext)) continue;
Path file = root.resolve("icon." + extension);
try {
Files.deleteIfExists(file);
} catch (IOException e) {
LOG.warning("Failed to delete icon file: " + file, e);
}
}

@2012hzy

2012hzy commented Jun 15, 2026

Copy link
Copy Markdown
Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors setVersionIconFile in HMCLGameRepository.java to copy the new icon file before deleting other existing icon files, avoiding potential NoSuchFileException issues. The reviewer suggested extracting the logic for deleting other icon extensions into a private helper method, deleteIconFileExcept, to prevent code duplication and improve overall maintainability.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +292 to 303
// Remove all icons with other extensions
Path root = getVersionRoot(id);
for (String extension : FXUtils.IMAGE_EXTENSIONS) {
if (extension.equals(ext)) continue;
Path file = root.resolve("icon." + extension);
try {
Files.deleteIfExists(file);
} catch (IOException e) {
LOG.warning("Failed to delete icon file: " + file, e);
}
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

这里引入了与 deleteIconFile(String id) 几乎完全相同的循环逻辑,造成了代码重复。我们可以通过提取一个私有的辅助方法 deleteIconFileExcept(String id, String excludedExt) 来消除这种重复。\n\n建议将此处的逻辑提取为辅助方法,并建议在后续将 deleteIconFile(String id) 也重构为调用该辅助方法(例如 deleteIconFileExcept(id, null)),以保持代码的整洁和可维护性。

Suggested change
// Remove all icons with other extensions
Path root = getVersionRoot(id);
for (String extension : FXUtils.IMAGE_EXTENSIONS) {
if (extension.equals(ext)) continue;
Path file = root.resolve("icon." + extension);
try {
Files.deleteIfExists(file);
} catch (IOException e) {
LOG.warning("Failed to delete icon file: " + file, e);
}
}
}
// Remove all icons with other extensions\n deleteIconFileExcept(id, ext);\n }\n\n private void deleteIconFileExcept(String id, String excludedExt) {\n Path root = getVersionRoot(id);\n for (String extension : FXUtils.IMAGE_EXTENSIONS) {\n if (extension.equals(excludedExt)) continue;\n Path file = root.resolve(\"icon.\" + extension);\n try {\n Files.deleteIfExists(file);\n } catch (IOException e) {\n LOG.warning(\"Failed to delete icon file: \" + file, e);\n }\n }\n }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] 添加游戏图标功能, 选择icon.webp会导致文件丢失

1 participant