[Feature] 让Java下载页面可以同时下载多个Java#5394
[Feature] 让Java下载页面可以同时下载多个Java#5394NoClassFoundError wants to merge 13 commits intoHMCL-dev:mainfrom
Conversation
|
cc @3gf8jv4dv |
|
我在 #2988 (review) 建议过搞一个复选框，但 Glavo 最后没做。不知道现在 Glavo 怎么想。 |
There was a problem hiding this comment.
Pull request overview
This PR updates the Mojang Java download dialog to support selecting and downloading multiple Java major versions in one operation.
Changes:
- Replace single-choice radio buttons with multi-select checkboxes and update dialog validation accordingly.
- Update accept handler to gather multiple selected versions and download them sequentially (including optional override flow).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| else chain = chain.thenComposeAsync(Schedulers.javafx(), ignore -> task); | ||
| } | ||
|
|
||
| for (GameJavaVersion v : notInstalled) { | ||
| Task<?> task = downloadTask(v); | ||
| if (chain == null) chain = task; | ||
| else chain = chain.thenComposeAsync(Schedulers.javafx(), ignore -> task); |
There was a problem hiding this comment.
The download chain is built with thenComposeAsync(...), which stops executing subsequent tasks if any earlier download fails (because thenComposeAsync relies on dependents succeeding). With multi-select, a single failed Java download will prevent the remaining selected versions from downloading. Consider chaining with withComposeAsync(...) (doesn’t break the chain on dependent failure) or wrapping each per-version task so failures are handled and the chain continues, while still reporting errors to the user.
| else chain = chain.thenComposeAsync(Schedulers.javafx(), ignore -> task); | |
| } | |
| for (GameJavaVersion v : notInstalled) { | |
| Task<?> task = downloadTask(v); | |
| if (chain == null) chain = task; | |
| else chain = chain.thenComposeAsync(Schedulers.javafx(), ignore -> task); | |
| else chain = chain.withComposeAsync(Schedulers.javafx(), ignore -> task); | |
| } | |
| for (GameJavaVersion v : notInstalled) { | |
| Task<?> task = downloadTask(v); | |
| if (chain == null) chain = task; | |
| else chain = chain.withComposeAsync(Schedulers.javafx(), ignore -> task); |
| Label prompt = new Label(i18n("java.download.prompt")); | ||
| vbox.getChildren().add(prompt); | ||
|
|
||
| for (GameJavaVersion version : supportedGameJavaVersions) { | ||
| JFXRadioButton button = new JFXRadioButton("Java " + version.majorVersion()); | ||
| JFXCheckBox button = new JFXCheckBox("Java " + version.majorVersion()); |
There was a problem hiding this comment.
The UI now allows selecting multiple Java versions (checkboxes), but the prompt/confirmation i18n keys used here are still phrased for a single selection (e.g., java.download.prompt / download.java.override). This will display misleading text once multi-select is enabled; consider updating/adding i18n strings to explicitly indicate multiple selections and (for override) potentially multiple existing versions.
| private void startDownload(List<GameJavaVersion> installed, List<GameJavaVersion> notInstalled) { | ||
| Task<?> chain = null; | ||
|
|
||
| for (GameJavaVersion v : installed) { | ||
| Task<?> task = Task.supplyAsync(() -> JavaManager.REPOSITORY.getJavaExecutable(platform, v)) |
There was a problem hiding this comment.
startDownload manually builds a sequential task chain using a nullable chain and repeated if (chain == null) ... else ... blocks. This is harder to reason about and maintain as the flow grows. Consider collecting per-version tasks into a list and using an existing helper like Task.runSequentially(...) (or a small utility) to assemble the sequence more clearly.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (isDependentsSucceeded() != (Task.this.getException() == null)) | ||
| throw new AssertionError("When whenComplete succeeded, Task.exception must be null.", Task.this.getException()); | ||
|
|
||
| if (isDependenciesSucceeded()) { |
There was a problem hiding this comment.
The method should check isDependentsSucceeded() instead of isDependenciesSucceeded(). The wrapper task has no dependencies (tasks that run after it), only dependents (Task.this, which runs before it). The check should determine whether Task.this succeeded, which is what isDependentsSucceeded() does.
| if (isDependenciesSucceeded()) { | |
| if (isDependentsSucceeded()) { |
|
|
||
| if (exceptionToDisplay != null) { | ||
| LOG.warning("Failed to download java", exceptionToDisplay); | ||
| Controllers.dialog(DownloadProviders.localizeErrorMessage(exception), i18n("install.failed")); |
There was a problem hiding this comment.
The error message should use exceptionToDisplay instead of exception. The variable exception may be null even when exceptionToDisplay is not null (when exceptions were collected from failed Result objects), causing an incorrect or missing error message to be displayed.
| Controllers.dialog(DownloadProviders.localizeErrorMessage(exception), i18n("install.failed")); | |
| Controllers.dialog(DownloadProviders.localizeErrorMessage(exceptionToDisplay), i18n("install.failed")); |
3 participants
No description provided.