Coop Tab UI Updates #3445
Coop Tab UI Updates #3445
Conversation
# Conflicts: # src/main/resources/theme/icons.css
WalkthroughRefactors coop to be scenario-driven: adds CoopScenario/CoopFaction/CoopType types, scenario loading and caching, UI wiring for scenario→missions, mapper support, cache entry, FXML layout updates, theme/icon and i18n additions, and removes the old CoopCategory enum from coop package. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CoopController
participant CoopService
participant CoopMapper
participant Cache
User->>CoopController: Open Coop view
CoopController->>CoopService: getScenarios()
CoopService->>Cache: lookup COOP_SCENARIOS
alt cache hit
Cache-->>CoopService: cached Flux<CoopScenario>
else cache miss
CoopService->>CoopService: request API (includes maps)
CoopService->>CoopMapper: map DTO -> CoopScenario
CoopService->>Cache: store COOP_SCENARIOS
end
CoopService-->>CoopController: Flux<CoopScenario>
CoopController->>CoopController: populate scenarioComboBox
User->>CoopController: select scenario
CoopController->>CoopController: populateMissionList(selectedScenario)
CoopController-->>User: update missions, map preview, leaderboard
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 4
🧹 Nitpick comments (7)
src/main/java/com/faforever/client/theme/ThemeService.java (1)
102-104: Consider alphabetical ordering of constants.The new constants
MOD_ICON_STYLE_CLASSandWRENCH_STYLE_CLASSare placed between faction icon constants, which disrupts the grouping pattern. Consider either:
- Grouping all faction icons together (AEON, CYBRAN, SERAPHIM, UEF) followed by other icons (MOD, WRENCH), or
- Ordering all icon constants alphabetically
This improves maintainability and makes it easier to locate specific constants.
src/main/java/com/faforever/client/coop/CoopType.java (1)
3-5: Consider adding documentation for enum values.The enum values
SC,SCFA, andCUSTOMwould benefit from Javadoc comments explaining what they represent. This improves code clarity, especially for new contributors who may not be familiar with the abbreviations.Example:
public enum CoopType { + /** Supreme Commander base game scenarios */ SC, + /** Supreme Commander: Forged Alliance expansion scenarios */ SCFA, + /** Custom user-created scenarios */ CUSTOM }src/main/java/com/faforever/client/domain/api/CoopCategory.java (1)
5-8: Consider clarifying the purpose and naming of this record.The
CoopCategoryrecord has a field namedcoopCategoryof typeCoopFaction, which creates naming ambiguity. Additionally, having bothcategoryName(String) andcoopCategory(CoopFaction) suggests potential redundancy.Consider:
- If
categoryNameis a display name whilecoopCategoryis the enum value, rename the field todisplayNamefor clarity- If they serve different purposes, add Javadoc to explain the distinction
- Verify if both fields are necessary or if one can be derived from the other
Example:
/** * Represents a cooperative scenario category. * @param displayName The human-readable category name * @param faction The faction associated with this category */ public record CoopCategory( String displayName, CoopFaction faction ) {}src/main/resources/theme/play/coop/coop.fxml (1)
3-3: Remove unnecessary import.The
<?import java.lang.*?>import is unnecessary asjava.langis automatically imported in Java. This import serves no purpose in FXML.Apply this diff:
-<?import java.lang.*?> <?import javafx.collections.*?>src/main/java/com/faforever/client/coop/CoopController.java (3)
33-33: Remove unused import.The
javafx.application.HostServicesimport is not used in this file.Apply this diff:
-import javafx.application.HostServices; import javafx.collections.FXCollections;
55-55: Remove unused import.The
jdk.jfr.Categoryimport is not used in this file.Apply this diff:
import javafx.util.StringConverter; -import jdk.jfr.Category; import lombok.RequiredArgsConstructor;
212-215: Consider using streams for better readability.The explicit loop can be replaced with a more idiomatic stream operation using
flatMap.Apply this diff:
- List<CoopMission> coopMissions = new ArrayList<>(); - for (CoopScenario coopScenario : coopScenarios) { - coopMissions.addAll(coopScenario.maps()); - } + List<CoopMission> coopMissions = coopScenarios.stream() + .flatMap(scenario -> scenario.maps().stream()) + .toList();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.idea/codeStyles/Project.xml(1 hunks).idea/runConfigurations/Main.xml(1 hunks)src/main/java/com/faforever/client/config/CacheConfig.java(2 hunks)src/main/java/com/faforever/client/config/CacheNames.java(1 hunks)src/main/java/com/faforever/client/coop/CoopCategory.java(0 hunks)src/main/java/com/faforever/client/coop/CoopController.java(10 hunks)src/main/java/com/faforever/client/coop/CoopFaction.java(1 hunks)src/main/java/com/faforever/client/coop/CoopService.java(2 hunks)src/main/java/com/faforever/client/coop/CoopType.java(1 hunks)src/main/java/com/faforever/client/domain/api/CoopCategory.java(1 hunks)src/main/java/com/faforever/client/domain/api/CoopMission.java(1 hunks)src/main/java/com/faforever/client/domain/api/CoopScenario.java(1 hunks)src/main/java/com/faforever/client/mapstruct/CoopMapper.java(2 hunks)src/main/java/com/faforever/client/theme/ThemeService.java(1 hunks)src/main/resources/i18n/messages.properties(1 hunks)src/main/resources/theme/icons.css(1 hunks)src/main/resources/theme/play/coop/coop.fxml(1 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/com/faforever/client/coop/CoopCategory.java
🧰 Additional context used
🧬 Code graph analysis (2)
src/main/java/com/faforever/client/config/CacheConfig.java (1)
src/main/java/com/faforever/client/config/CacheNames.java (1)
CacheNames(3-50)
src/main/java/com/faforever/client/coop/CoopService.java (1)
src/main/java/com/faforever/client/config/CacheNames.java (1)
CacheNames(3-50)
🪛 Biome (2.1.2)
src/main/resources/theme/icons.css
[error] 403-403: Unknown property is not allowed.
See CSS Specifications and browser specific properties for more details.
To resolve this issue, replace the unknown property with a valid CSS property.
(lint/correctness/noUnknownProperty)
🔇 Additional comments (12)
.idea/codeStyles/Project.xml (1)
28-30: Verify intent of EditorConfig disabling.This change disables EditorConfig in the IDE project configuration, which appears unrelated to the stated PR objectives (Coop Tab UI updates, scenario selection, wiki/Discord links). Confirm whether this was intentional or a stray modification that should be excluded from this PR.
.idea/runConfigurations/Main.xml (1)
1-18: Original review comment is based on incorrect assumptions about repository practices and should be disregarded.The change is not problematic:
Repository intentionally tracks
.idea/files: Despite the root.gitignorehaving.idea/*, the repository deliberately commits.idea/configuration files. Files like.idea/runConfigurations/Main.xml,.idea/codeStyles/Project.xml, and.idea/vcs.xmlare tracked in version control, indicating this is standard practice for the project.Java 25 is already the build target: The
build.gradlefile already specifiessourceCompatibility = JavaVersion.VERSION_25andtargetCompatibility = JavaVersion.VERSION_25. This IDE configuration change merely aligns with the existing build configuration rather than introducing an unverified upgrade.No compatibility issues detected: The codebase has only 7 deprecation warnings, which is normal and not blocking for Java 25 adoption.
The IDE run configuration update is a straightforward synchronization with the project's existing Java 25 build configuration, not a risky or undiscussed change.
Likely an incorrect or invalid review comment.
src/main/resources/theme/icons.css (1)
402-404: LGTM! Static analysis warning is a false positive.The
.wrench-iconclass is correctly implemented following the same pattern as other icon classes in this file. The static analysis tool (Biome) incorrectly flags-fx-shapeas an unknown property, but this is a valid JavaFX CSS property used throughout this file.src/main/java/com/faforever/client/config/CacheConfig.java (1)
95-95: LGTM!The
COOP_SCENARIOScache is properly configured with the same expiration and size semantics as the relatedCOOP_MAPScache. The placement next toCOOP_MAPSis logical and maintains good organization.src/main/resources/i18n/messages.properties (1)
46-46: LGTM!The new translation key
coop.wiki.buttonis properly placed within the coop section and follows the existing naming conventions. The value "Visit Wiki" is clear and appropriate for a button label.src/main/java/com/faforever/client/config/CacheNames.java (1)
34-34: LGTM!The
COOP_SCENARIOSconstant is correctly defined and placed logically next to the relatedCOOP_MAPSconstant. The naming convention is consistent with other cache names in the file.src/main/java/com/faforever/client/coop/CoopService.java (1)
43-48: LGTM!The
getScenarios()method is well-implemented, following the same pattern as the existinggetMissions()method. The caching annotation, ElideNavigator configuration withaddInclude("maps")to fetch related missions, and the use of reactive streams are all appropriate.The page size of 1000 matches
getMissions()and should be sufficient for coop scenarios, but consider monitoring if the number of scenarios approaches this limit in the future.src/main/java/com/faforever/client/coop/CoopFaction.java (1)
3-5: LGTM!The enum is well-defined and covers the required faction types for the coop scenario feature.
src/main/java/com/faforever/client/mapstruct/CoopMapper.java (1)
21-25: LGTM!The CoopScenario mapping methods follow the established MapStruct patterns in this mapper interface and provide proper bidirectional conversion between DTOs and domain objects.
src/main/java/com/faforever/client/domain/api/CoopScenario.java (1)
16-18: LGTM!The compact constructor provides good defensive programming by ensuring the
mapsfield is never null and always immutable, which prevents accidental mutations and null pointer exceptions.src/main/java/com/faforever/client/coop/CoopController.java (2)
127-129: LGTM!The scenario selection binding properly drives the mission list population, establishing a clean data flow from scenario selection to mission availability.
310-318: LGTM!The
populateMissionListmethod correctly handles null scenarios and properly updates the mission combo box with the selected scenario's maps.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/main/java/com/faforever/client/coop/CoopController.java (2)
33-33: Remove unused imports.The following imports are not used in this file:
- Line 33:
HostServices- Line 54:
StringConverter- Line 55:
jdk.jfr.Category(incorrect import, unrelated to the domain model)- Line 65:
URISyntaxException- Line 69:
ArraysApply this diff to remove the unused imports:
-import javafx.application.HostServices;-import javafx.util.StringConverter; -import jdk.jfr.Category;-import java.net.URISyntaxException;-import java.util.Arrays;Also applies to: 54-55, 65-65, 69-69
258-276: Improve naming consistency for scenario-based architecture.The method correctly implements faction-based icons for scenarios, including the new wrench icon for custom missions. However, there are naming inconsistencies:
- Line 259: The lambda parameter is named
categorybut should bescenarioto match the new scenario-based model.- Line 275: The style class
"coop-category"should be"coop-scenario"to align with the architectural change.Apply this diff to improve naming consistency:
- private ListCell<CoopScenario> scenarioListCell() { - return new StringListCell<>(fxApplicationThreadExecutor, CoopScenario::name, category -> { + private ListCell<CoopScenario> scenarioListCell() { + return new StringListCell<>(fxApplicationThreadExecutor, CoopScenario::name, scenario -> { Label label = new Label(); Region iconRegion = new Region(); label.setGraphic(iconRegion); iconRegion.getStyleClass().add(ThemeService.CSS_CLASS_ICON); - switch (category.faction()) { + switch (scenario.faction()) { case AEON -> iconRegion.getStyleClass().add(ThemeService.AEON_STYLE_CLASS); case CYBRAN -> iconRegion.getStyleClass().add(ThemeService.CYBRAN_STYLE_CLASS); case UEF -> iconRegion.getStyleClass().add(ThemeService.UEF_STYLE_CLASS); case SERAPHIM -> iconRegion.getStyleClass().add(ThemeService.SERAPHIM_STYLE_CLASS); case CUSTOM -> iconRegion.getStyleClass().add(ThemeService.WRENCH_STYLE_CLASS); default -> { return null; } } return label; - }, Pos.CENTER_LEFT, "coop-category"); + }, Pos.CENTER_LEFT, "coop-scenario"); }src/main/resources/theme/play/coop/coop.fxml (2)
3-3: Remove unnecessary import.The
java.lang.*package is automatically imported and does not need to be explicitly declared.Apply this diff:
-<?import java.lang.*?>
64-65: Consider adding prompt text for better UX.The scenario and mission ComboBoxes lack
promptTextattributes, which could help users understand their purpose when empty.Consider adding prompt text to guide users:
- <ComboBox fx:id="scenarioComboBox" prefHeight="25.0" prefWidth="200.0" /> - <ComboBox fx:id="missionComboBox" prefHeight="25.0" prefWidth="200.0" /> + <ComboBox fx:id="scenarioComboBox" prefHeight="25.0" prefWidth="200.0" promptText="%coop.scenario.prompt" /> + <ComboBox fx:id="missionComboBox" prefHeight="25.0" prefWidth="200.0" promptText="%coop.mission.prompt" />Then add the corresponding keys to your i18n properties files (e.g., "Select Scenario" and "Select Mission").
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/com/faforever/client/coop/CoopController.java(10 hunks)src/main/java/com/faforever/client/domain/api/CoopMission.java(1 hunks)src/main/resources/i18n/messages.properties(2 hunks)src/main/resources/theme/play/coop/coop.fxml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/resources/i18n/messages.properties
- src/main/java/com/faforever/client/domain/api/CoopMission.java
⏰ 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/java/com/faforever/client/coop/CoopController.java (7)
127-129: LGTM!The scenarioComboBox initialization is correctly configured with appropriate cell factories and selection listener.
131-133: LGTM!The missionComboBox cell factory configuration is correct and consistent with the new architecture.
205-231: LGTM!The transition from mission-based to scenario-based data loading is well-implemented. The aggregation of missions from all scenarios provides the necessary data for the games table while maintaining the new scenario-centric UI model.
278-280: LGTM!The missionListCell factory is correctly implemented using the
concatNamemethod reference.
310-318: LGTM!The mission list population logic correctly clears and repopulates the mission combo box based on the selected scenario, with appropriate null handling and default selection.
333-346: LGTM! Past review issues addressed.The browser navigation implementation now correctly addresses all the issues raised in the previous review:
- Proper Desktop support checking using
isDesktopSupported()andisSupported(Desktop.Action.BROWSE)- User notifications for unsupported platforms and errors
- Direct URI usage without unnecessary reconstruction
- No silent failures
353-359: LGTM!The new action methods for wiki and Discord navigation are correctly implemented and properly delegate to the browser navigation helper.
src/main/resources/theme/play/coop/coop.fxml (4)
11-11: Past issue resolved.The
gridLinesVisible="true"debug attribute has been successfully removed as requested in the previous review.
57-57: Past issue resolved.The hard-coded English text has been successfully replaced with the i18n key
%coop.info.labelas requested in the previous review.
67-69: LGTM!The clickable overlay pattern for the map preview ImageView is implemented correctly, allowing mouse interaction while preserving the image's visual properties.
115-127: LGTM!The leaderboard TableView is properly configured with explicit columns, i18n keys for all text labels, and appropriate resize policy.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/resources/theme/play/coop/coop.fxml (2)
3-9: Consider using specific imports instead of wildcards.Wildcard imports (e.g.,
<?import javafx.scene.layout.*?>) import all classes from a package, which can reduce clarity and potentially cause naming conflicts. Specific imports make it clearer which components are actually used in the FXML.
36-38: Consider using "Infinity" for unbounded maximum width.The value
1.7976931348623157E308representsDouble.MAX_VALUEand is used to specify unbounded maximum width. For better readability and maintainability, consider using the more idiomaticInfinityvalue or the constantMAX_VALUE.Apply this diff for improved readability:
- <TextField fx:id="titleTextField" maxWidth="1.7976931348623157E308" minWidth="100.0" prefWidth="400.0" promptText="%game.create.title" /> - <PasswordField fx:id="passwordTextField" maxWidth="1.7976931348623157E308" minWidth="100.0" prefWidth="350.0" promptText="%game.create.password" /> + <TextField fx:id="titleTextField" maxWidth="Infinity" minWidth="100.0" prefWidth="400.0" promptText="%game.create.title" /> + <PasswordField fx:id="passwordTextField" maxWidth="Infinity" minWidth="100.0" prefWidth="350.0" promptText="%game.create.password" />Note: Similar occurrences exist elsewhere in the file (e.g., lines 78, 88, 90, 116).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/resources/i18n/messages.properties(2 hunks)src/main/resources/theme/play/coop/coop.fxml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/resources/i18n/messages.properties
⏰ 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 (4)
src/main/resources/theme/play/coop/coop.fxml (4)
11-18: LGTM: Root layout structure.The GridPane root element and responsive column constraints (55%/45% split) provide a good foundation for the two-panel layout. The debug attribute
gridLinesVisiblehas been correctly removed from previous versions.
47-52: LGTM: Discord button accessibility addressed.The Discord button now includes a tooltip with the i18n key
%coop.discord.button, which addresses the accessibility concern raised in previous reviews. This ensures screen reader users can understand the button's purpose.
64-71: LGTM: Scenario selection structure.The scenario and mission combo boxes, along with the map preview StackPane, are well-structured and align with the PR objectives to add scenario-based selection. The use of a Pane overlay on the ImageView for click handling is a standard JavaFX pattern.
116-129: LGTM: TableView configuration.The explicit TableColumn definitions with individual width constraints and i18n keys are well-configured. The use of
CONSTRAINED_RESIZE_POLICYensures proper column sizing behavior for the leaderboard table.
| if (!Desktop.isDesktopSupported() || !Desktop.getDesktop().isSupported(Desktop.Action.BROWSE)) { | ||
| log.warn("Desktop browsing is not supported on this platform."); | ||
| notificationService.addImmediateWarnNotification("coop.browser.notSupported"); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| Desktop.getDesktop().browse(uri); | ||
| } catch (IOException e) { | ||
| log.error("Could not open browser.", e); | ||
| notificationService.addImmediateErrorNotification(e, "coop.browser.error"); | ||
| } |
There was a problem hiding this comment.
Use the platformService.showDocument for opening a browser
| @Cacheable(value = CacheNames.COOP_SCENARIOS, sync = true) | ||
| public Flux<CoopScenario> getScenarios() { | ||
| ElideNavigatorOnCollection<com.faforever.commons.api.dto.CoopScenario> navigator = ElideNavigator.of( | ||
| com.faforever.commons.api.dto.CoopScenario.class).collection().addInclude("maps").pageSize(1000); |
There was a problem hiding this comment.
The max page size in the api is now 100 so 1000 won't do anything.
Also instead of specifying the include here it should be specified in the type map in the fafApiAccessor. GetMany also handles the page size
|
|
||
| public record CoopCategory( | ||
| String categoryName, | ||
| CoopFaction coopCategory |
There was a problem hiding this comment.
The variable naming here is a bit confusing. What exactly is the category?
| public String concatName() { | ||
| return name + " - V" + version; | ||
| } |
There was a problem hiding this comment.
Can this method be moved to the controller?
| public CoopScenario { | ||
| maps = maps == null ? List.of() : List.copyOf(maps); | ||
| } |
There was a problem hiding this comment.
We should make sure to not pass in null for the maps
| @InheritInverseConfiguration | ||
| com.faforever.commons.api.dto.CoopMission map(CoopMission bean); | ||
|
|
||
| @Mapping(target = ".", source = "dto") |
There was a problem hiding this comment.
I don't think this annotation is necessary since there is only one parameter
2 participants
The coop tab has been updated with the following changes:
Summary by CodeRabbit
New Features
UI Improvements
Style
✏️ Tip: You can customize this high-level summary in your review settings.