Conversation
CiiLu
commented
Feb 4, 2026
CiiLu
commented
同时包括 assets libraries 补全
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue where tasks were not properly categorized during game launch file completion. The changes enable multiple stage names to be grouped together and displayed under a single stage node in the UI.
Changes:
- Modified the stage hint system to support grouping multiple stage names under a single display stage
- Updated task counting logic in StageNode to track multiple concurrent tasks per stage
- Applied the new multi-stage grouping to the game launch flow, grouping assets, libraries, and modpack downloads under the dependencies stage
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| HMCLCore/src/main/java/org/jackhuang/hmcl/task/TaskExecutor.java | Changed stages field type from List<String> to List<List<String>> to support stage grouping |
| HMCLCore/src/main/java/org/jackhuang/hmcl/task/Task.java | Added withMultiStagesHint method and converted withStagesHint to use it for backward compatibility |
| HMCL/src/main/java/org/jackhuang/hmcl/ui/construct/TaskListPane.java | Updated addStages to map multiple stage names to single display nodes, added task counting to track concurrent tasks per stage |
| HMCL/src/main/java/org/jackhuang/hmcl/game/LauncherHelper.java | Applied multi-stage grouping to game launch, grouping dependencies, assets, libraries, and modpack downloads together |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| stageNodes.computeIfAbsent(stage, s -> { | ||
| StageNode node = new StageNode(stage); | ||
| private void addStages(@NotNull Collection<List<String>> stageGroups) { | ||
| for (List<String> group : stageGroups) { |
There was a problem hiding this comment.
The code calls group.get(0) without checking if the list is empty. If an empty list is passed in the stageGroups collection, this will throw an IndexOutOfBoundsException. Consider adding a check to skip empty groups or validate that all groups contain at least one element.
| for (List<String> group : stageGroups) { | |
| for (List<String> group : stageGroups) { | |
| if (group == null || group.isEmpty()) { | |
| continue; | |
| } |
| public void fail() { | ||
| runningTasksCount = Math.max(0, runningTasksCount - 1); | ||
| status.set(Status.FAILED); | ||
| } |
There was a problem hiding this comment.
The succeed() method only sets the status to SUCCESS when runningTasksCount reaches 0, which allows multiple tasks in the same stage to complete successfully. However, the fail() method immediately sets the status to FAILED regardless of the remaining task count. This creates an inconsistency: if a stage has multiple running tasks and one fails while others are still running, the stage will be marked as FAILED even though other tasks might still succeed. Consider whether the fail logic should also check the running task count, or document why immediate failure is the desired behavior.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| List<Task.StagesHint> stages = new ArrayList<>(); | ||
| for (Object value : settings.asStringMap().values()) { | ||
| if (value instanceof RemoteVersion remoteVersion) { | ||
| ret = ret.thenComposeAsync(version -> dependencyManager.installLibraryAsync(version, remoteVersion)); | ||
| stages.add(String.format("hmcl.install.%s:%s", remoteVersion.getLibraryId(), remoteVersion.getSelfVersion())); | ||
| stages.add(new Task.StagesHint(String.format("hmcl.install.%s:%s", remoteVersion.getLibraryId(), remoteVersion.getSelfVersion()))); | ||
| if ("game".equals(remoteVersion.getLibraryId())) { | ||
| stages.add("hmcl.install.libraries"); | ||
| stages.add("hmcl.install.assets"); | ||
| stages.add(new Task.StagesHint("hmcl.install.libraries")); | ||
| stages.add(new Task.StagesHint("hmcl.install.assets")); | ||
| } |
There was a problem hiding this comment.
This local variable is still named "stages" but now holds Task.StagesHint objects (including aliases). Renaming to "hints" (or similar) would reduce confusion with the new stages-hints API.
| int remaining = runningTasksCount - 1; | ||
| runningTasksCount = Math.max(0, remaining); | ||
|
|
||
| if (runningTasksCount == 0) { |
There was a problem hiding this comment.
StageNode.succeed() can set status to SUCCESS even if a previous task in the same stage already called fail(), which would incorrectly clear the FAILED state when the last remaining task succeeds. Consider preserving FAILED once it happens (e.g., only transition to SUCCESS when status is not FAILED), while still decrementing runningTasksCount.
| if (runningTasksCount == 0) { | |
| if (runningTasksCount == 0 && status.get() != Status.FAILED) { |
| }); | ||
| } | ||
| for (String stage : hint.aliases()) { | ||
| stageNodes.put(stage, node); |
There was a problem hiding this comment.
addStagesHints() unconditionally overwrites stageNodes mappings for aliases. If an alias key is already mapped to a different StageNode (because it was previously registered as a primary stage), this can orphan the original StageNode in the list and reroute updates unexpectedly. Consider detecting this case (alias already present and != current node) and skipping/merging explicitly instead of overwriting silently.
| stageNodes.put(stage, node); | |
| StageNode existing = stageNodes.get(stage); | |
| if (existing == null) { | |
| stageNodes.put(stage, node); | |
| } else if (existing == node) { | |
| // Alias already correctly mapped to this node; nothing to do. | |
| } else { | |
| // Alias is already mapped to a different StageNode; avoid overwriting silently. | |
| } |
| List<Task.StagesHint> stages = new ArrayList<>(); | ||
|
|
||
| Task<Version> libraryTask = Task.supplyAsync(() -> new Version(name)); | ||
| libraryTask = libraryTask.thenComposeAsync(libraryTaskHelper(gameVersion, "game", gameVersion)); | ||
| stages.add("hmcl.install.game:" + gameVersion); | ||
| stages.add("hmcl.install.libraries"); | ||
| stages.add("hmcl.install.assets"); | ||
| stages.add(new Task.StagesHint("hmcl.install.game:" + gameVersion)); | ||
| stages.add(new Task.StagesHint("hmcl.install.libraries")); | ||
| stages.add(new Task.StagesHint("hmcl.install.assets")); |
There was a problem hiding this comment.
This local variable is still named "stages" but now holds Task.StagesHint objects (including aliases). Renaming to "hints" (or similar) would reduce confusion with the old List stages API.
2 participants
