From 7d71a4b86a3ee06b9bbf4e5b14288e4cd30a241c Mon Sep 17 00:00:00 2001 From: Francin Vincent Date: Mon, 6 Apr 2026 22:41:21 +0200 Subject: [PATCH] fix: add listener lifecycle management and confirmSetup error handling (#52, #53) - Track all Firebase listeners via cleanup runnables in AndroidFirebase - Add removeAllListeners() to FirebaseAPI interface - Add stop() to GameNetworkHandler, called from GameScreen hide/dispose - Fix confirmSetup: propagate errors to onError callback instead of silently calling onSuccess on failure paths - Update SetupFlowController to pass error callback to confirmSetup --- .../android/AndroidFirebase.java | 69 ++++++++++++------- .../regicidechess/database/FirebaseAPI.java | 6 +- .../screens/game/GameNetworkHandler.java | 6 +- .../screens/game/GameScreen.java | 4 +- .../screens/setup/SetupFlowController.java | 5 +- 5 files changed, 59 insertions(+), 31 deletions(-) diff --git a/android/src/main/java/com/group14/regicidechess/android/AndroidFirebase.java b/android/src/main/java/com/group14/regicidechess/android/AndroidFirebase.java index b57bf72..4e311f6 100644 --- a/android/src/main/java/com/group14/regicidechess/android/AndroidFirebase.java +++ b/android/src/main/java/com/group14/regicidechess/android/AndroidFirebase.java @@ -22,6 +22,17 @@ public class AndroidFirebase implements FirebaseAPI, API { private final DatabaseReference db = FirebaseDatabase.getInstance().getReference(); + private final List cleanupActions = new ArrayList<>(); + + private ValueEventListener trackValue(DatabaseReference ref, ValueEventListener listener) { + cleanupActions.add(() -> ref.removeEventListener(listener)); + return listener; + } + + private ChildEventListener trackChild(DatabaseReference ref, ChildEventListener listener) { + cleanupActions.add(() -> ref.removeEventListener(listener)); + return listener; + } // ── API (Legacy) ────────────────────────────────────────────────────────── @@ -74,8 +85,8 @@ public void joinLobby(String gameId, Callback onSuccess, Callback @Override public void listenForOpponentReady(String gameId, Runnable onReady) { - db.child("lobbies").child(gameId).child("status") - .addValueEventListener(new ValueEventListener() { + DatabaseReference statusRef = db.child("lobbies").child(gameId).child("status"); + statusRef.addValueEventListener(trackValue(statusRef, new ValueEventListener() { @Override public void onDataChange(DataSnapshot snapshot) { if ("joined".equals(snapshot.getValue(String.class))) { @@ -84,7 +95,7 @@ public void onDataChange(DataSnapshot snapshot) { } } @Override public void onCancelled(DatabaseError e) {} - }); + })); } @Override @@ -94,8 +105,8 @@ public void startGame(String gameId) { @Override public void listenForGameStart(String gameId, Runnable onStart) { - db.child("lobbies").child(gameId).child("status") - .addValueEventListener(new ValueEventListener() { + DatabaseReference statusRef = db.child("lobbies").child(gameId).child("status"); + statusRef.addValueEventListener(trackValue(statusRef, new ValueEventListener() { @Override public void onDataChange(DataSnapshot snapshot) { if ("started".equals(snapshot.getValue(String.class))) { @@ -104,13 +115,13 @@ public void onDataChange(DataSnapshot snapshot) { } } @Override public void onCancelled(DatabaseError e) {} - }); + })); } // ── Setup ───────────────────────────────────────────────────────────────── @Override - public void confirmSetup(String gameId, boolean isWhite, int[][] board, Runnable onSuccess) { + public void confirmSetup(String gameId, boolean isWhite, int[][] board, Runnable onSuccess, Callback onError) { String player = isWhite ? "white" : "black"; DatabaseReference gameRef = db.child("games").child(gameId); @@ -141,11 +152,12 @@ public void onDataChange(DataSnapshot snapshot) { } onSuccess.run(); } - @Override public void onCancelled(DatabaseError e) { onSuccess.run(); } + @Override public void onCancelled(DatabaseError e) { onError.call(e.getMessage()); } }) ) + .addOnFailureListener(e -> onError.call(e.getMessage())) ) - .addOnFailureListener(e -> onSuccess.run()); + .addOnFailureListener(e -> onError.call(e.getMessage())); } @Override @@ -172,8 +184,8 @@ public void getOpponentBoard(String gameId, boolean localIsWhite, Callback onMove) { lastExistingKey[0] = child.getKey(); } - db.child("games").child(gameId).child("moves") - .addChildEventListener(new ChildEventListener() { + DatabaseReference movesRef = db.child("games").child(gameId).child("moves"); + movesRef.addChildEventListener(trackChild(movesRef, new ChildEventListener() { private boolean seenStart = (lastExistingKey[0] == null); @Override @@ -257,12 +269,12 @@ public void onChildAdded(DataSnapshot snapshot, String prev) { @Override public void onChildRemoved(DataSnapshot s) {} @Override public void onChildMoved (DataSnapshot s, String p) {} @Override public void onCancelled (DatabaseError e) {} - }); + })); }) .addOnFailureListener(e -> { // Fallback: attach listener without filtering (original behaviour) - db.child("games").child(gameId).child("moves") - .addChildEventListener(new ChildEventListener() { + DatabaseReference movesRef = db.child("games").child(gameId).child("moves"); + movesRef.addChildEventListener(trackChild(movesRef, new ChildEventListener() { @Override public void onChildAdded(DataSnapshot snapshot, String prev) { int fromCol = getInt(snapshot, "fromCol"); @@ -277,7 +289,7 @@ public void onChildAdded(DataSnapshot snapshot, String prev) { @Override public void onChildRemoved(DataSnapshot s) {} @Override public void onChildMoved (DataSnapshot s, String p) {} @Override public void onCancelled (DatabaseError e) {} - }); + })); }); } @@ -296,12 +308,13 @@ public void listenForHeartbeat(String gameId, boolean listenForWhite, String player = listenForWhite ? "white" : "black"; Handler handler = new Handler(Looper.getMainLooper()); Runnable timeoutRunnable = onTimeout::run; + cleanupActions.add(() -> handler.removeCallbacks(timeoutRunnable)); // Start timeout timer — reset each time a valid heartbeat arrives handler.postDelayed(timeoutRunnable, timeoutMs); - db.child("games").child(gameId).child("heartbeat").child(player) - .addValueEventListener(new ValueEventListener() { + DatabaseReference heartbeatRef = db.child("games").child(gameId).child("heartbeat").child(player); + heartbeatRef.addValueEventListener(trackValue(heartbeatRef, new ValueEventListener() { @Override public void onDataChange(DataSnapshot snapshot) { Long timestamp = snapshot.getValue(Long.class); @@ -319,7 +332,7 @@ public void onDataChange(DataSnapshot snapshot) { onHeartbeat.call(latency); } @Override public void onCancelled(DatabaseError e) {} - }); + })); } // ── Game over ───────────────────────────────────────────────────────────── @@ -331,8 +344,8 @@ public void signalGameOver(String gameId, String reason) { @Override public void listenForGameOver(String gameId, Callback onGameOver) { - db.child("games").child(gameId).child("gameOver") - .addValueEventListener(new ValueEventListener() { + DatabaseReference gameOverRef = db.child("games").child(gameId).child("gameOver"); + gameOverRef.addValueEventListener(trackValue(gameOverRef, new ValueEventListener() { @Override public void onDataChange(DataSnapshot snapshot) { String reason = snapshot.getValue(String.class); @@ -342,7 +355,7 @@ public void onDataChange(DataSnapshot snapshot) { } } @Override public void onCancelled(DatabaseError e) {} - }); + })); } // ───────────────────────────────────────────────────────────────────────── @@ -370,4 +383,10 @@ public void unconfirmSetup(String gameId, boolean isWhite, Runnable onSuccess, C }) .addOnFailureListener(e -> onError.call(e.getMessage())); } -} \ No newline at end of file + + @Override + public void removeAllListeners() { + for (Runnable cleanup : cleanupActions) cleanup.run(); + cleanupActions.clear(); + } +} diff --git a/core/src/main/java/com/group14/regicidechess/database/FirebaseAPI.java b/core/src/main/java/com/group14/regicidechess/database/FirebaseAPI.java index c88bd7b..212ba4d 100644 --- a/core/src/main/java/com/group14/regicidechess/database/FirebaseAPI.java +++ b/core/src/main/java/com/group14/regicidechess/database/FirebaseAPI.java @@ -38,7 +38,7 @@ public interface FirebaseAPI { * Saves this player's board layout and marks them as setup-ready. * Sets games/{gameId}/bothReady = true once BOTH players have called this. */ - void confirmSetup(String gameId, boolean isWhite, int[][] board, Runnable onSuccess); + void confirmSetup(String gameId, boolean isWhite, int[][] board, Runnable onSuccess, Callback onError); /** * Unconfirms the setup, marking this player as not ready. @@ -80,9 +80,11 @@ void listenForHeartbeat(String gameId, boolean listenForWhite, long timeoutMs, void signalGameOver(String gameId, String reason); void listenForGameOver(String gameId, Callback onGameOver); + void removeAllListeners(); + // ───────────────────────────────────────────────────────────────────────── interface Callback { void call(T value); } -} \ No newline at end of file +} diff --git a/core/src/main/java/com/group14/regicidechess/screens/game/GameNetworkHandler.java b/core/src/main/java/com/group14/regicidechess/screens/game/GameNetworkHandler.java index 4a70618..f33bcb1 100644 --- a/core/src/main/java/com/group14/regicidechess/screens/game/GameNetworkHandler.java +++ b/core/src/main/java/com/group14/regicidechess/screens/game/GameNetworkHandler.java @@ -68,6 +68,10 @@ public void start() { startHeartbeat(); } + public void stop() { + DatabaseManager.getInstance().getApi().removeAllListeners(); + } + // ───────────────────────────────────────────────────────────────────────── // Outgoing // ───────────────────────────────────────────────────────────────────────── @@ -148,4 +152,4 @@ private void updateConnectionUI(long latencyMs) { else connectionIcon.setColor(Color.RED); connectionLabel.setText(latencyMs + " ms"); } -} \ No newline at end of file +} diff --git a/core/src/main/java/com/group14/regicidechess/screens/game/GameScreen.java b/core/src/main/java/com/group14/regicidechess/screens/game/GameScreen.java index 14cd5e5..5aa8f77 100644 --- a/core/src/main/java/com/group14/regicidechess/screens/game/GameScreen.java +++ b/core/src/main/java/com/group14/regicidechess/screens/game/GameScreen.java @@ -438,12 +438,14 @@ public void resize(int width, int height) { @Override public void hide() { inputHandler.clearObservers(); + networkHandler.stop(); inMatchState.exit(); } @Override public void dispose() { + networkHandler.stop(); stage.dispose(); boardRenderer.dispose(); } -} \ No newline at end of file +} diff --git a/core/src/main/java/com/group14/regicidechess/screens/setup/SetupFlowController.java b/core/src/main/java/com/group14/regicidechess/screens/setup/SetupFlowController.java index 99dc3f5..df56334 100644 --- a/core/src/main/java/com/group14/regicidechess/screens/setup/SetupFlowController.java +++ b/core/src/main/java/com/group14/regicidechess/screens/setup/SetupFlowController.java @@ -69,7 +69,8 @@ public void confirm() { gameId, localPlayer.isWhite(), encoded, - this::listenForBothReady + this::listenForBothReady, + error -> Gdx.app.postRunnable(() -> listener.onError("Setup failed: " + error)) ); } @@ -144,4 +145,4 @@ private void fetchOpponentBoard() { public boolean isConfirmed() { return isConfirmed; } -} \ No newline at end of file +}