Add validation of title and password text fields for coop games #3418
Conversation
src/main/java/com/faforever/client/game/CreateGameController.java
Outdated
Show resolved
Hide resolved
Marc-Spector
force-pushed
the
feature/validate-coop-game-title
branch
from
89ba353 to
4b12957
Compare
WalkthroughThis pull request adds ASCII-character validation to Coop game titles and passwords to match the existing validation in regular game creation. The Validator utility is refactored to replace custom null checks with Java's standard Objects.requireNonNull and adds an isAscii method for character validation. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant TitleField as Title TextField
participant Validator
participant WarningLabel
participant PlayButton
User->>TitleField: Enter non-ASCII text (e.g., "Hëllö")
TitleField->>Validator: isAscii(text)
Validator-->>TitleField: false
TitleField->>WarningLabel: setVisible(true)
WarningLabel-->>User: Display warning
TitleField->>PlayButton: setDisable(true)
PlayButton-->>User: Button disabled
User->>TitleField: Clear and enter ASCII text (e.g., "Hello")
TitleField->>Validator: isAscii(text)
Validator-->>TitleField: true
TitleField->>WarningLabel: setVisible(false)
WarningLabel-->>User: Warning hidden
TitleField->>PlayButton: setDisable(false)
PlayButton-->>User: Button enabled
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![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
🧹 Nitpick comments (10)
src/main/java/com/faforever/client/mod/ModUploadTask.java (1)
111-113: Swap logging argument order for clarityMessage says “requestId=[{}], zip filePath=[{}]” but arguments are (filePath, requestId). Swap them to avoid confusing logs.
- log.debug("Uploading mod to S3: requestId=[{}], zip filePath=[{}]", filePath, requestId); + log.debug("Uploading mod to S3: requestId=[{}], zip filePath=[{}]", requestId, filePath);src/main/java/com/faforever/client/game/CreateGameController.java (3)
197-205: Gate subscription with when(showing) and drop duplicate listenerUse when(showing) to avoid lifecycle leaks and update the create button from the same subscription; then remove the extra addListener on title.
- titleTextField.textProperty().subscribe(text -> { - lastGamePrefs.setLastGameTitle(text); - validateTitle(text); - }); + titleTextField.textProperty().when(showing).subscribe(text -> { + lastGamePrefs.setLastGameTitle(text); + validateTitle(text); + setCreateGameButtonState(); + }); @@ - JavaFxUtil.addListener(titleTextField.textProperty(), createButtonStateListener); + // Redundant after subscription calls setCreateGameButtonState()
228-235: Use Validator.isAscii for consistency (avoid ad-hoc CharsetEncoder)Unify ASCII checks with Validator.isAscii used elsewhere.
- CharsetEncoder charsetEncoder = StandardCharsets.US_ASCII.newEncoder(); if (StringUtils.isBlank(title)) { yield "game.create.titleMissing"; - } else if (!charsetEncoder.canEncode(title)) { + } else if (!Validator.isAscii(title)) { yield "game.create.titleNotAscii"; - } else if (password != null && !charsetEncoder.canEncode(password)) { + } else if (password != null && !Validator.isAscii(password)) { yield "game.create.passwordNotAscii"; } else if (featuredModListView.getSelectionModel().getSelectedItem() == null) { yield "game.create.featuredModMissing";
195-201: Apply initial validation once on initializeAfter setting initial title/password, call validateTitle(...) once so the invalid border reflects the starting state.
setLastGameTitle(); initPassword(); + validateTitle(titleTextField.getText());Also applies to: 269-270
src/main/java/com/faforever/client/util/Validator.java (1)
18-20: Make isAscii null-safeGuard against null to avoid accidental NPEs from UI bindings.
- public static boolean isAscii(String content) { - return StandardCharsets.US_ASCII.newEncoder().canEncode(content); - } + public static boolean isAscii(String content) { + return content != null && StandardCharsets.US_ASCII.newEncoder().canEncode(content); + }src/main/java/com/faforever/client/coop/CoopController.java (1)
238-243: Fix method name typo: “Ascii” not “Acsii”Rename helper and update both call sites.
- private void addListenerToUpdateInvalidSelectorStateIfTextIsNotAcsii(TextField field) { + private void addListenerToUpdateInvalidSelectorStateIfTextIsNotAscii(TextField field) { @@ - addListenerToUpdateInvalidSelectorStateIfTextIsNotAcsii(titleTextField); - addListenerToUpdateInvalidSelectorStateIfTextIsNotAcsii(passwordTextField); + addListenerToUpdateInvalidSelectorStateIfTextIsNotAscii(titleTextField); + addListenerToUpdateInvalidSelectorStateIfTextIsNotAscii(passwordTextField);Also applies to: 136-138
src/test/java/com/faforever/client/coop/CoopControllerTest.java (3)
137-143: Clarify positive-case test nameTitle must be non-empty and ASCII; adjust DisplayName to reflect both.
- @DisplayName("Play button is enabled if title is not empty") + @DisplayName("Play button is enabled if title is ASCII and not empty")
151-164: Also assert layout management of warning labelsTo avoid layout gaps when hidden, bind managed to visible and assert both are false by default and toggle together.
Example assertions to add:
assertFalse(instance.titleWarningLabel.isVisible()); + assertFalse(instance.titleWarningLabel.isManaged()); assertFalse(instance.passwordWarningLabel.isVisible()); + assertFalse(instance.passwordWarningLabel.isManaged());If not already done in controller/FXML, bind once during initialization:
titleWarningLabel.managedProperty().bind(titleWarningLabel.visibleProperty()); passwordWarningLabel.managedProperty().bind(passwordWarningLabel.visibleProperty());
119-189: Reduce duplication with parameterized testsMultiple tests repeat the same pattern (set text → assert disabled/invalid). Consider JUnit 5 @ParameterizedTest with MethodSource to cover ASCII/non-ASCII for both fields succinctly.
src/main/resources/theme/play/coop/coop.fxml (1)
53-71: Avoid hardcoded color and font where possiblePrefer themed CSS classes (e.g., “warning-label” or equivalent) over inline textFill="RED" and Font size for consistency and accessibility. If such a class exists, switch to it; otherwise consider adding one in the CSS.
Also applies to: 65-76
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/main/java/com/faforever/client/coop/CoopController.java(6 hunks)src/main/java/com/faforever/client/game/CreateGameController.java(3 hunks)src/main/java/com/faforever/client/map/MapUploadTask.java(2 hunks)src/main/java/com/faforever/client/mod/ModUploadTask.java(2 hunks)src/main/java/com/faforever/client/util/Validator.java(2 hunks)src/main/resources/theme/play/coop/coop.fxml(4 hunks)src/main/resources/theme/play/create_game.fxml(1 hunks)src/main/resources/theme/style.css(1 hunks)src/test/java/com/faforever/client/coop/CoopControllerTest.java(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/main/java/com/faforever/client/mod/ModUploadTask.java (1)
src/test/java/com/faforever/client/mod/ModUploadTaskTest.java (3)
Test(46-49)Test(51-55)RunWith(25-67)
src/main/resources/theme/style.css (1)
src/test/java/com/faforever/client/game/CreateGameControllerTest.java (1)
Test(448-455)
src/main/java/com/faforever/client/game/CreateGameController.java (2)
src/main/java/com/faforever/client/util/Validator.java (1)
Validator(6-21)src/test/java/com/faforever/client/game/CreateGameControllerTest.java (4)
Test(203-211)Test(214-223)Test(448-455)Test(225-234)
src/main/java/com/faforever/client/coop/CoopController.java (1)
src/main/java/com/faforever/client/util/Validator.java (1)
Validator(6-21)
⏰ 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). (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: checks
- GitHub Check: checks
🔇 Additional comments (9)
src/main/java/com/faforever/client/map/MapUploadTask.java (1)
22-22: LGTM on null-safety switchReplacing Validator.notNull with Objects.requireNonNull is consistent and preserves behavior (NPE with message). No further changes needed.
Also applies to: 55-56
src/main/java/com/faforever/client/mod/ModUploadTask.java (1)
28-28: LGTM on null-safety switchObjects.requireNonNull(modPath, ...) is appropriate and consistent with the codebase changes.
Also applies to: 62-62
src/main/java/com/faforever/client/coop/CoopController.java (1)
134-146: Validation wiring looks solidInvalid-state bindings and playButton disablement correctly combine emptiness and ASCII checks.
src/main/resources/theme/style.css (1)
641-643: Selector rename verified and correctly implementedThe CSS selector rename is complete and correct:
- Old
.game-title-input:invalidselector successfully removed from CSS- New
.invalid-text-field:invalidselector present (lines 641-643)- Relevant TextFields properly updated: titleTextField in create_game.fxml (line 42) retains both "game-title-input" (for font-size) and "invalid-text-field" classes; password and title fields in coop.fxml (lines 55, 69) have "invalid-text-field" applied
src/test/java/com/faforever/client/coop/CoopControllerTest.java (3)
119-126: Good default-state coverageValidates initial disabled state and empty fields; this guards regressions in default UI state.
166-176: LGTM: pseudo-class state toggles for titleLive check against PseudoClass set is correct and proves the invalid state toggles.
178-188: LGTM: pseudo-class state toggles for passwordMirrors the title case; assertions look good.
src/main/resources/theme/play/coop/coop.fxml (2)
53-70: CSS and controller wiring for validity state are correctly implementedVerification confirms the implementation is valid:
- CSS targets
.invalid-text-field:invalidatsrc/main/resources/theme/style.css:641, combining the style class with the pseudo-classCoopControllerdefinesINVALID_PSEUDO_CLASSand toggles it viapseudoClassStateChanged()at line 241- TextFields in
coop.fxmlhave the requiredstyleClass="invalid-text-field"The wiring works as intended.
26-27: No mismatch found — JavaFX 25 is explicitly configuredThe app runtime is correctly set to JavaFX 25 in
build.gradle(line 236:version = "25"), and Java compatibility is also set toJavaVersion.VERSION_25. The FXML namespacejavafx/25aligns with the configured runtime version. No action required.Likely an incorrect or invalid review comment.
| <GridPane fx:id="coopRoot" gridLinesVisible="true" hgap="10.0" xmlns="http://javafx.com/javafx/25" | ||
| xmlns:fx="http://javafx.com/fxml/1" fx:controller="com.faforever.client.coop.CoopController"> |
There was a problem hiding this comment.
Remove debug grid lines
gridLinesVisible="true" should not ship; it leaks debug visuals.
-<GridPane fx:id="coopRoot" gridLinesVisible="true" hgap="10.0" xmlns="http://javafx.com/javafx/25"
+<GridPane fx:id="coopRoot" hgap="10.0" xmlns="http://javafx.com/javafx/25"📝 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.
| <GridPane fx:id="coopRoot" gridLinesVisible="true" hgap="10.0" xmlns="http://javafx.com/javafx/25" | |
| xmlns:fx="http://javafx.com/fxml/1" fx:controller="com.faforever.client.coop.CoopController"> | |
| <GridPane fx:id="coopRoot" hgap="10.0" xmlns="http://javafx.com/javafx/25" | |
| xmlns:fx="http://javafx.com/fxml/1" fx:controller="com.faforever.client.coop.CoopController"> |
🤖 Prompt for AI Agents
In src/main/resources/theme/play/coop/coop.fxml around lines 26 to 27, the
GridPane has gridLinesVisible="true" which exposes debug visuals; remove the
gridLinesVisible attribute or set it to "false" so the debug grid won't appear
in production UI, and ensure no other debug-only attributes remain in the FXML.
| <VBox spacing="2.0"> | ||
| <children> | ||
| <TextField fx:id="titleTextField" maxWidth="1.7976931348623157E308" | ||
| minWidth="100.0" prefWidth="400.0" promptText="%game.create.title" | ||
| styleClass="invalid-text-field"/> | ||
| <Label fx:id="titleWarningLabel" text="%game.create.titleNotAscii" | ||
| textFill="RED" wrapText="true"> | ||
| <font> | ||
| <Font size="14.0"/> | ||
| </font> | ||
| </Label> | ||
| </children> | ||
| </VBox> | ||
| <VBox spacing="2.0"> | ||
| <children> | ||
| <PasswordField fx:id="passwordTextField" maxWidth="1.7976931348623157E308" | ||
| minWidth="100.0" prefWidth="350.0" | ||
| promptText="%game.create.password" | ||
| styleClass="invalid-text-field"/> | ||
| <Label fx:id="passwordWarningLabel" text="%game.create.passwordNotAscii" | ||
| textFill="RED" wrapText="true"> | ||
| <font> | ||
| <Font size="14.0"/> | ||
| </font> | ||
| </Label> | ||
| </children> | ||
| </VBox> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Initialize warning labels as hidden and non-managed
Prevent layout gaps and flicker by defaulting labels to invisible and unmanaged in FXML. Controller can toggle visible only.
-<Label fx:id="titleWarningLabel" text="%game.create.titleNotAscii"
- textFill="RED" wrapText="true">
+<Label fx:id="titleWarningLabel" text="%game.create.titleNotAscii"
+ textFill="RED" wrapText="true" visible="false" managed="false">
<font>
<Font size="14.0"/>
</font>
</Label>
...
-<Label fx:id="passwordWarningLabel" text="%game.create.passwordNotAscii"
- textFill="RED" wrapText="true">
+<Label fx:id="passwordWarningLabel" text="%game.create.passwordNotAscii"
+ textFill="RED" wrapText="true" visible="false" managed="false">
<font>
<Font size="14.0"/>
</font>
</Label>🤖 Prompt for AI Agents
In src/main/resources/theme/play/coop/coop.fxml around lines 51 to 77, the
warning Labels for title and password are always visible and managed which can
cause layout gaps and flicker; update each Label to set visible="false" and
managed="false" in the FXML so they are hidden and do not occupy layout space by
default, allowing the controller to set visible/managed to true when showing
warnings.
| <Label fx:id="titleWarningLabel" text="%game.create.titleNotAscii" | ||
| textFill="RED" wrapText="true"> |
There was a problem hiding this comment.
🧩 Analysis chain
Ensure i18n keys exist
Confirm that game.create.titleNotAscii and game.create.passwordNotAscii are defined in bundle(s).
Also applies to: 70-71
🏁 Script executed:
#!/bin/bash
rg -n 'game\.create\.(titleNotAscii|passwordNotAscii)=' src/main/resources -g '**/*.properties' -C1Length of output: 632
I can see the Korean bundle has these keys defined. Let me verify they exist in the primary/default bundle as well.
🏁 Script executed:
#!/bin/bash
# Check if keys exist in the default messages.properties file
rg -n 'game\.create\.(titleNotAscii|passwordNotAscii)=' src/main/resources/i18n/messages.properties -C1Length of output: 0
🏁 Script executed:
#!/bin/bash
# Get a complete overview of all bundles containing these keys
rg -l 'game\.create\.(titleNotAscii|passwordNotAscii)=' src/main/resources -g '**/*.properties'Length of output: 46
Add i18n keys to default bundle
The keys game.create.titleNotAscii and game.create.passwordNotAscii are defined only in messages_ko.properties but are missing from the default messages.properties bundle. This will cause the FXML labels to fail resolution for non-Korean locales. Add these keys to src/main/resources/i18n/messages.properties.
Also applies to: 70-71
🤖 Prompt for AI Agents
In src/main/resources/theme/play/coop/coop.fxml around lines 56-57 and 70-71 the
FXML references i18n keys game.create.titleNotAscii and
game.create.passwordNotAscii which exist only in messages_ko.properties; add
these two keys with appropriate default (English) messages to
src/main/resources/i18n/messages.properties so the labels resolve for non-Korean
locales, ensuring the key names exactly match and saving the file encoding as
UTF-8.
| <TextField fx:id="titleTextField" promptText="%game.create.title" styleClass="game-title-input, invalid-text-field" | ||
| GridPane.columnSpan="2147483647"/> |
There was a problem hiding this comment.
Fix styleClass separator (comma breaks CSS class matching)
Use space-separated classes; the comma makes the first class “game-title-input,” which won’t match CSS.
-<TextField fx:id="titleTextField" promptText="%game.create.title" styleClass="game-title-input, invalid-text-field"
+<TextField fx:id="titleTextField" promptText="%game.create.title" styleClass="game-title-input invalid-text-field"📝 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.
| <TextField fx:id="titleTextField" promptText="%game.create.title" styleClass="game-title-input, invalid-text-field" | |
| GridPane.columnSpan="2147483647"/> | |
| <TextField fx:id="titleTextField" promptText="%game.create.title" styleClass="game-title-input invalid-text-field" | |
| GridPane.columnSpan="2147483647"/> |
🤖 Prompt for AI Agents
In src/main/resources/theme/play/create_game.fxml around lines 42 to 43, the
styleClass attribute uses a comma-separated list which makes the first class
include a trailing comma and breaks CSS matching; change the attribute to use
space-separated class names (e.g., "game-title-input invalid-text-field") and
remove any trailing commas so both classes are applied correctly.
| @Test | ||
| @DisplayName("Play button is disabled if title is not acsii or empty") | ||
| public void test0() { | ||
| runOnFxThreadAndWait(() -> instance.titleTextField.setText("Это не Acsii заголовок")); | ||
| assertTrue(instance.playButton.isDisabled()); | ||
|
|
||
| runOnFxThreadAndWait(() -> instance.titleTextField.setText("")); | ||
| assertTrue(instance.playButton.isDisabled()); | ||
| } |
There was a problem hiding this comment.
Fix “ASCII” typos in test names and data
Use the correct “ASCII” spelling in DisplayName and sample strings to keep tests readable and searchable.
Apply this diff:
- @DisplayName("Play button is disabled if title is not acsii or empty")
+ @DisplayName("Play button is disabled if title is not ASCII or empty")
- runOnFxThreadAndWait(() -> instance.titleTextField.setText("Это не Acsii заголовок"));
+ runOnFxThreadAndWait(() -> instance.titleTextField.setText("Это не ASCII заголовок"));
...
- @DisplayName("Play button is disabled if a password is not acsii")
+ @DisplayName("Play button is disabled if a password is not ASCII")
...
- @DisplayName("State of `invalid` selector in a field is updated if title is not ascii")
+ @DisplayName("State of `invalid` selector in a field is updated if title is not ASCII")
...
- @DisplayName("State of `invalid` selector in a field is updated if password is not ascii")
+ @DisplayName("State of `invalid` selector in a field is updated if password is not ASCII")
- runOnFxThreadAndWait(() -> instance.titleTextField.setText("Это не Acsii заголовок"));
+ runOnFxThreadAndWait(() -> instance.titleTextField.setText("Это не ASCII заголовок"));Also applies to: 145-149, 167-171, 179-183
🤖 Prompt for AI Agents
In src/test/java/com/faforever/client/coop/CoopControllerTest.java around lines
127 to 135 (and likewise at 145-149, 167-171, 179-183), the word “Acsii” is
misspelled; update the @DisplayName annotations to use the correct “ASCII”
spelling and fix any sample strings containing the same typo (e.g., replace
"Acsii" with "ASCII" and correct sample non-ASCII test data text if it
mistakenly contains "Acsii"); run the tests to ensure names and strings compile
and are searchable with the corrected spelling.
Sheikah45
left a comment
Sheikah45
left a comment
There was a problem hiding this comment.
Is this good from your point of view?
Closes #3308
Summary by CodeRabbit
New Features
UI/UX Improvements