Added checkboxes to include various command line switches #3431
Conversation
WalkthroughA new feature adds UI checkboxes in game settings to automatically include command-line parameters for statistics reporting and sound control. A new parameter resolver dynamically tracks enabled parameters based on preferences and integrates them into the game launch command. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SettingsUI as Settings UI
participant Prefs as ForgedAlliancePrefs
participant Resolver as GameLaunchLocalParametersResolver
participant Builder as LaunchCommandBuilder
participant Launcher as ForgedAllianceLaunchService
User->>SettingsUI: Toggle skipStatistic/noGameSound
SettingsUI->>Prefs: Update property (bidirectional binding)
Prefs->>Resolver: Property change listener triggered
Resolver->>Resolver: Add/remove parameter from set
Launcher->>Builder: Build launch command
Builder->>Resolver: getParameters()
Resolver-->>Builder: Return active parameters
Builder->>Builder: Append parameter commands to base command
Builder-->>Launcher: Return complete command
Launcher-->>User: Launch game with parameters
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3431 +/- ##
=============================================
+ Coverage 58.26% 58.38% +0.11%
- Complexity 3924 3947 +23
=============================================
Files 581 579 -2
Lines 19313 19349 +36
Branches 1032 1036 +4
=============================================
+ Hits 11253 11297 +44
Misses 7553 7553
+ Partials 507 499 -8
... and 27 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/main/java/com/faforever/client/fa/ForgedAllianceLaunchService.java(3 hunks)src/main/java/com/faforever/client/fa/GameLaunchLocalParameter.java(1 hunks)src/main/java/com/faforever/client/fa/GameLaunchLocalParametersResolver.java(1 hunks)src/main/java/com/faforever/client/fa/LaunchCommandBuilder.java(4 hunks)src/main/java/com/faforever/client/preferences/ForgedAlliancePrefs.java(2 hunks)src/main/java/com/faforever/client/preferences/ui/SettingsController.java(2 hunks)src/main/resources/i18n/messages.properties(1 hunks)src/main/resources/theme/settings/settings.fxml(1 hunks)src/test/java/com/faforever/client/fa/ForgedAllianceLaunchServiceTest.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (11)
src/main/resources/i18n/messages.properties (1)
872-875: LGTM! Localization keys follow existing conventions.The new i18n keys for the game settings are properly formatted and align with the existing pattern for Forged Alliance settings.
src/test/java/com/faforever/client/fa/ForgedAllianceLaunchServiceTest.java (1)
32-33: LGTM! Test infrastructure updated appropriately.The mock field addition properly supports testing the new dependency introduced in
ForgedAllianceLaunchService.src/main/java/com/faforever/client/preferences/ui/SettingsController.java (2)
149-150: LGTM! CheckBox fields properly declared.The new checkbox fields follow the existing pattern in this controller.
337-338: LGTM! Bidirectional bindings correctly implemented.The bindings properly connect the UI checkboxes to the underlying preference properties, ensuring changes persist and are reflected in the UI.
src/main/java/com/faforever/client/fa/ForgedAllianceLaunchService.java (1)
46-46: LGTM! Resolver integration is correct.The
GameLaunchLocalParametersResolverdependency is properly injected and used to augment the launch command with dynamic local parameters.Also applies to: 130-130
src/main/java/com/faforever/client/fa/LaunchCommandBuilder.java (2)
53-53: LGTM! Field and setter follow builder pattern.The new field and fluent setter method properly extend the builder to support dynamic local parameters.
Also applies to: 198-201
217-219: LGTM! Dynamic parameter handling implemented correctly.The refactoring from hardcoded parameters to a dynamic list is well-executed:
- Maintains existing behavior (INIT_FILE, NO_BUG_REPORT always included)
- Safely handles null
gameLaunchLocalParameterswithList.of()- Delegates command construction to each parameter's
getCommand()methodThis provides flexibility while preserving backward compatibility.
src/main/java/com/faforever/client/fa/GameLaunchLocalParameter.java (1)
8-30: LGTM! Enum design is clean and correct.The
GameLaunchLocalParameterenum provides a type-safe way to represent launch parameters:
- Clear constructor overloading for parameters with/without values
getCommand()correctly returns either[name, value]or[name]based on whether a value is present- The four constants map to the command-line switches mentioned in the linked issue (#3425)
This is a good abstraction that encapsulates the command construction logic.
src/main/java/com/faforever/client/preferences/ForgedAlliancePrefs.java (3)
189-195: LGTM - property accessors follow standard JavaFX pattern.The property accessor methods correctly return the underlying BooleanProperty fields, following the established pattern in this class. However, the effectiveness of these methods depends on resolving the naming issue flagged for line 42.
197-203: LGTM - setter methods implemented correctly.The setter methods follow the standard JavaFX pattern and are consistent with other boolean property setters in this class. Note that renaming the
skipStatisticproperty will require updating the setter method name as well.
205-211: LGTM - getter methods follow JavaBean conventions.The boolean getter methods correctly use the
isprefix and return the property values, consistent with JavaBean conventions and the existing code style in this class.
| import java.net.URI; | ||
| import java.nio.file.Files; | ||
| import java.nio.file.Path; | ||
| import java.util.ArrayList; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify if this import is needed.
The ArrayList import was added but may be unused. The getParameters() method returns a List, and no explicit ArrayList instantiation appears in this file.
🏁 Script executed:
#!/bin/bash
# Check if ArrayList is used in ForgedAllianceLaunchService
rg -n '\bArrayList\b' --type java src/main/java/com/faforever/client/fa/ForgedAllianceLaunchService.javaLength of output: 30
Remove the unused ArrayList import at line 22.
The ArrayList class is imported but never used in this file. Since getParameters() returns a List interface and there's no explicit ArrayList instantiation, this import should be removed.
🤖 Prompt for AI Agents
In src/main/java/com/faforever/client/fa/ForgedAllianceLaunchService.java around
line 22, the import statement for java.util.ArrayList is unused; remove that
import so only required imports remain (keep java.util.List if used) and then
recompile to ensure there are no compile errors from the removal.
| @Component | ||
| public class GameLaunchLocalParametersResolver { | ||
|
|
||
| private final Set<GameLaunchLocalParameter> parameters = new HashSet<>(); |
There was a problem hiding this comment.
Consider thread safety for the parameters set.
The HashSet at line 16 is not thread-safe. While JavaFX properties typically fire on the JavaFX Application Thread, there's no explicit guarantee here, and getParameters() can be called from any thread (e.g., during game launch).
Use a thread-safe collection:
+ import java.util.concurrent.ConcurrentHashMap;
+ import java.util.Collections;
- private final Set<GameLaunchLocalParameter> parameters = new HashSet<>();
+ private final Set<GameLaunchLocalParameter> parameters = ConcurrentHashMap.newKeySet();This ensures safe concurrent access without explicit synchronization.
Also applies to: 29-35, 37-39
🤖 Prompt for AI Agents
In src/main/java/com/faforever/client/fa/GameLaunchLocalParametersResolver.java
around line 16 (also applies to lines 29-35 and 37-39), the field initialization
uses a non-thread-safe HashSet which can be accessed from multiple threads;
replace it with a thread-safe set implementation (for example
Collections.synchronizedSet(new HashSet<>()) or ConcurrentHashMap.newKeySet() or
CopyOnWriteArraySet) and update any other similar set fields/usages listed to
the same thread-safe implementation; when exposing the set via getters, return
either an unmodifiable view or a defensive copy to avoid exposing internal
mutable state across threads.
| public GameLaunchLocalParametersResolver(Preferences preferences) { | ||
| ForgedAlliancePrefs faPrefs = preferences.getForgedAlliance(); | ||
|
|
||
| subscribe(faPrefs.skipStatisticProperty(), GameLaunchLocalParameter.GAME_OVER); | ||
| subscribe(faPrefs.noGameSoundProperty(), GameLaunchLocalParameter.NO_SOUND); | ||
| } |
There was a problem hiding this comment.
Critical: Initial preference values are not captured.
The subscribe() method only listens for changes to the property, not the current value. If a preference is already enabled when the application starts (e.g., user previously checked "skip statistics"), the corresponding parameter won't be added to the set, and the launch command won't include it.
Fix by using addListener() with immediate triggering:
public GameLaunchLocalParametersResolver(Preferences preferences) {
ForgedAlliancePrefs faPrefs = preferences.getForgedAlliance();
- subscribe(faPrefs.skipStatisticProperty(), GameLaunchLocalParameter.GAME_OVER);
- subscribe(faPrefs.noGameSoundProperty(), GameLaunchLocalParameter.NO_SOUND);
+ subscribeAndInitialize(faPrefs.skipStatisticProperty(), GameLaunchLocalParameter.GAME_OVER);
+ subscribeAndInitialize(faPrefs.noGameSoundProperty(), GameLaunchLocalParameter.NO_SOUND);
}
- private void subscribe(BooleanProperty property, GameLaunchLocalParameter parameter) {
- property.subscribe(newValue -> resolveParameter(parameter, newValue));
+ private void subscribeAndInitialize(BooleanProperty property, GameLaunchLocalParameter parameter) {
+ property.addListener((observable, oldValue, newValue) -> resolveParameter(parameter, newValue));
+ resolveParameter(parameter, property.get()); // Initialize with current value
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main/java/com/faforever/client/fa/GameLaunchLocalParametersResolver.java
around lines 18 to 23, the current code only calls subscribe(...) which reacts
to future changes and therefore misses applying preference values that are
already true at startup; replace the subscribe(...) calls with adding a
ChangeListener (or equivalent addListener) that updates the
GameLaunchLocalParameter set and immediately invoke that listener (or manually
check the property.get() and apply the parameter) so the current preference
value is captured on initialization as well as on subsequent changes.
| private final BooleanProperty skipStatistic = new SimpleBooleanProperty(false); | ||
| private final BooleanProperty noGameSound = new SimpleBooleanProperty(false); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Misleading property name: skipStatistic does not convey "exit on game over" functionality.
Based on the linked issue #3425 and the AI summary indicating this maps to the GAME_OVER parameter (corresponding to the /exitongameover command-line switch), the name skipStatistic is misleading. The current name suggests disabling statistics collection rather than automatically exiting when the game ends.
Additionally, these properties lack JavaDoc comments explaining their purpose and which command-line flags they correspond to.
Apply this diff to rename the property and add documentation:
+ /**
+ * When enabled, the game will automatically exit when the match ends.
+ * Corresponds to the /exitongameover command-line switch.
+ */
- private final BooleanProperty skipStatistic = new SimpleBooleanProperty(false);
+ private final BooleanProperty exitOnGameOver = new SimpleBooleanProperty(false);
+ /**
+ * When enabled, the game launches without sound.
+ * Corresponds to the /nosound command-line switch.
+ * Useful for working around sound issues.
+ */
private final BooleanProperty noGameSound = new SimpleBooleanProperty(false);Note: If you rename this property, ensure all references in GameLaunchLocalParametersResolver, SettingsController, and related files are updated accordingly.
Run the following script to find all references to skipStatistic:
🏁 Script executed:
#!/bin/bash
# Find all references to skipStatistic that need to be renamed
rg -n --type=java 'skipStatistic' -C2Length of output: 4928
Misleading property name: skipStatistic does not convey "exit on game over" functionality.
The property name is misleading. While it maps to GameLaunchLocalParameter.GAME_OVER and the i18n label correctly states "Immediately exit the game when it is over", the property name skipStatistic suggests disabling statistics collection rather than exiting after game completion. Additionally, the i18n description ("Game statistic will be skipped") contradicts the actual functionality.
Required changes across 4 files:
- Rename property field and all accessor methods:
skipStatistic→exitOnGameOverinForgedAlliancePrefs.java(lines 42, 189-191, 197-199, 205-207) - Update UI binding in
SettingsController.java(lines 149, 337) - Update
GameLaunchLocalParametersResolver.java(line 21) - Fix i18n keys and descriptions in
messages.properties(lines 872-873) - Add JavaDoc explaining the property's purpose and command-line switch correspondence
🤖 Prompt for AI Agents
In src/main/java/com/faforever/client/preferences/ForgedAlliancePrefs.java
around lines 42 and 189-191,197-199,205-207 rename the BooleanProperty field and
all its accessors from skipStatistic to exitOnGameOver, add JavaDoc to the
property explaining it controls "Immediately exit the game when it is over" and
that it maps to the GameLaunchLocalParameter.GAME_OVER / corresponding
command-line switch; in
src/main/java/com/faforever/client/ui/SettingsController.java update the UI
binding and references at lines ~149 and ~337 to use exitOnGameOver; in
src/main/java/com/faforever/client/launcher/GameLaunchLocalParametersResolver.java
update the parameter mapping at line 21 to use exitOnGameOver/GAME_OVER; and in
src/main/resources/i18n/messages.properties update the key and description lines
(~872-873) so the label and description reflect "Immediately exit the game when
it is over" (remove/replace "Game statistic will be skipped"). Ensure method
names (getter/setter/property) and usages are consistently renamed project-wide
and update any imports or tests that reference the old name.
| </rowConstraints> | ||
| <children> | ||
| <Label contentDisplay="RIGHT" maxWidth="1.7976931348623157E308" | ||
| styleClass="setting-title" text="Game settings" GridPane.rowIndex="0"/> |
There was a problem hiding this comment.
Replace hardcoded text with i18n key.
Line 703 contains hardcoded text "Game settings" instead of an i18n key. This breaks localization consistency—all other labels in the file use keys like %settings.fa.title.
Add an i18n key to messages.properties:
+settings.fa.gameSettings = Game settingsThen update the FXML:
- styleClass="setting-title" text="Game settings" GridPane.rowIndex="0"/>
+ styleClass="setting-title" text="%settings.fa.gameSettings" GridPane.rowIndex="0"/>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| styleClass="setting-title" text="Game settings" GridPane.rowIndex="0"/> | |
| styleClass="setting-title" text="%settings.fa.gameSettings" GridPane.rowIndex="0"/> |
🤖 Prompt for AI Agents
In src/main/resources/theme/settings/settings.fxml around line 703 the label
uses hardcoded text "Game settings" which breaks localization; replace it with
an i18n key (for example %settings.game.title) by adding that key and its
translation to src/main/resources/messages.properties, then update the FXML to
use the i18n key (replace text="Game settings" with text="%settings.game.title")
so it follows the existing localization pattern.
There was a problem hiding this comment.
I don't quite understand the purpose of the game resolver and actively subscribing to the prefs object.
We only need to care what the preferences are when we launch the game right? I would think more that the prefs object would just be an observable set of the enums. Then we can just get that.
Then in the settings controller we can have the add remove logic since it is really more a UI translation rather than embedding that into the preferences.
It also gives a little more flexibility and less proliferation of preferences fields.
You can also inject the ForgedAlliancePrefs directly
There was a problem hiding this comment.
We only need to care what the preferences are when we launch the game right?
That is right, you are right! I wrote the code, which was the first thing that came to mind. My implementation of the code is crap. I will redo it.
There was a problem hiding this comment.
I don't think these enums need parameters. In the launcher they could just be in a switch statement and should only be used in one place.
| command.addAll(Arrays.asList( | ||
| "/init", ForgedAlliancePrefs.INIT_FILE_NAME, | ||
| "/nobugreport" | ||
| )); | ||
|
|
||
| List<GameLaunchLocalParameter> defaultGameParameters = Lists.newArrayList(INIT_FILE, NO_BUG_REPORT); | ||
| defaultGameParameters.addAll(gameLaunchLocalParameters != null ? gameLaunchLocalParameters : List.of()); | ||
| defaultGameParameters.forEach(parameter -> command.addAll(parameter.getCommand())); |
There was a problem hiding this comment.
Also I wouldn't add always present options to the enums. It should just be optional options.
Closes #3425
Summary by CodeRabbit