针对#issues6178图标修复(概念代码)#6181
Conversation
|
/gemini review |
There was a problem hiding this comment.
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.
| 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); | ||
| } | ||
| } |
There was a problem hiding this comment.
这里存在两个问题:
- 潜在的
NoSuchFileException异常(Bug):当首次为版本设置图标时,target文件尚不存在。在这种情况下直接调用Files.isSameFile(iconFile, target)会因为目标文件不存在而抛出NoSuchFileException异常,导致设置图标功能完全失效。 - 代码重复:删除其他格式图标的代码在
if分支和外部被完全重复编写了两次。
我们可以通过先检查 Files.exists(target) 来避免异常,并合并重复的清理逻辑,使代码更加简洁和健壮。
| 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); | |
| } | |
| } |
|
/gemini review |
There was a problem hiding this comment.
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.
| // 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); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
这里引入了与 deleteIconFile(String id) 几乎完全相同的循环逻辑,造成了代码重复。我们可以通过提取一个私有的辅助方法 deleteIconFileExcept(String id, String excludedExt) 来消除这种重复。\n\n建议将此处的逻辑提取为辅助方法,并建议在后续将 deleteIconFile(String id) 也重构为调用该辅助方法(例如 deleteIconFileExcept(id, null)),以保持代码的整洁和可维护性。
| // 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 } |
修复#issues6178
测试:已完成编译测试,本地通过且功能(大体正常??)
关联:Fixes #6178