Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds functionality to collapse snapshot Minecraft versions in the download page, preventing snapshot versions from overwhelming the mod download page. The feature groups snapshot and pre-release versions under their target release version in collapsible sections, making it easier to find the desired mod version. If the selected instance uses a snapshot version, recommendations are shown normally.
Changes:
- Added
getReleaseOfSnapshotmethod to determine the target release version for snapshots and pre-releases - Modified download page UI to group versions by release and separate snapshots into collapsible sections
- Added test coverage for the new version mapping functionality
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| GameVersionNumber.java | Added getReleaseOfSnapshot static method to map snapshot/pre-release versions to their target release version |
| GameVersionNumberTest.java | Added test cases for the new getReleaseOfSnapshot method |
| DownloadPage.java | Modified version display logic to group and separate releases from snapshots, with snapshots shown in collapsible sublists |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
HMCLCore/src/test/java/org/jackhuang/hmcl/util/versioning/GameVersionNumberTest.java
Outdated
Show resolved
Hide resolved
| List<String> versionList = control.versions.keys().stream() | ||
| .map(gv -> Objects.requireNonNullElse(GameVersionNumber.getReleaseOfSnapshot(gv), gv)) | ||
| .distinct() | ||
| .sorted(Collections.reverseOrder(GameVersionNumber::compare)) | ||
| .toList()) { | ||
| List<RemoteMod.Version> versions = control.versions.get(gameVersion); | ||
| if (versions == null || versions.isEmpty()) { | ||
| continue; | ||
| } | ||
| .toList(); | ||
|
|
||
| for (String release : versionList) { | ||
| List<String> releases = new ArrayList<>(); | ||
| List<String> snapshots = new ArrayList<>(); | ||
|
|
||
| var sublist = new ComponentSublist(() -> { | ||
| ArrayList<ModItem> items = new ArrayList<>(versions.size()); | ||
| for (RemoteMod.Version v : versions) { | ||
| items.add(new ModItem(control.addon, v, control)); | ||
| for (String gv : control.versions.keys()) { | ||
| if (release.equals(Objects.requireNonNullElse(GameVersionNumber.getReleaseOfSnapshot(gv), gv))) { | ||
| (gv.equals(release) ? releases : snapshots).add(gv); |
There was a problem hiding this comment.
The method GameVersionNumber.getReleaseOfSnapshot is called multiple times for the same version string (once on line 302 and again on line 312 for each game version). Consider caching the results in a Map to avoid redundant computation, especially since this involves parsing and comparison operations that could be expensive for large lists of versions.
| var sublist = new ComponentSublist(() -> { | ||
| ArrayList<ModItem> items = new ArrayList<>(); | ||
| for (String gv : target) { | ||
| List<RemoteMod.Version> versions = control.versions.get(gv); | ||
| if (versions != null) { | ||
| for (RemoteMod.Version v : versions) { | ||
| items.add(new ModItem(control.addon, v, control)); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The sorting of versions within each group (releases and snapshots) is not preserved. The target lists are built by iterating through control.versions.keys(), which may not maintain the original sort order. Consider sorting the releases and snapshots lists before creating ModItems to ensure versions are displayed in the correct order (newest first).
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The method getReleaseOfSnapshot does not handle Special versions (e.g., April Fools versions like "20w14infinite"). These versions will fall through and return null, which is consistent with the test case on line 516. However, it would be beneficial to add a code comment explaining this behavior for maintainability.
| // Special or non-standard versions (e.g. April Fools snapshots like "20w14infinite") | |
| // are intentionally not mapped to a GA release and will fall through to return null. |
| for (String gv : control.versions.keys()) { | ||
| if (release.equals(Objects.requireNonNullElse(GameVersionNumber.getReleaseOfSnapshot(gv), gv))) { | ||
| (gv.equals(release) ? releases : snapshots).add(gv); | ||
| } | ||
| return items; | ||
| }); | ||
| sublist.getStyleClass().add("no-padding"); | ||
| sublist.setTitle("Minecraft " + gameVersion); | ||
| } |
There was a problem hiding this comment.
The loop iterates through control.versions.keys() for each release version, which creates an O(nm) time complexity where n is the number of unique releases and m is the total number of game versions. Consider building a map from release to game versions in a single pass through control.versions.keys() to improve performance from O(nm) to O(m).
…VersionNumberTest.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
很少有人在快照版使用模组，防止部分模组的下载页面被快照版占领，耗费更多时间找想要下载的版本。
如果选择的实例是快照版本，推荐是正常的（如图）。
PCL 等启动器也做了类似的操作。