Conversation
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/gemini review |
![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 overhauls the world management system by introducing a WorldLock mechanism for session control and an ImportableWorld class for handling world imports. It adds functionality for restoring backups, renaming world folders, and refactors the UI to better handle read-only states and loading failures. Feedback highlights a logic error in the restoration task that could cause failures after renaming, a potential resource leak in the world import logic, and a regression in the file lock state detection for overlapping locks.
| } catch (OverlappingFileLockException | NoSuchFileException overlappingFileLockException) { | ||
| return false; |
There was a problem hiding this comment.
OverlappingFileLockException 表示当前 JVM 已持有该文件的锁（虽然不是由当前 World 实例持有），因此在这种情况下应该返回 true（已锁定），而不是 false。这在旧版代码中是正确的，此处似乎是重构引入的回归。
| } catch (OverlappingFileLockException | NoSuchFileException overlappingFileLockException) { | |
| return false; | |
| } catch (OverlappingFileLockException overlappingFileLockException) { | |
| return true; | |
| } catch (NoSuchFileException noSuchFileException) { | |
| return false; |
There was a problem hiding this comment.
我觉得这个类的设计就很奇怪，尤其是 ImportableWorld 这个名字我认为非常不恰当。
我认为你可以把它改名叫 WorldInfo 之类的名字，或者修改 World 使其不可变，然后用新的可变类来管理对世界的修改。
There was a problem hiding this comment.
这个不是表达世界信息的类的，而是表达“即将被导入的世界的”，Import：导入，Importable：可导入的，为什么会认为是扩展的意思呢？
这个类的作用就是表示安装世界或在世界备份页面时的静态世界文件的，而World类现在仅表示在世界文件夹，可被世界管理页面管理的类的。
# Conflicts: # HMCL/src/main/java/org/jackhuang/hmcl/ui/versions/WorldManageUIUtils.java
There was a problem hiding this comment.
本审查建议由 Codex (GPT-5.4 xhigh) 生成。
发现 3 个会影响正确性的回归，已分别挂在对应代码位置。
主要问题是：
- 世界锁状态在同一 JVM 的多实例场景下会被误判为未锁定，导致列表页仍可能对已被管理页占用的世界执行破坏性操作。
- 备份恢复使用固定临时目录但恢复前不清理，失败重试后会把旧残留文件混进新的恢复结果。
- 数据包能力判断改成了实例版本而不是世界自身版本，会把不支持数据包的世界误判为支持，并在首次刷新时创建
datapacks/目录。
建议先修这三处，再继续推进其它 UI/交互改动。
| return fileChannel.tryLock() == null; | ||
| } catch (AccessDeniedException accessDeniedException) { | ||
| return true; | ||
| } catch (OverlappingFileLockException | NoSuchFileException overlappingFileLockException) { |
There was a problem hiding this comment.
这里不能把 OverlappingFileLockException 当成未锁定。现在 WorldLock 被移到了 World 实例内部，同一个 HMCL 进程里会同时存在多个指向同一世界的 World 对象，例如世界列表里的对象和 WorldManagePage 里的对象。管理页已经持锁时，列表页这边 tryLock() 抛出这个异常，语义其实是“被本进程中的另一个实例锁住了”，不是 UNLOCKED。当前返回 false 会让列表页继续启用启动、删除、复制、重命名等操作，后续 delete()/copy()/rename() 也会沿着同样的判定继续执行，存在对已打开世界做破坏性操作的风险。
| // Check if the world format is correct | ||
| ImportableWorld importableWorld = ImportableWorld.fromPath(backupZipPath); | ||
| try { | ||
| new Unzipper(backupZipPath, tempPath).setSubDirectory(importableWorld.fileName()).unzip(); |
There was a problem hiding this comment.
这里在解压到固定的 .<world>.tmp 之前没有清理旧目录，而 Unzipper 默认不会覆盖已存在文件。只要上一次恢复在解压后、切换目录前失败过，残留的 .tmp 内容就会混进下一次恢复结果里，用户拿到的就不是备份快照，而是“新备份 + 旧垃圾文件”的混合状态。建议在解压前先删除 tempPath/tempPath2，或者改成每次使用唯一临时目录。
|
|
||
| Optional<String> gameVersion = profile.getRepository().getGameVersion(instanceId); | ||
| currentWorldSupportQuickPlay.set(World.supportsQuickPlay(GameVersionNumber.asGameVersion(gameVersion))); | ||
| currentWorldSupportDataPack.set(World.supportsDataPacks(GameVersionNumber.asGameVersion(gameVersion))); |
There was a problem hiding this comment.
这里改成按 profile.getRepository().getGameVersion(instanceId) 判断数据包能力，会把“实例版本”和“世界自身版本”混在一起。旧逻辑是按 world.getGameVersion() 决定是否显示数据包页；现在如果把一个较老的世界放进较新的实例，页面会被错误开启，而 DataPack.loadFromDir() 在首次刷新时还会直接创建 datapacks/ 目录，等于仅仅打开管理页就修改了一个本不该支持该功能的世界。这里应该继续基于当前世界自身的版本判断。
继续 #5215 对世界管理界面进行的优化第三期
用户可感知的更新
复用页面：
Controllers.getWorldManagePage().setWorld(World world, Profile profile, String instanceId)来获取和设置页面世界锁机制：
World类本身持有锁，而不是在其它类中。World类本身都不知道当前的锁是不是被自己锁定的，导致执行各种操作时需要协调WorldManagePage各种先释放锁再执行最后加锁，尽管某些操作根本不需要释放锁，现在World类本身知道自己是否持有锁，因此可以不再需要操作锁或者可以到内聚到World类中，简化了状态管理。ImportableWorld类：ImportableWorld类，用来表示在外部，即将被安装的世界。可以是压缩包或文件夹。World类仅表示在世界文件夹，以文件夹格式存储的世界。世界备份页面：
重命名/复制世界：
level.dat中的名称，也能修改文件夹名称。其它：
WorldManagePage页面，而是变为只读模式。