Skip to content

Conversation

@CiiLu
Copy link
Contributor

@CiiLu CiiLu commented Jan 20, 2026

No description provided.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR attempts to simplify two methods by replacing explicit conditional checks with loop-based iteration. It adds a new getLibraries() helper method to LibraryAnalyzer that returns all detected library types.

Changes:

  • Added getLibraries() method to LibraryAnalyzer that returns all library types present in the analyzed version
  • Refactored McbbsModpackExportTask#execute to use a loop over getLibraries() instead of explicit ifPresent() calls for each library type
  • Refactored DefaultLauncher#getEnvVars to use a loop over getModLoaders() instead of explicit checks for each mod loader type

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
HMCLCore/src/main/java/org/jackhuang/hmcl/download/LibraryAnalyzer.java Adds new getLibraries() method to return all detected library types
HMCLCore/src/main/java/org/jackhuang/hmcl/mod/mcbbs/McbbsModpackExportTask.java Replaces explicit library type checks with loop over getLibraries()
HMCLCore/src/main/java/org/jackhuang/hmcl/launch/DefaultLauncher.java Replaces explicit mod loader checks with loop over getModLoaders(), adds ModLoaderType import

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +585 to 588
else if (type == ModLoaderType.LEGACY_FABRIC) env.put("INST_LEGACYFABRIC", "1");
else env.put("INST_" + type.name().toUpperCase(Locale.ROOT), "1");
}

Copy link

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This refactoring breaks functionality for LITE_LOADER and OPTIFINE:

  1. LITE_LOADER: The ModLoaderType enum value is LITE_LOADER, so type.name().toUpperCase() will produce "LITE_LOADER", resulting in environment variable INST_LITE_LOADER. However, the old code set INST_LITELOADER (no underscore). This is a breaking change.

  2. OPTIFINE: The LibraryType.OPTIFINE has modLoader = false in its definition (line 304 of LibraryAnalyzer.java), so it won't be included in getModLoaders() results. The old code set INST_OPTIFINE, but the new code will not set this variable at all.

To fix this, you should either:

  • Add special cases for LITE_LOADER and handle OPTIFINE separately, or
  • Iterate over all libraries that need environment variables (not just mod loaders) and use a mapping function to handle the naming differences
Suggested change
else if (type == ModLoaderType.LEGACY_FABRIC) env.put("INST_LEGACYFABRIC", "1");
else env.put("INST_" + type.name().toUpperCase(Locale.ROOT), "1");
}
else if (type == ModLoaderType.LEGACY_FABRIC) env.put("INST_LEGACYFABRIC", "1");
else if (type == ModLoaderType.LITE_LOADER) env.put("INST_LITELOADER", "1");
else env.put("INST_" + type.name().toUpperCase(Locale.ROOT), "1");
}
if (analyzer.hasOptiFine()) {
env.put("INST_OPTIFINE", "1");
}

Copilot uses AI. Check for mistakes.
Comment on lines +98 to +101
for (LibraryAnalyzer.LibraryType type : analyzer.getLibraries()) {
if (type == MINECRAFT) addons.add(new McbbsModpackManifest.Addon(MINECRAFT.getPatchId(), gameVersion));
else addons.add(new McbbsModpackManifest.Addon(type.getPatchId(), analyzer.getVersion(type).orElseThrow()));
}
Copy link

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This refactoring introduces multiple critical issues:

  1. Potential runtime exceptions: Using orElseThrow() can cause exceptions even when has(type) is true, because getVersion() can return an empty Optional if the version string is null in the internal map.

  2. Changed behavior - includes unintended library types: The old code explicitly handled specific library types (MINECRAFT, FORGE, CLEANROOM, NEO_FORGE, LITELOADER, OPTIFINE, FABRIC, QUILT, LEGACY_FABRIC). The new code includes ALL library types, which means it will also add FABRIC_API, QUILT_API, LEGACY_FABRIC_API, and BOOTSTRAP_LAUNCHER to the manifest. These were not included before and could break the modpack format.

To fix these issues:

  • Replace orElseThrow() with safe handling like filtering out libraries without versions first, or using a default value
  • Filter library types to only include the relevant ones that should be in the manifest, similar to the explicit list in the old code

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant