Add more useful information in status bar before running a game #3417
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sheikah45
requested changes
Oct 22, 2025
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughIntroduces task-based async execution: new SimpleTask and CompletableTask constructor, TaskService gains submitFutureTask overloads with i18n title resolution, GameRunner now submits league/replay/ICE operations through TaskService, status i18n keys added, and tests updated to capture and run submitted future suppliers. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GameRunner
participant TaskService
participant SimpleTask
participant Operations as League/Replay/ICE
participant StatusBar
User->>GameRunner: Start online game
GameRunner->>TaskService: submitFutureTask("league.loadInfo", supplier)
TaskService->>SimpleTask: create & run (title resolved via i18n)
TaskService->>StatusBar: register active task (show title)
SimpleTask->>Operations: execute league load (async)
par parallel connections
GameRunner->>TaskService: submitFutureTask("replayServer.connecting", supplier)
TaskService->>SimpleTask: create & run (replay)
SimpleTask->>Operations: connect replay server
GameRunner->>TaskService: submitFutureTask("iceAdapter.connecting", supplier)
TaskService->>SimpleTask: create & run (ice)
SimpleTask->>Operations: connect ICE adapter
end
Operations-->>SimpleTask: results
SimpleTask-->>TaskService: futures complete
TaskService->>StatusBar: remove completed tasks (on FX thread)
GameRunner->>GameRunner: handle results, wait for process exit, finalize
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Potential attention points:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Collaborator
Author
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/main/java/com/faforever/client/ui/statusbar/StatusBarController.java (1)
95-102: Compilation error: "_" is not a valid identifier on Java 9+.Using "_" as a lambda parameter name is illegal and breaks builds on Java 17. Rename the parameter to a valid identifier (e.g., ignored) or use a method reference.
- JavaFxUtil.addListener(taskService.getActiveWorkers(), (Observable _) -> { + JavaFxUtil.addListener(taskService.getActiveWorkers(), (Observable ignored) -> {src/main/java/com/faforever/client/game/GameRunner.java (3)
151-166: Compilation error: "_" in lambda param.Java forbids "_" as an identifier. Rename the parameter.
- .doOnNext(_ -> { + .doOnNext(notice -> {
193-214: Compilation errors: "_" identifiers in chained stages.Several lambdas use "_" placeholders; invalid on Java 9+. Rename them.
- .thenApply(_ -> gameMapper.map(gameLaunchResponse, loadLeagueInfoFuture.join())) + .thenApply(ignored -> gameMapper.map(gameLaunchResponse, loadLeagueInfoFuture.join())) - .whenCompleteAsync((process, _) -> { + .whenCompleteAsync((process, throwable) -> { if (process != null) { this.process.set(process); runningGameId.set(uid); } }, fxApplicationThreadExecutor) - .thenAccept(this::handleTermination) - .whenComplete((_, _) -> { + .thenAccept(this::handleTermination) + .whenComplete((ignored, throwable) -> { iceAdapter.stop(); replayServer.stop(); fafServerAccessor.notifyGameEnded(); }) - .whenCompleteAsync((_, _) -> { + .whenCompleteAsync((ignored, throwable) -> { process.set(null); runningGameId.set(null); }, fxApplicationThreadExecutor);
556-574: Compilation error: "_" identifier in whenCompleteAsync and exceptionally chains.Rename unused parameters.
- CompletableFuture.supplyAsync(() -> forgedAllianceLaunchService.launchOfflineGame(null)) - .whenCompleteAsync((process, _) -> { + CompletableFuture.supplyAsync(() -> forgedAllianceLaunchService.launchOfflineGame(null)) + .whenCompleteAsync((process, throwable) -> { if (process != null) { this.process.set(process); } }, fxApplicationThreadExecutor) .thenCompose(Process::onExit) .thenAccept(this::handleTermination) - .exceptionally(throwable -> { + .exceptionally(throwable -> { throwable = ConcurrentUtil.unwrapIfCompletionException(throwable); log.error("Launching offline game failed", throwable); if (throwable instanceof NotifiableException notifiableException) { notificationService.addErrorNotification(notifiableException); } else { notificationService.addImmediateErrorNotification(throwable, "tutorial.launchFailed"); } return null; });
🧹 Nitpick comments (5)
src/main/resources/i18n/messages.properties (1)
1317-1319: Polish i18n strings (grammar + consistency with existing ellipses).
- Use ellipsis like other “Connecting…”/“Loading…” keys.
- Fix article: “Loading league information”.
-iceAdapter.connecting = Connecting to ICE adapter -replayServer.connecting = Connecting to replay server -league.loadInfo = Loading a league information +iceAdapter.connecting = Connecting to ICE adapter… +replayServer.connecting = Connecting to replay server… +league.loadInfo = Loading league information…src/main/java/com/faforever/client/task/CompletableTask.java (1)
15-18: Default ctor locks priority to MEDIUM; setPriority() becomes unusable.The new no-arg ctor sets priority immediately, but setPriority() throws if priority is already set. Any subclass (e.g., SimpleTask) now cannot change priority, eliminating prioritization flexibility.
Options:
- Add constructors that accept Priority in subclasses (e.g., SimpleTask) and call super(priority).
- Or allow one-time set in setPriority() (only if not yet executed), initializing priority to null in the no-arg ctor.
Do you want a follow-up patch adding a Priority-accepting SimpleTask ctor and tests?
src/main/java/com/faforever/client/task/SimpleTask.java (2)
17-21: Allow priority configuration for SimpleTask.SimpleTask currently always uses MEDIUM (inherited default). Provide an overload to pass Priority and call super(priority) to keep prioritization usable.
public class SimpleTask<V> extends CompletableTask<V> { @@ - public SimpleTask(String title, Supplier<? extends CompletableFuture<V>> futureSupplier) { + public SimpleTask(String title, Supplier<? extends CompletableFuture<V>> futureSupplier) { this.futureSupplier = futureSupplier; updateTitle(StringUtils.defaultIfBlank(title, StringUtils.EMPTY)); } + + public SimpleTask(Priority priority, String title, Supplier<? extends CompletableFuture<V>> futureSupplier) { + super(priority); + this.futureSupplier = futureSupplier; + updateTitle(StringUtils.defaultIfBlank(title, StringUtils.EMPTY)); + }
22-27: Improve cancellation propagation to underlying future.Cancelling the Task cancels CompletableTask’s internal future, but not the supplier’s returned Future, which may keep running. Track it and cancel as well.
public class SimpleTask<V> extends CompletableTask<V> { private final Supplier<? extends CompletableFuture<V>> futureSupplier; + private volatile Future<V> running; @@ @Override protected V call() throws Exception { - Future<V> task = futureSupplier.get(); - return task.get(); + running = futureSupplier.get(); + try { + return running.get(); + } finally { + running = null; + } } } + +// In CompletableTask (outside this hunk), optionally override cancel to also cancel `running` if present.src/main/java/com/faforever/client/task/TaskService.java (1)
57-65: NPE and title resolution edge-cases.
- Guard null Supplier.
- Consider deferring i18n lookup to FX thread if resource bundles can change at runtime (optional).
- public <V> CompletableFuture<V> submitFutureTask(String titleI18nKey, Supplier<CompletableFuture<V>> task) { - String title = titleI18nKey != null ? i18n.get(titleI18nKey) : null; - return submitTask(new SimpleTask<>(title, task)).getFuture(); + public <V> CompletableFuture<V> submitFutureTask(String titleI18nKey, Supplier<CompletableFuture<V>> task) { + if (task == null) { + return CompletableFuture.failedFuture(new NullPointerException("task supplier must not be null")); + } + String title = titleI18nKey != null ? i18n.get(titleI18nKey) : null; + return submitTask(new SimpleTask<>(title, task)).getFuture(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/java/com/faforever/client/game/GameRunner.java(8 hunks)src/main/java/com/faforever/client/task/CompletableTask.java(1 hunks)src/main/java/com/faforever/client/task/SimpleTask.java(1 hunks)src/main/java/com/faforever/client/task/TaskService.java(5 hunks)src/main/java/com/faforever/client/ui/statusbar/StatusBarController.java(1 hunks)src/main/resources/i18n/messages.properties(1 hunks)src/test/java/com/faforever/client/game/GameRunnerTest.java(6 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 (1)
src/test/java/com/faforever/client/game/GameRunnerTest.java (1)
205-209: Mocking approach to run submitted future tasks looks good.Capturing the Supplier and joining the returned future ensures tasks complete within tests. This keeps assertions deterministic.
Collaborator
Author
|
@coderabbitai resolve |
✅ Actions performedComments resolved. |
Sheikah45
approved these changes
Oct 25, 2025
Sheikah45
force-pushed
the
feature/more-info-before-running-game
branch
from
6df65df to
bf5f5f1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/main/java/com/faforever/client/game/GameRunner.java (2)
183-183: Confusing variable name:hasLeague.The variable
hasLeagueistruewhen the leaderboard isnullor"global", and league info is loaded only whenhasLeagueisfalse. This naming is counterintuitive.Consider renaming for clarity:
- boolean hasLeague = leaderboard == null || "global".equals(leaderboard); + boolean needsDivisionInfo = leaderboard != null && !"global".equals(leaderboard);Then update line 189:
- CompletableFuture<League> loadLeagueInfoFuture = hasLeague ? completedFuture(null) : taskService.submitFutureTask("league.loadInfo", () -> getDivisionInfo(leaderboard).toFuture()); + CompletableFuture<League> loadLeagueInfoFuture = needsDivisionInfo ? taskService.submitFutureTask("league.loadInfo", () -> getDivisionInfo(leaderboard).toFuture()) : completedFuture(null);Also applies to: 189-189
193-196: Consider avoiding blocking.join()calls in async chain.Lines 194-196 call
.join()on futures insidethenApply. While these futures are already complete (afterallOf), this pattern can block the completion thread and is considered a code smell.Consider extracting values after
allOfcompletes:return CompletableFuture.allOf(downloadMapFuture, loadLeagueInfoFuture, runReplayServerFuture, runIceAdapterFuture) - .thenApply(_ -> gameMapper.map(gameLaunchResponse, loadLeagueInfoFuture.join())) - .thenApply(parameters -> launchOnlineGame(parameters, runIceAdapterFuture.join(), - runReplayServerFuture.join())) + .thenApply(_ -> { + League league = loadLeagueInfoFuture.join(); + Integer icePort = runIceAdapterFuture.join(); + Integer replayPort = runReplayServerFuture.join(); + GameParameters parameters = gameMapper.map(gameLaunchResponse, league); + return launchOnlineGame(parameters, icePort, replayPort); + })Alternatively, use
thenComposewiththenCombineto avoid explicit joins.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/java/com/faforever/client/game/GameRunner.java(8 hunks)src/main/java/com/faforever/client/task/CompletableTask.java(1 hunks)src/main/java/com/faforever/client/task/SimpleTask.java(1 hunks)src/main/java/com/faforever/client/task/TaskService.java(5 hunks)src/main/java/com/faforever/client/ui/statusbar/StatusBarController.java(1 hunks)src/main/resources/i18n/messages.properties(1 hunks)src/test/java/com/faforever/client/game/GameRunnerTest.java(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/main/java/com/faforever/client/task/CompletableTask.java
- src/main/resources/i18n/messages.properties
- src/test/java/com/faforever/client/game/GameRunnerTest.java
- src/main/java/com/faforever/client/task/TaskService.java
- src/main/java/com/faforever/client/ui/statusbar/StatusBarController.java
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-24T20:19:47.769Z
Learnt from: Brutus5000
PR: FAForever/downlords-faf-client#3417
File: src/main/java/com/faforever/client/task/CompletableTask.java:22-25
Timestamp: 2025-10-24T20:19:47.769Z
Learning: In Java 21 and later (via JEP 443), the single underscore `_` is the canonical and valid way to declare unnamed variables and patterns, including unused lambda parameters. Do not flag `_` as a compilation error in projects using Java 21 or newer.
Applied to files:
src/main/java/com/faforever/client/game/GameRunner.java
📚 Learning: 2025-10-24T20:18:48.136Z
Learnt from: Brutus5000
PR: FAForever/downlords-faf-client#3417
File: src/main/java/com/faforever/client/game/GameRunner.java:488-491
Timestamp: 2025-10-24T20:18:48.136Z
Learning: In Java 21 and later, the single underscore `_` is the standard and correct syntax for unnamed variables and unused lambda parameters. Do not flag `_` as a compilation error in lambda expressions when the project uses Java 21+.
Applied to files:
src/main/java/com/faforever/client/game/GameRunner.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). (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: checks
- GitHub Check: checks
🔇 Additional comments (6)
src/main/java/com/faforever/client/task/SimpleTask.java (1)
9-26: LGTM! Clean wrapper for async operations.The implementation correctly wraps
CompletableFuturesuppliers into theTaskframework, enabling progress tracking in the status bar. The blocking call totask.get()on line 25 is appropriate since this runs in a background thread managed by the task service.src/main/java/com/faforever/client/game/GameRunner.java (5)
41-41: LGTM! TaskService integration.The
TaskServicedependency is properly added to support progress tracking for game launch operations.Also applies to: 122-122
168-172: Good refactor to reactive subscription.The connection state handling now uses
subscribeinstead ofaddListener, which is cleaner and more idiomatic. The logic correctly restores the game session when reconnecting.
153-153: LGTM! Appropriate use of unnamed variables.The lambda parameters are correctly updated to use
_(unnamed variable) to explicitly indicate unused parameters. This is the canonical syntax in Java 21+ and improves code clarity.Also applies to: 194-197, 205-213, 488-489, 514-514, 524-525, 557-557
197-213: Well-structured process lifecycle management.The completion chain properly:
- Sets process state on the FX thread (lines 197-202)
- Waits for process exit (line 203)
- Performs cleanup operations (lines 205-209)
- Clears state on the FX thread (lines 210-213)
The sequencing ensures cleanup happens before final state updates, and FX thread safety is maintained.
189-191: All i18n keys are properly defined.The verification confirms that all three i18n keys exist in the localization files at
src/main/resources/i18n/messages.properties:
league.loadInfo(line 1319)replayServer.connecting(line 1318)iceAdapter.connecting(line 1317)No issues found.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #3400
Summary by CodeRabbit
New Features
Tests