Conversation
|
关于 log4j,我们正在考虑是否可以不再打包 log4j2.xml,而是遵循默认的配置。 |
Co-authored-by: 3gf8jv4dv <3gf8jv4dv@gmail.com>
|
我觉得这个选项名字不是很好,叫「输出调试日志」可能会更合适。而且我感觉它应该放在「查看日志」的下面,而不是高级选项里。 |
| FXUtils.unbind(txtServerIP, lastVersionSetting.serverIpProperty()); | ||
| chkAutoAllocate.selectedProperty().unbindBidirectional(lastVersionSetting.autoMemoryProperty()); | ||
| chkFullscreen.selectedProperty().unbindBidirectional(lastVersionSetting.fullscreenProperty()); | ||
| useDebugLogOutputPane.selectedProperty().unbindBidirectional(versionSetting.useDebugLogOutputProperty()); |
There was a problem hiding this comment.
你这里 unbind 错东西了。unbind 的目标应该是 lastVersionSetting 的属性,而不是 versionSetting 的属性 。
# Conflicts: # HMCLCore/src/main/java/org/jackhuang/hmcl/launch/DefaultLauncher.java
| InputStream source; | ||
| if (GameVersionNumber.asGameVersion(repository.getGameVersion(version)).compareTo("1.12") < 0) { | ||
| if (options.isShowDebugLog()) { | ||
| source = DefaultLauncher.class.getResourceAsStream("/assets/game/log4j2-debug.xml"); |
There was a problem hiding this comment.
你确定你的这个配置文件对 Minecraft 1.7+ 都生效吗?我怎么看到这个文件里用了高版本 Log4j 才有的功能?
| String sourcePath = "/assets/game/log4j2-"; | ||
|
|
||
| if (GameVersionNumber.asGameVersion(repository.getGameVersion(version)).compareTo("1.12") < 0) { | ||
| sourcePath += "1.7"; |
There was a problem hiding this comment.
不建议使用 += 拼接字符串,应当用 StringBuilder 来拼接。
Co-authored-by: 3gf8jv4dv <3gf8jv4dv@gmail.com>
HMCLCore/src/main/java/org/jackhuang/hmcl/launch/DefaultLauncher.java
Outdated
Show resolved
Hide resolved
HMCLCore/src/main/java/org/jackhuang/hmcl/launch/DefaultLauncher.java
Outdated
Show resolved
Hide resolved
HMCL/src/main/java/org/jackhuang/hmcl/game/HMCLGameRepository.java
Outdated
Show resolved
Hide resolved
|
|
||
| String formatMsgNoLookups = res.addDefault("-Dlog4j2.formatMsgNoLookups=", "true"); | ||
| if (!"-Dlog4j2.formatMsgNoLookups=false".equals(formatMsgNoLookups) && isUsingLog4j()) { | ||
| if (options.isEnableDebugLogOutput()) { |
There was a problem hiding this comment.
为什么要绕开添加 -Dlog4j2.formatMsgNoLookups=true 这个参数的逻辑?默认情况下这个参数是必须添加的,否则非常危险。
而且我不明白为什么也要绕开 isUsingLog4j() 的检测。对未使用 log4j 的版本添加 -Dlog4j.configurationFile 这个参数有什么意义吗?
| if (!"-Dlog4j2.formatMsgNoLookups=false".equals(formatMsgNoLookups) && isUsingLog4j()) { | ||
| res.addDefault("-Dlog4j.configurationFile=", FileUtils.getAbsolutePath(getLog4jConfigurationFile())); | ||
| if (isUsingLog4j()) { | ||
| String formatMsgNoLookups = res.addDefault("-Dlog4j2.formatMsgNoLookups=", "true"); |
There was a problem hiding this comment.
请在最外面调用此方法。该参数总是应该被默认添加,不应该依赖 isUsingLog4j 的检测。即便该 MC 版本本身没有使用 log4j2,也不排除模组依赖 log4j2 的可能性。log4j2 的这项功能存在极高的风险性,而该 JVM 参数又几乎没有负面作用,我们应当以最保险的方式添加此参数。
There was a problem hiding this comment.
Pull request overview
This PR adds a user-friendly option to enable debug-level logging output for Minecraft games, addressing the complexity users face when manually configuring Log4j2 for debugging. Previously, users had to manually download a custom log4j2.xml file and add JVM arguments, which was error-prone. The PR introduces a simple toggle in the game settings that automatically applies pre-configured debug log4j2 files.
Key Changes
- Added two new debug log4j2 configuration files (for Minecraft 1.7 and 1.12+) with debug-level logging enabled and a separate debug.log file
- Introduced
enableDebugLogOutputoption across the codebase with UI controls, serialization, and launch configuration support - Modified the launcher logic to use debug log4j2 configurations when the option is enabled
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| HMCLCore/src/main/resources/assets/game/log4j2-1.7-debug.xml | New debug-level log4j2 configuration for Minecraft 1.7-1.11, with all-level logging and dedicated debug.log file |
| HMCLCore/src/main/resources/assets/game/log4j2-1.12-debug.xml | New debug-level log4j2 configuration for Minecraft 1.12+, with all-level logging and dedicated debug.log file |
| HMCLCore/src/main/java/org/jackhuang/hmcl/launch/DefaultLauncher.java | Updated to select debug or standard log4j2 configuration based on enableDebugLogOutput option and modified condition logic to apply config when debug output is enabled |
| HMCLCore/src/main/java/org/jackhuang/hmcl/game/LaunchOptions.java | Added enableDebugLogOutput field with getter and builder setter method |
| HMCL/src/main/resources/assets/lang/I18N_zh_CN.properties | Added Chinese (Simplified) translation for the debug log output setting |
| HMCL/src/main/resources/assets/lang/I18N_zh.properties | Added Chinese (Traditional) translation for the debug log output setting |
| HMCL/src/main/resources/assets/lang/I18N.properties | Added English translation for the debug log output setting |
| HMCL/src/main/java/org/jackhuang/hmcl/ui/versions/VersionSettingsPage.java | Added UI toggle control for debug log output option in version settings page |
| HMCL/src/main/java/org/jackhuang/hmcl/setting/VersionSetting.java | Added enableDebugLogOutput property with getter, setter, and JSON serialization/deserialization support |
| HMCL/src/main/java/org/jackhuang/hmcl/game/HMCLGameRepository.java | Wired enableDebugLogOutput setting to LaunchOptions when generating launch configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public Builder setEnableDebugLogOutput(boolean u) { | ||
| options.enableDebugLogOutput = u; |
There was a problem hiding this comment.
The parameter name 'u' is unclear and inconsistent with other builder methods in this class. All other setter methods use descriptive parameter names that match the field being set (e.g., 'daemon', 'useNativeGLFW', 'useNativeOpenAL'). This parameter should be renamed to 'enableDebugLogOutput' for consistency and clarity.
| public Builder setEnableDebugLogOutput(boolean u) { | |
| options.enableDebugLogOutput = u; | |
| public Builder setEnableDebugLogOutput(boolean enableDebugLogOutput) { | |
| options.enableDebugLogOutput = enableDebugLogOutput; |
| public void setEnableDebugLogOutput(boolean u) { | ||
| this.enableDebugLogOutputProperty.set(u); |
There was a problem hiding this comment.
The parameter name 'u' is unclear and inconsistent with other setter methods in this class. All other setter methods use descriptive parameter names that match the field being set (e.g., 'showLogs' in setShowLogs). This parameter should be renamed to 'enableDebugLogOutput' for consistency and clarity.
| public void setEnableDebugLogOutput(boolean u) { | |
| this.enableDebugLogOutputProperty.set(u); | |
| public void setEnableDebugLogOutput(boolean enableDebugLogOutput) { | |
| this.enableDebugLogOutputProperty.set(enableDebugLogOutput); |
| try (InputStream input = DefaultLauncher.class.getResourceAsStream(sourcePath)) { | ||
| Files.copy(input, targetFile, StandardCopyOption.REPLACE_EXISTING); |
There was a problem hiding this comment.
The InputStream returned by getResourceAsStream could be null if the resource file is not found. The code should add a null check (similar to the pattern used in MaintainTask.java line 166) to provide a clearer error message if the bundled log4j configuration file is missing. Consider using Objects.requireNonNull(input, "Bundled log4j2 configuration is missing.") before passing it to Files.copy.
| @@ -282,6 +283,10 @@ public boolean isUseNativeOpenAL() { | |||
| return useNativeOpenAL; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
The new method isEnableDebugLogOutput() lacks JavaDoc documentation. While some similar methods like isUseNativeGLFW() also lack documentation, many other getter methods in this class have JavaDoc comments explaining their purpose (e.g., isDaemon(), isNoGeneratedJVMArgs()). Consider adding documentation like: "/** * Whether to enable debug level logging output in log4j2 configuration. */" to explain what this option does.
| /** | |
| * Whether to enable debug level logging output in log4j2 configuration. | |
| */ |
| public BooleanProperty enableDebugLogOutputProperty() { | ||
| return enableDebugLogOutputProperty; | ||
| } | ||
|
|
There was a problem hiding this comment.
The new method isEnableDebugLogOutput() lacks JavaDoc documentation while the similar method isShowLogs() above it has documentation. For consistency and clarity, consider adding documentation like: "/** * True if debug level logging should be enabled in log4j2 configuration. */" to explain what this option does.
| /** | |
| * True if debug level logging should be enabled in log4j2 configuration. | |
| */ |
在崩溃群排查错误的过程中,因为 HMCL 给游戏提供的
log4j2.xml可能会丢失一些需要的信息,需要用户手动替换一个日志级别低的 log4j2 文件让用户自己操作非常容易出错且沟通成本高,因此需要一个快捷选项方便用户使用。本 PR 在游戏高级设置里添加了相关选项,可以一键切换到预设的低级别 log4j2 配置文件
