feat: support hook parameters/environment variables for pre-launch hook for Modrinth Desktop App
Support Hook Parameters for the Pre-Launch Hook
The parameter/variable names are the same as MultiMC and MultiMC-based launchers (e.g., Prism Launcher). It's also adapted by some other launchers that are not based on MutliMC (e.g., ATLauncher).
As this is my first (and likely last) time working with Rust and Tauri, this code may not adhere to high-quality code standards.
This PR has a few issues:
- ~~Java Version: It always uses
JAVA_21from the settings instead of the one from the profile if available, also the profile or settings could use a different Java path, to solve this we need to call the functionget_java_version_from_profilewhich requires theProfileandVersionInfo, I'm unfamiliar with the code though it seems that we need to either useget_optimal_jre_keyor require an additional parameter inrun_credentialsto requireVersionInfo, We also need to update the function call from JS since the project use bridge between JS and Rust code, and we probably don't have access toVersionInfofrom there too, we might need to move this logic into somewhere else~~ Issue solved, though we still might need to review it - Only support
Pre-Launch: We need to refactor the code or move the Hooks parameters logic somewhere else along withPre-Launch - Incorrect Handling of
$INST_JAVA_ARGS: The current implementation does not pass$INST_JAVA_ARGScorrectly. - Code Quality: The code could likely benefit from improvements. Feel free to request changes or leave a review. We could also consider closing this PR if it is not suitable.
- Error Handling: The code uses the
unwrapfunction which seems to throw an error in case the result has not succeeded. It seems some of the code in the project uses this too, I'm uncertain if there is a possibility the functions we're using won't succeed though we might want to consider handling the errors.
To solve the first and second issues, we might consider moving the code to launch_minecraft as currently the logic for Wrapper and Post Exit hooks are in there, and in there, we have access to VersionInfo. The first issue could be solved without refactoring the code.
Unrelated
In launch_minecraft, Noticed that the parameter name for Wrapper Hook is wrapper while Post Exit Hook is post_exit_hook, I'm not sure if there is a reason though this might make the code a bit inconsistent.
The hooks expect an executable to be used instead of executing the command in the command line, which doesn't allow to use of something like java --version in case java is in the path.
You will get an error:
An error occurred
I/O error: program not found, path:
The variable jre_key is of type JavaVersion, it seems to have the Java Path instead of the key in the settings/profile, called it jre_key because it uses the function get_optimal_jre_key
Related Feature requests
- #1238
- #952
In case this PR will be merged (after reviewing it and improving it), we will also need to add docs, we could document this in Modrinth App Docs, then add a link in the app to the page for more details.
Screenshot of using Pre-Launch Hook parameters with an executable JAR file
This PR is ready for review, it's not ready for merge yet.
The parameter $INST_JAVA_ARGS (Java args/parameters) is not being passed as expected, it should be something like this:
-XX:+UnlockExperimentalVMOptions -XX:+UseG1GC -XX:G1NewSizePercent=20 -XX:G1ReservePercent=20 -XX:MaxGCPauseMillis=50 -XX:G1HeapRegionSize=32M
What is currently:
"hello C:\\Users\\%USERNAME%\\AppData\\Roaming\\com.modrinth.theseus\\meta\\java_versions\\zulu21.34.19-ca-jre21.0.3-win_x64\\bin\\javaw.exe_ARGS"
Assuming the command is:
hello $INST_JAVA_ARGS
It seems that the line:
replace("$INST_JAVA", jre_key.unwrap().path.as_str())
conflict with:
replace("$INST_JAVA_ARGS", java_args.join(" ").as_str())
Because both start with $INST_JAVA. Not sure how it should be done on Rust.
Even without $INST_JAVA it seems that java_args.join(" ").as_str() return empty string:
"hello "
Either use regular extensions with boundaries or rearrange the replacements order so that $INST_JAVA_ARGS gets replaced before $INST_JAVA. You should however check how this replacement is done in other launchers so that we don't deviate from conventional replacement behaviour.
You should however check how this replacement is done in other launchers so that we don't deviate from conventional replacement behaviour.
I was able to reproduce the same bug in MultiMC (they use similar approach), so if you want, you can create an issue there.
To fix it you could just change order of variables, so INST_JAVA_ARGS comes before INST_JAVA.
Even without $INST_JAVA it seems that java_args.join(" ").as_str() return empty string:
By default, java_args are empty...
Also I found out that other launchers also parse normal environment variables, could you also add that (Rust docs)?
You should however check how this replacement is done in other launchers so that we don't deviate from conventional replacement behaviour.
They usually use regular expression pattern, Example in ATLauncher.
By default,
java_argsare empty...
How can we access the Java arguments that will be used to launch the game? Can we only access it in the launch_minecraft function?
I'm going to go ahead and close this PR for now, since it seems like it has a decent ways to go and has been stale for quite a while. This is definitely something I think we should add in the future, though.
Only support Pre-Launch: We need to refactor the code or move the Hooks parameters logic somewhere else along with Pre-Launch
I wasn't sure about whether I should refactor the existing code to finish some of the tasks.