Mod Manager Initial Design

[foresterfx]

Supports:

• Read/Write operations on loadorder through the UI interactions
• Launching FFX, FFX-2, FFX-2LM via menu options (debug as well)
• Warning information given to user if binaries not found within game folder
• Importing/Exporting of zipped directory containing mods

[EvelynTSMG]

... Could you at least tell the "AI" to be straightforward and to-the-point instead of talking as if it's trying to sell me a car?

My brain wants to explode whenever I try to extract any information out of this stars-forsaken text, so if it's not easy to read, I'm not doing so. Especially when you didn't care to write it in the first place.

[fkelava]

Your JsonSerializerOptions likely didn't IncludeFields.

[fkelava]

Once #233 merges, this should use the style defined in Fahrenheit's FhImGuiHelper. We may have to slightly refactor the helper to allow its use outside of the game process to do so. Try it- if push comes to shove we'll do it ourselves and you'll rebase against it.

[fkelava]

Depending on @EvelynTSMG's opinion, we may want to move this up somewhere in or around the ImGuiHelper. Reusable elements shared in- and out of game seem like something we'll eventually have to get to.

[EvelynTSMG]

Please click the "Resolve Conversation" button on GitHub when fulfilling a change request. It makes it much easier to scroll through the PR later. I have taken the liberty of doing so myself for the ones you reacted with 👍 to.

[fkelava]

This is wrong. The mod manager is emitted to bin in a normal build (and runs from there), so AppContext.BaseDirectory will be {GAME_DIR}\fahrenheit\bin, and you'll incorrectly try to find mods under {GAME_DIR}\fahrenheit\bin\mods.

[fkelava]

One should also be permitted to specify an author, version, and link. All three can be text inputs as well.

[fkelava]

This is insufficiently constrained. If the user gives the wrong folder, import will proceed regardless and the resulting mod will have no effect. An EFL mod may only contain (at its root) one or more of the following subfolders:

• ffx_data
• ffx_ps2
• ffx-2_data
• metamenu
• version_config

Check case-insensitively.

[fkelava]

I take it this was overlooked when erasing some code. It's unused.

[fkelava]

This check is well-intentioned, but a bit pointless. If fhstage1.dll or the game binary were missing, then Stage0 would surface that error. If fhstage0 were missing, the try-block beneath would catch that. Besides, quite a few more files are required for boot to properly succeed.

We can just do away with this and include a "health check" function in a follow-up PR.

[fkelava]

This doesn't help. You can still get the directory deleted between this check and File.Exists(manifest_path) below. Just ditch this and take the fault at the point you try to read the manifest. "Manifest not found" does not need to clarify whether that is because the file isn't there or the entire directory was torched.

[fkelava]

You can do away with this and replace all the usages with a snippet roughly as follows (adapt to the actual target type) - don't forget to add a global using System.Threading in usings.cs;

FhPendingModToggle? action = Interlocked.Exchange(ref _pending_mod_toggle, null);
if (action == null)
    return;

[fkelava]

I'm perplexed by why this is not simply:

try {
    FhManifest manifest = JsonSerializer.Deserialize<FhManifest>(File.ReadAllText(manifest_path))
        ?? throw new Exception($"Manifest invalid or illegible: {manifest_path}");
        
    return new(manifest, // ...
}
// ... catch-block as it is

You can then override empty fields if you really care to. But I don't follow why we're creating a manifest from whole cloth when it's right there.

[fkelava]

Considering the last comment about manifest deserialization, this can be removed.

[fkelava]

This doesn't seem right. For one, this finally block itself can throw. Then what?

If the intended contract here is to throw on failure, then you can reduce the method to this:

string temporary_path = $"{destination}.tmp";
string backup_path    = $"{destination}.bak";

File.WriteAllText(temporary_path, contents);
File.Replace     (temporary_path, destination, backup_path);

File.Replace is the actually atomic way to do what you wanted¹. As a bonus you get a backup copy.

If you want to handle throws, then wrap the latter two calls in a try with a non-throwing catch.

Footnotes

1. https://zenn.dev/arika/articles/20251010-csharp-replace-is-atomic?locale=en ↩

[fkelava]

Trim instances of double empty lines anywhere they might be found to single empty lines, ex. this, L29-30 in the same file, etc.