fix(wrapper): 更换老旧的 RetroWrapper 到 LegacyFix#3159
Conversation
pcl-ce-automation
Bot
added
🛠️ 等待审查
审阅者指南用 LegacyFix 替换过时的基于 RetroWrapper 的兼容层，并将其接入到启动参数构造、资源解压以及配置开关中，同时更新相关的 UI 绑定和本地化键。 Minecraft 启动中 LegacyFix 集成的时序图sequenceDiagram
participant McLaunchArgumentsJvmNew
participant McLaunchArgumentsReplace
participant McLaunchNeedsLegacyFix
participant Config
participant ModBase
McLaunchArgumentsJvmNew->>McLaunchNeedsLegacyFix: McLaunchNeedsLegacyFix(instance)
McLaunchNeedsLegacyFix->>Config: Launch.DisableLF
McLaunchNeedsLegacyFix->>Config: Instance.DisableLF[mc.PathInstance]
McLaunchNeedsLegacyFix-->>McLaunchArgumentsJvmNew: needsLegacyFix
alt needsLegacyFix
McLaunchArgumentsJvmNew->>ModBase: Log("[Launch] 启用 LegacyFix")
McLaunchArgumentsJvmNew->>McLaunchArgumentsJvmNew: dataList.Add(-javaagent:"legacyfix.jar")
McLaunchArgumentsJvmNew->>McLaunchArgumentsJvmNew: [instance.releaseTime < 2011-05-25] dataList.Add(-Djava.util.Arrays.useLegacyMergeSort=true)
end
McLaunchArgumentsReplace->>McLaunchNeedsLegacyFix: McLaunchNeedsLegacyFix(instance)
McLaunchNeedsLegacyFix-->>McLaunchArgumentsReplace: needsLegacyFix
alt needsLegacyFix
McLaunchArgumentsReplace->>ModBase: GetResourceStream("Resources/legacyfix.jar")
McLaunchArgumentsReplace->>ModBase: WriteFile("legacyfix.jar", stream)
McLaunchArgumentsReplace->>McLaunchArgumentsReplace: cpStrings.Add("legacyfix.jar")
else LegacyFix disabled
McLaunchNeedsLegacyFix->>ModBase: Log("[Launch] LegacyFix 已被禁用")
end
文件级变更
技巧与命令与 Sourcery 交互
自定义你的体验访问你的 控制面板 来：
获取帮助Original review guide in EnglishReviewer's GuideReplace the outdated RetroWrapper-based compatibility layer with LegacyFix, wiring it into launch-time argument construction, resource extraction, and configuration toggles, plus updating related UI bindings and localization keys. Sequence diagram for LegacyFix integration in Minecraft launchsequenceDiagram
participant McLaunchArgumentsJvmNew
participant McLaunchArgumentsReplace
participant McLaunchNeedsLegacyFix
participant Config
participant ModBase
McLaunchArgumentsJvmNew->>McLaunchNeedsLegacyFix: McLaunchNeedsLegacyFix(instance)
McLaunchNeedsLegacyFix->>Config: Launch.DisableLF
McLaunchNeedsLegacyFix->>Config: Instance.DisableLF[mc.PathInstance]
McLaunchNeedsLegacyFix-->>McLaunchArgumentsJvmNew: needsLegacyFix
alt needsLegacyFix
McLaunchArgumentsJvmNew->>ModBase: Log("[Launch] 启用 LegacyFix")
McLaunchArgumentsJvmNew->>McLaunchArgumentsJvmNew: dataList.Add(-javaagent:"legacyfix.jar")
McLaunchArgumentsJvmNew->>McLaunchArgumentsJvmNew: [instance.releaseTime < 2011-05-25] dataList.Add(-Djava.util.Arrays.useLegacyMergeSort=true)
end
McLaunchArgumentsReplace->>McLaunchNeedsLegacyFix: McLaunchNeedsLegacyFix(instance)
McLaunchNeedsLegacyFix-->>McLaunchArgumentsReplace: needsLegacyFix
alt needsLegacyFix
McLaunchArgumentsReplace->>ModBase: GetResourceStream("Resources/legacyfix.jar")
McLaunchArgumentsReplace->>ModBase: WriteFile("legacyfix.jar", stream)
McLaunchArgumentsReplace->>McLaunchArgumentsReplace: cpStrings.Add("legacyfix.jar")
else LegacyFix disabled
McLaunchNeedsLegacyFix->>ModBase: Log("[Launch] LegacyFix 已被禁用")
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - 我发现了 1 个问题，并留下了一些整体性的反馈：
- 在
McLaunchNeedsLegacyFix中，即使needs为 false 也会打印日志[Launch] 启用 LegacyFix，这可能会造成误导；建议只在实际需要 LegacyFix 时再记录这条日志，并在不需要时单独记录另一条日志。 - 配置项的键从
LaunchAdvanceDisableRW/VersionAdvanceDisableRW改成了LaunchAdvanceDisableLF/VersionAdvanceDisableLF；如果你想保留现有用户的设置，就应该保持持久化用的键名不变，只重命名属性名，或者新增一条迁移路径。
供 AI Agent 使用的提示
Please address the comments from this code review:
## Overall Comments
- 在 `McLaunchNeedsLegacyFix` 中，即使 `needs` 为 false 也会打印日志 `[Launch] 启用 LegacyFix`，这可能会造成误导；建议只在实际需要 LegacyFix 时再记录这条日志，并在不需要时单独记录另一条日志。
- 配置项的键从 `LaunchAdvanceDisableRW` / `VersionAdvanceDisableRW` 改成了 `LaunchAdvanceDisableLF` / `VersionAdvanceDisableLF`；如果你想保留现有用户的设置，就应该保持持久化用的键名不变，只重命名属性名，或者新增一条迁移路径。
## Individual Comments
### Comment 1
<location path="PCL.Core/App/Config.cs" line_range="520-523" />
<code_context>
+ /// 禁用 LegacyFix
/// </summary>
- [ConfigItem<bool>("LaunchAdvanceDisableRW", false, ConfigSource.Local)] public partial bool DisableRw { get; set; }
+ [ConfigItem<bool>("LaunchAdvanceDisableLF", false, ConfigSource.Local)] public partial bool DisableLF { get; set; }
/// <summary>
</code_context>
<issue_to_address>
**issue (bug_risk):** 将配置键从 `...DisableRW` 重命名为 `...DisableLF` 可能会破坏现有用户的设置。
这些键之前是以 `LaunchAdvanceDisableRW` / `VersionAdvanceDisableRW` 的形式持久化的，现在改成 `...DisableLF` 后，之前禁用了 RetroWrapper 的用户在升级后会丢失该偏好设置，并且可能在不知情的情况下被启用 LegacyFix。如果你需要保持向后兼容，要么保留原始的键名，只重命名属性名，要么添加迁移逻辑，从旧键读取值并映射到新键。
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据你的反馈改进后续的 Review。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- In
McLaunchNeedsLegacyFix, the log message[Launch] 启用 LegacyFixis emitted even whenneedsis false, which can be misleading; consider logging only when LegacyFix is actually required and perhaps adding a separate log when it is not needed. - The config item keys were changed from
LaunchAdvanceDisableRW/VersionAdvanceDisableRWtoLaunchAdvanceDisableLF/VersionAdvanceDisableLF; if you want to preserve existing user settings, keep the persisted key strings the same and only rename the property names or add a migration path.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `McLaunchNeedsLegacyFix`, the log message `[Launch] 启用 LegacyFix` is emitted even when `needs` is false, which can be misleading; consider logging only when LegacyFix is actually required and perhaps adding a separate log when it is not needed.
- The config item keys were changed from `LaunchAdvanceDisableRW` / `VersionAdvanceDisableRW` to `LaunchAdvanceDisableLF` / `VersionAdvanceDisableLF`; if you want to preserve existing user settings, keep the persisted key strings the same and only rename the property names or add a migration path.
## Individual Comments
### Comment 1
<location path="PCL.Core/App/Config.cs" line_range="520-523" />
<code_context>
+ /// 禁用 LegacyFix
/// </summary>
- [ConfigItem<bool>("LaunchAdvanceDisableRW", false, ConfigSource.Local)] public partial bool DisableRw { get; set; }
+ [ConfigItem<bool>("LaunchAdvanceDisableLF", false, ConfigSource.Local)] public partial bool DisableLF { get; set; }
/// <summary>
</code_context>
<issue_to_address>
**issue (bug_risk):** Renaming the config keys from `...DisableRW` to `...DisableLF` may break existing user settings.
These keys were previously persisted as `LaunchAdvanceDisableRW` / `VersionAdvanceDisableRW`, and changing them to `...DisableLF` means existing users who disabled RetroWrapper will lose that preference after upgrade and may unexpectedly have LegacyFix enabled. If you need backward compatibility, either keep the original key names and only rename the property, or add a migration that reads the old keys and maps their values to the new ones.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9960a5a0b2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// 禁用 LegacyFix | ||
| /// </summary> | ||
| [ConfigItem<bool>("LaunchAdvanceDisableRW", false, ConfigSource.Local)] public partial bool DisableRw { get; set; } | ||
| [ConfigItem<bool>("LaunchAdvanceDisableLF", false, ConfigSource.Local)] public partial bool DisableLF { get; set; } |
There was a problem hiding this comment.
This rename removes the generated DisableRw members, but PageSetupLaunch.xaml.cs and PageInstanceSetup.xaml.cs still reference Config.Launch.DisableRw / Config.Instance.DisableRw and CheckAdvanceDisableRW. After source generation and XAML name generation, those symbols no longer exist, so the project will not compile until the remaining code-behind references are updated to the new DisableLF/CheckAdvanceDisableLF names.
Useful? React with 👍 / 👎.
| if (McLaunchNeedsLegacyFix(instance)) | ||
| { | ||
| var legacyFixPath = Path.Combine(ModBase.pathPure, "legacyfix.jar"); | ||
| dataList.Add("-javaagent:\"" + legacyFixPath + "\""); |
There was a problem hiding this comment.
This -javaagent is only added in McLaunchArgumentsJvmNew(), but the 1.5.x-and-below versions this feature targets are routed through McLaunchArgumentsJvmOld() when their JSON only has minecraftArguments. The later classpath copy only places legacyfix.jar on -cp; it does not invoke the JAR's Premain-Class, so those legacy versions launch without any LegacyFix patches.
Useful? React with 👍 / 👎.
| @@ -1,4 +1,4 @@ | |||
| <ResourceDictionary | |||
| <ResourceDictionary | |||
There was a problem hiding this comment.
The file now starts with two UTF-8 BOMs (EF BB BF EF BB BF), so XML readers treat the second marker as content before the root element. When the English ResourceDictionary is parsed or LocalizationTest.LoadKeyList calls XDocument.Load, the file is not well-formed and English localization loading fails before any keys can be read.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 932c3c0c70
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ModBase.Log("[Launch] LegacyFix 已被禁用"); | ||
| return false; | ||
| } | ||
| var needs = mc.releaseTime < new DateTime(2013, 6, 25); |
There was a problem hiding this comment.
For custom/unknown instances that omit releaseTime, McInstance sets releaseTime to 1970-01-01 (McInstance.cs lines 162-163), so this date-only check now returns true even though the version was not identified as <=1.5.x. That makes PCL copy/inject LegacyFix for arbitrary modern/custom JSONs, and in the new-JVM-args path adds a -javaagent, which can break launch or alter the classpath unexpectedly; keep the existing known-date/Drop guard before treating the instance as legacy.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8d41d3a07
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ModBase.Log("[Launch] LegacyFix 已被禁用"); | ||
| return false; | ||
| } | ||
| return true; |
There was a problem hiding this comment.
Even for known 1.6+ or current releases, this helper falls through to return true, and both callers then extract/inject legacyfix.jar (the new JVM path adds it as -javaagent). That makes the LegacyFix agent run for normal modern instances despite the UI text saying it is for 1.5.x and below, which can break launches or alter behavior outside the intended legacy scope; keep the old version/Drop guard before returning true.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 636400f52d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// 禁用 LegacyFix | ||
| /// </summary> | ||
| [ConfigItem<bool>("LaunchAdvanceDisableRW", false, ConfigSource.Local)] public partial bool DisableRw { get; set; } | ||
| [ConfigItem<bool>("LaunchAdvanceDisableLF", false, ConfigSource.Local)] public partial bool DisableLF { get; set; } |
There was a problem hiding this comment.
When upgrading a profile where LaunchAdvanceDisableRW is already true, this renamed ConfigItem reads the new LaunchAdvanceDisableLF key with the default false instead, and the old key is no longer referenced anywhere. That silently re-enables legacy-wrapper injection for users who explicitly disabled it for old-version launches; please migrate or alias the old global/per-instance keys before defaulting the new LegacyFix setting to enabled.
Useful? React with 👍 / 👎.
|
要写鸣谢吗？ |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11ecdf3abf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (McLaunchNeedsLegacyFix(instance)) | ||
| { | ||
| var legacyFixPath = Path.Combine(ModBase.pathPure, "legacyfix.jar"); | ||
| dataList.Add("-javaagent:\"" + legacyFixPath + "\""); |
There was a problem hiding this comment.
When a legacy instance reaches this new-JVM-args path with Java Wrapper enabled (the block immediately below, on UTF-8 code pages) or with a normal Minecraft main class, the JVM command seen by the bundled LegacyFixAgent is not one of the launchers its required launcher patch accepts (LegacyFixLauncher, Prism/MultiMC, or LaunchWrapper). The agent exits on unknown launchers during premain, so adding it as a bare -javaagent can abort these launches before Minecraft starts; this path needs to invoke/identify LegacyFix’s launcher instead of only attaching the agent.
Useful? React with 👍 / 👎.
2 participants
RetroWrapper 太久没更新了，用 LegacyFix 更有保障
文件版本：https://github.com/betacraftuk/legacyfix/actions/runs/25253333468
#3155 不小心签错名了，为了美观我选择新建一个新分支（
Summary by Sourcery
将过时的 RetroWrapper 集成替换为 LegacyFix，用于启动旧版本的 Minecraft。
新功能：
改进：
Original summary in English
Summary by Sourcery
Replace the outdated RetroWrapper integration with LegacyFix for launching older Minecraft versions.
New Features:
Enhancements: