code icon indicating copy to clipboard operation
code copied to clipboard

feat: support hook parameters/environment variables for pre-launch hook for Modrinth Desktop App

Open EchoEllet opened this issue 1 year ago • 6 comments

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_21 from 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 function get_java_version_from_profile which requires the Profile and VersionInfo, I'm unfamiliar with the code though it seems that we need to either use get_optimal_jre_key or require an additional parameter in run_credentials to require VersionInfo, 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 to VersionInfo from 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 with Pre-Launch
  • Incorrect Handling of $INST_JAVA_ARGS: The current implementation does not pass $INST_JAVA_ARGS correctly.
  • 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 unwrap function 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

Pre-Launch Hook parameters with an executable JAR file

EchoEllet avatar Jul 07 '24 03:07 EchoEllet

This PR is ready for review, it's not ready for merge yet.

EchoEllet avatar Jul 07 '24 08:07 EchoEllet

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 "

EchoEllet avatar Jul 09 '24 14:07 EchoEllet

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.

brawaru avatar Jul 09 '24 14:07 brawaru

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)?

Norbiros avatar Jul 09 '24 14:07 Norbiros

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.

EchoEllet avatar Jul 09 '24 14:07 EchoEllet

By default, java_args are 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?

EchoEllet avatar Jul 09 '24 14:07 EchoEllet

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.

Prospector avatar Jul 09 '25 21:07 Prospector

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.

EchoEllet avatar Jul 10 '25 05:07 EchoEllet