添加安装阶段对整合包可选文件的支持 - #1771
Conversation
|
Merge Conflict 了，记得改一改 |
|
ping @CaveNightingale |
|
人已经 9 个月不见了，大概是跑了（ |
好的 |
|
《跑了》 |
# Conflicts: # HMCL/src/main/java/org/jackhuang/hmcl/ui/download/LocalModpackPage.java # HMCL/src/main/java/org/jackhuang/hmcl/ui/download/RemoteModpackPage.java # HMCL/src/main/resources/assets/fxml/download/modpack.fxml
|
完成 |
|
@huanghongxun 这个已经一年没合了 |
功能请求要不加个全选（或全不选）按钮？ |
Curse 整合包导入测试注：未测试安装 RLCraft 1.12.2 - Release v2.9.3(改造版).zip HMCL-3.5.SNAPSHOT(需要将zip后缀改为jar).zip 问题：若网络环境不佳，Curse 整合包下面的文件名显示是直接为空白，我觉得这个文件名获取可能会有点问题？ |
Modrinth 整合包导入测试注：未测试安装 Cobblemon Modpack [Fabric] 1.4.1(改造版).zip HMCL-3.5.SNAPSHOT(需要将zip后缀改为jar).zip 建议：我发现下方的文件选择没标题可能会让用户不知道是什么东西，我建议在上面加个标题 |
Modrinth 整合包安装测试
|
|
请将加载整合包文件的 Task 显示至屏幕上，并以并发操作 |
我觉得不太行，可能会影响操作流畅性 |
那就添加一个 Spinner，让用户感知到HMCL 正在加载 |
|
此外，建议把下面的仅文件名改为模组下载界面的 UI 风格，即，可以点进去查看详情 |
|
@CaveNightingale ping |
好 |
他看错了，所以不需要这样了 |
|
主要还是这一条：
和：
建议改成：每一个可选模组为一个 TwoLineListItem，可参考模组下载界面，如果失败，则显示“失败，点击重试” |
|
我认为并不认同 如果有模组名称获取失败就重试几遍（5遍就够了），还是不行就直接贴一个 然后这个可选模组页面，加一个全选（全不选）按钮，然后加个 |
如果用户希望具体查看该可选模组的详细信息，那就需要改为我所提的这种风格了 |
|
这样会不会增加复杂度啊 |
|
|
虽然但是，好像整合包文件里本来就没有模组的详细信息啊？ |
如果移动到单独界面呢？即：
用户只看一个模组的文件名，是无法考虑要还是不要这个模组的，确实需要展示模组详细信息 加油 |
|
By the way，请问你这里的 RT 是指？
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
ping 该 pr 的状态是？ |
6c98e1e to
3bb5da6
Compare
之前在上海实习搞忘了，我现在处理一下。 |
83199f4 to
de8a7ff
Compare
de8a7ff to
23e4380
Compare
|
@zkitefly 我改完了，看看？ |
# Conflicts: # HMCL/src/main/java/org/jackhuang/hmcl/ui/download/ModpackPage.java # HMCLCore/src/main/java/org/jackhuang/hmcl/mod/curse/CurseCompletionTask.java # HMCLCore/src/main/java/org/jackhuang/hmcl/mod/curse/CurseInstallTask.java # HMCLCore/src/main/java/org/jackhuang/hmcl/mod/curse/CurseManifest.java # HMCLCore/src/main/java/org/jackhuang/hmcl/mod/curse/CurseManifestFile.java # HMCLCore/src/main/java/org/jackhuang/hmcl/mod/curse/CurseModpackProvider.java # HMCLCore/src/main/java/org/jackhuang/hmcl/mod/modrinth/ModrinthManifest.java
Glavo
left a comment
Glavo
left a comment
There was a problem hiding this comment.
- 没有可选文件时不应当显示可选文件按钮
- 切换到可选文件页面应该使用渐变过渡
- 这个页面中心的框都顶满高度了，不应该这样
| .collect(Collectors.toList())); | ||
| JsonUtils.writeToJsonFile(root.resolve("manifest.json"), newManifest); | ||
| if (selectedFiles != null) { | ||
| JsonUtils.writeToJsonFile(root.resolve("files.json"), selectedFiles.stream().map(ModpackFile::getPath).collect(Collectors.toList())); |
There was a problem hiding this comment.
ModpackFile.fileName 似乎经常是 null，这样可能写出去一片 mods/null。
| }) | ||
| .whenComplete(Schedulers.javafx(), (manifest, exception) -> { | ||
| this.manifest = manifest; | ||
| if (manifest.getManifest() instanceof ModpackManifest.SupportOptional) { |
渐变过渡是指什么效果？ |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.github.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for optional files during modpack installation and updates, specifically for CurseForge and Modrinth providers. It adds a new selection UI via OptionalFilesPage, defines a ModpackFile interface, and updates the core installation logic to respect user-selected files. Feedback highlights several critical issues: the conversion of CurseManifestFile from a record to a class requires manual implementation of equals and hashCode to prevent broken filtering logic, and multiple asynchronous callbacks in LocalModpackPage are susceptible to NullPointerException if manifest objects are null. Additionally, the reviewer recommended removing redundant UI labels and getter methods, and cautioned against performing blocking network operations within a parallel stream in CurseModpackProvider.
| public final class CurseManifestFile implements Validation, ModpackFile { | ||
|
|
||
| @SerializedName("projectID") | ||
| private final int projectID; | ||
|
|
||
| @SerializedName("fileID") | ||
| private final int fileID; | ||
|
|
||
| @SerializedName("fileName") | ||
| private final String fileName; | ||
|
|
||
| @SerializedName("url") | ||
| private final String url; | ||
|
|
||
| @SerializedName("required") | ||
| private final boolean required; | ||
|
|
||
| @Nullable | ||
| private transient final RemoteMod mod; | ||
|
|
||
| public CurseManifestFile() { | ||
| this(0, 0, null, null, true, null); | ||
| } | ||
|
|
||
| public CurseManifestFile(int projectID, int fileID, String fileName, String url, boolean required, RemoteMod mod) { | ||
| this.projectID = projectID; | ||
| this.fileID = fileID; | ||
| this.fileName = fileName; | ||
| this.url = url; | ||
| this.required = required; | ||
| this.mod = mod; | ||
| } | ||
|
|
||
| public CurseManifestFile(int projectID, int fileID, String fileName, String url, boolean required) { | ||
| this(projectID, fileID, fileName, url, required, null); | ||
| } | ||
|
|
||
| public int getProjectID() { | ||
| return projectID; | ||
| } | ||
|
|
||
| public int getFileID() { | ||
| return fileID; | ||
| } | ||
|
|
||
| @Override | ||
| public String getFileName() { | ||
| return fileName; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean isOptional() { | ||
| return !isRequired(); | ||
| } | ||
|
|
||
| @Override | ||
| public String getPath() { | ||
| return "mods/" + getFileName(); | ||
| } | ||
|
|
||
| public boolean isRequired() { | ||
| return required; | ||
| } | ||
|
|
||
| public int projectID() { | ||
| return projectID; | ||
| } | ||
|
|
||
| public int fileID() { | ||
| return fileID; | ||
| } | ||
|
|
||
| public String fileName() { | ||
| return fileName; | ||
| } | ||
|
|
||
| public boolean required() { | ||
| return required; | ||
| } | ||
|
|
||
| @Override | ||
| public void validate() throws JsonParseException { | ||
| if (projectID == 0 || fileID == 0) | ||
| throw new JsonParseException("Missing Project ID or File ID."); | ||
| } | ||
|
|
||
| @Override | ||
| @Nullable | ||
| public String url() { | ||
| if (url == null) { |
There was a problem hiding this comment.
由于 CurseManifestFile 从 record 转换为了 class，它不再自动拥有 equals 和 hashCode 实现。在 CurseCompletionTask 中，逻辑依赖于 Set.contains(f) 来过滤文件，缺少这些方法会导致过滤逻辑失效（变为引用相等判断）。请手动实现这两个方法。
| this.manifest = manifest; | ||
| if (manifest.getManifest() instanceof ModpackManifest.SupportOptional) { | ||
| allFiles.setAll(((ModpackManifest.SupportOptional) manifest.getManifest()).getFiles()); | ||
| if (allFiles.stream().anyMatch(ModpackFile::isOptional)) { | ||
| loadOptionalFiles(); | ||
| btnOptionalFiles.setVisible(true); | ||
| } | ||
| } |
There was a problem hiding this comment.
在 whenComplete 回调中，如果 exception 不为 null（例如读取整合包清单失败），则 manifest 参数通常为 null。此时直接访问 manifest.getManifest() 会导致 NullPointerException。建议在处理 manifest 逻辑前增加非空检查。
| List<? extends ModpackFile> files = ((ModpackManifest.SupportOptional) manifest | ||
| .setManifest(manifest1).getManifest()).getFiles(); | ||
| allFiles.setAll(files); |
There was a problem hiding this comment.
在 loadOptionalFiles 的异步回调中，如果 manifest1 为 null（例如网络请求失败），调用 manifest.setManifest(manifest1).getManifest() 将返回 null，随后强制转换为 SupportOptional 并调用 getFiles() 会触发 NullPointerException。应当先验证 exception 和 manifest1 的状态。
| private Label text1 = new Label(); | ||
| private ModpackFile currentFile = null; | ||
| private ChangeListener<Boolean> selectedListener = (observable, oldValue, newValue) -> { | ||
| if (currentFile != null) { | ||
| if (newValue) { | ||
| excludedFiles.remove(currentFile); | ||
| } else { | ||
| excludedFiles.add(currentFile); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| public OptionalFileEntry(JFXListView<ModpackFile> listView) { | ||
| super(listView); | ||
| container.setPickOnBounds(false); | ||
| container.setAlignment(Pos.CENTER_LEFT); | ||
| HBox.setHgrow(content, Priority.ALWAYS); | ||
| content.setMouseTransparent(true); | ||
| setSelectable(); | ||
| container.getChildren().setAll(checkBox, content); | ||
|
|
||
| infoButton.getStyleClass().add("toggle-icon4"); | ||
| infoButton.setGraphic(SVG.INFO.createIcon()); | ||
| container.getChildren().add(infoButton); | ||
| getContainer().getChildren().setAll(container); | ||
| } | ||
|
|
||
| @Override | ||
| protected void updateControl(ModpackFile item, boolean empty) { | ||
| if (empty) | ||
| return; | ||
| checkBox.selectedProperty().removeListener(selectedListener); | ||
| currentFile = item; | ||
| String name = item.getFileName(); | ||
| text1.setText(name); |
There was a problem hiding this comment.
text1 标签在 updateControl 中被设置了文本，但并未被添加到 container 或任何 UI 布局中。由于已经使用了 content (TwoLineListItem) 来展示文件名，这个 text1 变量是多余的，建议移除。
| public String getManifestType() { | ||
| return manifestType; | ||
| } | ||
|
|
||
| public int getManifestVersion() { | ||
| return manifestVersion; | ||
| } | ||
|
|
||
| public String getName() { | ||
| return name; | ||
| } | ||
|
|
||
| public String getVersion() { | ||
| return version; | ||
| } | ||
|
|
||
| public String getAuthor() { | ||
| return author; | ||
| } | ||
|
|
||
| public String getOverrides() { | ||
| return overrides; | ||
| } | ||
|
|
||
| public CurseManifestMinecraft getMinecraft() { | ||
| return minecraft; | ||
| } | ||
|
|
||
| public List<CurseManifestFile> getFiles() { | ||
| return files; | ||
| } | ||
|
|
||
| public String manifestType() { | ||
| return manifestType; | ||
| } | ||
|
|
||
| public int manifestVersion() { | ||
| return manifestVersion; | ||
| } | ||
|
|
||
| public String name() { | ||
| return name; | ||
| } | ||
|
|
||
| public String version() { | ||
| return version; | ||
| } | ||
|
|
||
| public String author() { | ||
| return author; | ||
| } | ||
|
|
||
| public String overrides() { | ||
| return overrides; | ||
| } | ||
|
|
||
| public CurseManifestMinecraft minecraft() { | ||
| return minecraft; | ||
| } | ||
|
|
||
| public List<CurseManifestFile> files() { | ||
| return files; | ||
| } |
There was a problem hiding this comment.
类中同时保留了 getName() / name() 等两套 Getter 方法。既然已从 record 转换为普通类，建议统一使用 Java 标准的 get... 命名风格，移除冗余的方法以保持代码整洁。
| manifest.getFiles().parallelStream() | ||
| .map(file -> { | ||
| if ((StringUtils.isBlank(file.getFileName()) || file.getUrl() == null) && file.isOptional()) { | ||
| try { | ||
| RemoteMod mod = CurseForgeRemoteModRepository.MODS.getModById(downloadProvider, Integer.toString(file.getProjectID())); | ||
| RemoteMod.File remoteFile = CurseForgeRemoteModRepository.MODS.getModFile(Integer.toString(file.getProjectID()), Integer.toString(file.getFileID())); | ||
| return file.withFileName(remoteFile.getFilename()).withURL(remoteFile.getUrl()).withMod(mod); | ||
| } catch (FileNotFoundException fof) { | ||
| LOG.warning("Could not query api.curseforge.com for deleted mods: " + file.getProjectID() + ", " + file.getFileID(), fof); | ||
| return file; | ||
| } catch (IOException | JsonParseException e) { | ||
| LOG.warning("Unable to fetch the file name projectID=" + file.getProjectID() + ", fileID=" + file.getFileID(), e); | ||
| return file; | ||
| } | ||
| } else { | ||
| return file; | ||
| } | ||
| }) | ||
| .collect(Collectors.toList())); |
There was a problem hiding this comment.
在 parallelStream 中执行同步网络请求（getModById, getModFile）可能会导致 ForkJoinPool 线程耗尽，且缺乏对并发数和超时/重试的精细控制。建议使用专门的线程池或异步任务处理。
|
ping @CaveNightingale 敲打本PR作者 日活非常低 :( |
请问之前要求的渐变过渡是指什么效果？ |
不到啊 问Glavo :( |
|
CC @Glavo |
|
|
动机
我们目前在运营一个服务器，服务器提供两组模组，一组要求玩家必须安装，例如匠神、农夫乐事等，另一组推荐玩家选择安装，例如投影图、FreeCam等。
我们曾经尝试使用整合包格式分发，事实上Modrinth和CurseForge整合包格式均支持声明可选模组/文件，但发现HMCL会直接将这些文件当作必选处理。
目前，我们仍然在使用直接压缩
.jar文件的方式分发模组。我们希望未来能够通过分发HMCL整合包的形式分发我们的服务器客户端。描述
风险
非目标