重构启动器配置模型#6122
Conversation
|
这个 tooltip 可以写得详细一点。比如「当前状态：继承全局设置\n点击切换为覆盖全局设置」。 
|
|
「管理所有全局游戏设置」我一开始理解错了，点进去后发现是多 profile 的机制。 要不考虑用个更合适的词？比如配置档啥的 (这样英文也好写成 profile)。 
|
Compare migration receipts against legacy source content instead of recorded file state so unchanged migrated data is not replayed after the current file is deleted.
…orted version flags
…nd logFontSize with legacy migration
…nly view Replace the direct merge of user and local profiles with a mutable backing list and an unmodifiable view. Introduce `addGameDirectory`, `removeGameDirectory`, and `hasGameDirectoryId` in `SettingsManager`, and delegate `Profiles` methods accordingly. Update `ProfilePage` and `ModpackHelper` to use the new `Profiles.addProfile`. Add test verifying shadowed profiles remain in backing stores.
…es class Shift the merged profile view construction, profile addition, and profile removal from `SettingsManager` into `Profiles`, where the profile list is owned. Expose `localGameDirectories()` and `userGameDirectories()` accessors in `SettingsManager` for the backing stores and delegate profile operations to `Profiles`. Update tests accordingly.
- create default profiles when user and local game directories load empty - ensure the merged profile view is rebuilt from initialized stores - replace the nil profile ID test with coverage for empty-store loading - preserve expected startup behavior instead of leaving profiles missing
- pass newly created local and user directories into profile loading - keep freshly added game directory entries from being dropped at startup - align profile nullability annotations and related tests with the updated loading behavior
… for better encapsulation
… newly created state
…g previousSelectedProfile
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the launcher's configuration and settings architecture, replacing the legacy Config and ConfigHolder with a modular, schema-validated settings model (SettingsManager, LauncherSettings, UserSettings, etc.) and introducing robust migration paths for legacy formats. The review feedback highlights a critical serialization issue in JsonSettingFile.java where using JsonUtils.GSON instead of LauncherSettings.SETTINGS_GSON fails to serialize JavaFX properties. Additionally, the reviewer identified a potential memory leak in ComponentSublistWrapper.java due to strong layout listeners, and a performance concern in Launcher.java regarding blocking disk I/O operations performed directly on the JavaFX Application Thread inside a binding.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| value.setBackupOnNextSave(false); | ||
| SettingFileUtils.backupInvalidConfig(location); | ||
| } | ||
| FileSaver.save(location, JsonUtils.GSON.toJson(value, type)); |
There was a problem hiding this comment.
Using JsonUtils.GSON to serialize the settings object will fail to correctly serialize JavaFX properties (such as Property, ObservableList, etc.) because JsonUtils.GSON does not have the JavaFX property adapters registered. This will result in corrupted or incomplete JSON files when saving.
Please use LauncherSettings.SETTINGS_GSON instead, which is configured with JavaFxPropertyTypeAdapterFactory and other necessary type adapters.
| FileSaver.save(location, JsonUtils.GSON.toJson(value, type)); | |
| FileSaver.save(location, LauncherSettings.SETTINGS_GSON.toJson(value, type)); |
| InvalidationListener listener = observable -> updateExpandedContentHeight(sublist); | ||
| node.layoutBoundsProperty().addListener(listener); | ||
| if (node instanceof Parent parent) { | ||
| parent.needsLayoutProperty().addListener(listener); | ||
| } | ||
| contentLayoutListeners.put(node, listener); |
There was a problem hiding this comment.
Adding strong listeners to node.layoutBoundsProperty() and parent.needsLayoutProperty() will cause a memory leak. Since nodes are part of the persistent sublist.getContent() model, these strong listeners will prevent ComponentSublistWrapper (and the entire Skin/View hierarchy) from being garbage collected when the wrapper is replaced or the Skin is disposed.
Wrapping the listeners in WeakInvalidationListener solves this memory leak.
| InvalidationListener listener = observable -> updateExpandedContentHeight(sublist); | |
| node.layoutBoundsProperty().addListener(listener); | |
| if (node instanceof Parent parent) { | |
| parent.needsLayoutProperty().addListener(listener); | |
| } | |
| contentLayoutListeners.put(node, listener); | |
| InvalidationListener listener = observable -> updateExpandedContentHeight(sublist); | |
| node.layoutBoundsProperty().addListener(new javafx.beans.WeakInvalidationListener(listener)); | |
| if (node instanceof Parent parent) { | |
| parent.needsLayoutProperty().addListener(new javafx.beans.WeakInvalidationListener(listener)); | |
| } | |
| contentLayoutListeners.put(node, listener); |
| HMCLCacheRepository.REPOSITORY.directoryProperty().bind(Bindings.createStringBinding(() -> { | ||
| String commonDirectory = SettingsManager.settings().getResolvedCommonDirectory(); | ||
| if (commonDirectory != null && FileUtils.canCreateDirectory(commonDirectory)) { | ||
| return commonDirectory; | ||
| } else { | ||
| return LauncherSettings.getDefaultCommonDirectory(); | ||
| } | ||
| }, SettingsManager.settings().commonDirectoryProperty(), SettingsManager.settings().commonDirectoryTypeProperty())); |
There was a problem hiding this comment.
Performing blocking disk I/O operations like FileUtils.canCreateDirectory(commonDirectory) inside a JavaFX binding (Bindings.createStringBinding(...)) can cause UI lag or temporary freezes because bindings are evaluated on the JavaFX Application Thread.
Consider performing this check asynchronously or caching the result of the directory validation outside of the binding to keep the UI thread responsive.
…newer HMCL versions
- replace the warning-only dialog with a confirmation flow before overwriting read-only account storage - remove the stale account entry after forcing an overwrite so the list reflects the new storage state - update account list page and item UI wiring to support the revised restricted-account interaction flow - improve the account management experience when offline authentication is restricted or storage cannot be written safely
- move page-specific bindings and drag-and-drop handling out of `Controllers` and into the corresponding page classes - simplify controller setup by replacing verbose lazy initializers with constructor references where possible - keep profile and account state management closer to the UI that consumes it to reduce central coupling and improve maintainability
- add overwrite actions for instance and preset settings when the current format cannot be saved safely - keep unsupported settings pages read-only while exposing a recovery path instead of forcing manual file edits - back up existing instance settings before replacement to reduce the risk of data loss during migration
…rectories are saved
- save migrated instance settings synchronously so migration receipts are not written before the new game settings reach disk - switch shutdown-critical settings writes to synchronous saves to reduce data loss when the process exits immediately afterward - treat unreadable account storage as unavailable and disable further writes instead of reporting read-write access for a broken file
![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: 8309b23ff9
ℹ️ 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".
| profile.setName(LocalizedText.plain(txtProfileName.getText())); | ||
| if (StringUtils.isNotBlank(getLocation())) { | ||
| profile.setGameDir(Path.of(getLocation())); | ||
| profile.setPath(createPortableLocation()); |
There was a problem hiding this comment.
When an existing profile is saved after changing between an absolute path and a relative path, this branch only mutates the Profile already held by its current backing store. New profiles below are routed to user vs. local storage based on newProfile.getPath().isAbsolute(), so edits that cross that boundary leave the profile in the wrong JSON file and skip the read-only/overwrite checks, e.g. a shared profile can become workspace-relative but still be saved in user-game-directories.json.
Useful? React with 👍 / 👎.
4 participants
No description provided.