From 6c8021f2196e74ecec96c9a79efc2a7c359d4462 Mon Sep 17 00:00:00 2001 From: davotoula Date: Mon, 23 Mar 2026 20:37:57 +0100 Subject: [PATCH] code review fixes: Deduplicate chess notify() into shared notifyChessEvent() helper using BaseChessEvent Remove addCompletedGameDirectly, extend moveToCompleted with optional liveState param Hoist currentUser lookup to top of ChessLobbyContent, remove 4 duplicate remembers Add missing imports in desktop ChessScreen, replace all FQN usages with short names Remove redundant distinctBy in completed games UI (state already prevents duplicates) --- .../EventNotificationConsumer.kt | 71 ++++++--------- .../screen/loggedIn/chess/ChessLobbyScreen.kt | 29 ++----- .../amethyst/commons/chess/ChessLobbyLogic.kt | 3 +- .../amethyst/commons/chess/ChessLobbyState.kt | 87 ++++--------------- .../amethyst/desktop/chess/ChessScreen.kt | 35 +++++--- 5 files changed, 76 insertions(+), 149 deletions(-) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/notifications/EventNotificationConsumer.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/notifications/EventNotificationConsumer.kt index 55f0849b9..e1252c1c7 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/notifications/EventNotificationConsumer.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/notifications/EventNotificationConsumer.kt @@ -48,6 +48,7 @@ import com.vitorpamplona.quartz.nip57Zaps.LnZapEvent import com.vitorpamplona.quartz.nip57Zaps.LnZapRequestEvent import com.vitorpamplona.quartz.nip59Giftwrap.seals.SealedRumorEvent import com.vitorpamplona.quartz.nip59Giftwrap.wraps.GiftWrapEvent +import com.vitorpamplona.quartz.nip64Chess.baseEvent.BaseChessEvent import com.vitorpamplona.quartz.nip64Chess.challenge.accept.LiveChessGameAcceptEvent import com.vitorpamplona.quartz.nip64Chess.move.LiveChessMoveEvent import com.vitorpamplona.quartz.utils.Log @@ -107,12 +108,29 @@ class EventNotificationConsumer( Log.d(TAG, "Unwrapped consume ${innerEvent.javaClass.simpleName}") when (innerEvent) { - is PrivateDmEvent -> notify(innerEvent, account) - is LnZapEvent -> notify(innerEvent, account) - is ChatMessageEvent -> notify(innerEvent, account) - is ChatMessageEncryptedFileHeaderEvent -> notify(innerEvent, account) - is LiveChessGameAcceptEvent -> notify(innerEvent, account) - is LiveChessMoveEvent -> notify(innerEvent, account) + is PrivateDmEvent -> { + notify(innerEvent, account) + } + + is LnZapEvent -> { + notify(innerEvent, account) + } + + is ChatMessageEvent -> { + notify(innerEvent, account) + } + + is ChatMessageEncryptedFileHeaderEvent -> { + notify(innerEvent, account) + } + + is LiveChessGameAcceptEvent -> { + notifyChessEvent(innerEvent, account, R.string.app_notification_chess_challenge_accepted) + } + + is LiveChessMoveEvent -> { + notifyChessEvent(innerEvent, account, R.string.app_notification_chess_your_turn) + } } } } @@ -486,11 +504,11 @@ class EventNotificationConsumer( } } - private suspend fun notify( - event: LiveChessGameAcceptEvent, + private suspend fun notifyChessEvent( + event: BaseChessEvent, account: Account, + contentStringRes: Int, ) { - Log.d(TAG, "New Chess Challenge Accept to Notify") if ( event.createdAt > TimeUtils.fifteenMinutesAgo() && event.pubKey != account.signer.pubKey @@ -499,40 +517,7 @@ class EventNotificationConsumer( val user = author.toBestDisplayName() val userPicture = author.profilePicture() val title = stringRes(applicationContext, R.string.app_notification_chess_channel_name) - val content = stringRes(applicationContext, R.string.app_notification_chess_challenge_accepted, user) - val noteUri = - "notifications$ACCOUNT_QUERY_PARAM" + - account.signer.pubKey - .hexToByteArray() - .toNpub() - - notificationManager() - .sendChessNotification( - event.id, - content, - title, - event.createdAt, - userPicture, - noteUri, - applicationContext, - ) - } - } - - private suspend fun notify( - event: LiveChessMoveEvent, - account: Account, - ) { - Log.d(TAG, "New Chess Move to Notify") - if ( - event.createdAt > TimeUtils.fifteenMinutesAgo() && - event.pubKey != account.signer.pubKey - ) { - val author = LocalCache.getOrCreateUser(event.pubKey) - val user = author.toBestDisplayName() - val userPicture = author.profilePicture() - val title = stringRes(applicationContext, R.string.app_notification_chess_channel_name) - val content = stringRes(applicationContext, R.string.app_notification_chess_your_turn, user) + val content = stringRes(applicationContext, contentStringRes, user) val noteUri = "notifications$ACCOUNT_QUERY_PARAM" + account.signer.pubKey diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/chess/ChessLobbyScreen.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/chess/ChessLobbyScreen.kt index 5a8f0b416..40f04a28a 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/chess/ChessLobbyScreen.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/chess/ChessLobbyScreen.kt @@ -298,6 +298,10 @@ fun ChessLobbyContent( val challenges by chessViewModel.challenges.collectAsState() val completedGames by chessViewModel.completedGames.collectAsState() val userPubkey = accountViewModel.account.userProfile().pubkeyHex + val currentUser = + remember(userPubkey) { + accountViewModel.checkGetOrCreateUser(userPubkey) + } val hasContent = activeGames.isNotEmpty() || @@ -368,10 +372,6 @@ fun ChessLobbyContent( accountViewModel.checkGetOrCreateUser(state.opponentPubkey) } val displayName = opponent?.toBestDisplayName() ?: state.opponentPubkey.take(8) - val playerUser = - remember(state.playerPubkey) { - accountViewModel.checkGetOrCreateUser(state.playerPubkey) - } ActiveGameCard( gameId = gameId, opponentName = displayName, @@ -381,7 +381,7 @@ fun ChessLobbyContent( OverlappingAvatars( avatar1Hex = state.playerPubkey, avatar2Hex = state.opponentPubkey, - avatar1Url = playerUser?.profilePicture(), + avatar1Url = currentUser?.profilePicture(), avatar2Url = opponent?.profilePicture(), ) }, @@ -410,10 +410,6 @@ fun ChessLobbyContent( val opponentName = opponentUser?.toBestDisplayName() ?: challenge.opponentPubkey?.take(8) - val currentUser = - remember(userPubkey) { - accountViewModel.checkGetOrCreateUser(userPubkey) - } OutgoingChallengeCard( opponentName = opponentName, userPlaysWhite = challenge.challengerColor == ChessColor.WHITE, @@ -477,10 +473,6 @@ fun ChessLobbyContent( ?: accountViewModel.checkGetOrCreateUser(challenge.challengerPubkey)?.toBestDisplayName() ?: challenge.challengerPubkey.take(8) } - val currentUser = - remember(userPubkey) { - accountViewModel.checkGetOrCreateUser(userPubkey) - } ChallengeCard( challengerName = displayName, challengerPlaysWhite = challenge.challengerColor == ChessColor.WHITE, @@ -518,10 +510,6 @@ fun ChessLobbyContent( ?: accountViewModel.checkGetOrCreateUser(challenge.challengerPubkey)?.toBestDisplayName() ?: challenge.challengerPubkey.take(8) } - val currentUser = - remember(userPubkey) { - accountViewModel.checkGetOrCreateUser(userPubkey) - } ChallengeCard( challengerName = displayName, challengerPlaysWhite = challenge.challengerColor == ChessColor.WHITE, @@ -576,7 +564,7 @@ fun ChessLobbyContent( } items( - completedGames.distinctBy { it.gameId }.take(10), + completedGames.take(10), key = { "completed-${it.gameId}-${it.completedAt}" }, ) { game -> val opponentPubkey = @@ -601,10 +589,7 @@ fun ChessLobbyContent( OverlappingAvatars( avatar1Hex = userPubkey, avatar2Hex = opponentPubkey, - avatar1Url = - remember(userPubkey) { - accountViewModel.checkGetOrCreateUser(userPubkey) - }?.profilePicture(), + avatar1Url = currentUser?.profilePicture(), avatar2Url = opponentUser?.profilePicture(), ) }, diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/chess/ChessLobbyLogic.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/chess/ChessLobbyLogic.kt index 8bb9c3c92..1530bb3dc 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/chess/ChessLobbyLogic.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/chess/ChessLobbyLogic.kt @@ -778,8 +778,7 @@ class ChessLobbyLogic( // If the discovered game is already finished, send it straight to completed val gameStatus = result.liveState.gameStatus.value if (gameStatus is GameStatus.Finished) { - // Use the direct helper to avoid the addActiveGame → moveToCompleted flicker - state.addCompletedGameDirectly(startEventId, result.liveState, gameStatus.result.notation, null) + state.moveToCompleted(startEventId, gameStatus.result.notation, null, liveState = result.liveState) } else if (!result.liveState.isSpectator) { state.addActiveGame(startEventId, result.liveState) pollingDelegate.addGameId(startEventId) diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/chess/ChessLobbyState.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/chess/ChessLobbyState.kt index 9604fff1d..73814c801 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/chess/ChessLobbyState.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/chess/ChessLobbyState.kt @@ -394,93 +394,35 @@ class ChessLobbyState( } /** - * Move a game from active/spectating to completed. - * Display names are optional; UI can look them up later if needed. + * Move a game to completed. Looks up the game from active/spectating maps, + * or uses the provided [liveState] if the game isn't in either map yet + * (e.g. a newly discovered game that is already finished). */ fun moveToCompleted( gameId: String, result: String, termination: String?, + liveState: LiveChessGameState? = null, whiteDisplayName: String? = null, blackDisplayName: String? = null, ) { - val existingState = _activeGames.value[gameId] ?: _spectatingGames.value[gameId] - existingState?.let { gameState -> - // Derive white/black pubkeys from player color - val whitePubkey = - if (gameState.playerColor == Color.WHITE) { - gameState.playerPubkey - } else { - gameState.opponentPubkey - } - val blackPubkey = - if (gameState.playerColor == Color.BLACK) { - gameState.playerPubkey - } else { - gameState.opponentPubkey - } + val gameState = _activeGames.value[gameId] ?: _spectatingGames.value[gameId] ?: liveState ?: return - val completed = - CompletedGame( - gameId = gameId, - whitePubkey = whitePubkey, - whiteDisplayName = whiteDisplayName, - blackPubkey = blackPubkey, - blackDisplayName = blackDisplayName, - result = result, - termination = termination, - moveCount = gameState.moveHistory.value.size, - completedAt = - com.vitorpamplona.quartz.utils.TimeUtils - .now(), - ) - - _completedGames.update { current -> - // Avoid duplicates - if (current.any { it.gameId == gameId }) { - current - } else { - listOf(completed) + current - } - } - - // Remove from active/spectating - _activeGames.update { it - gameId } - _spectatingGames.update { it - gameId } - - // Clear selection if this game was selected - if (_selectedGameId.value == gameId) { - _selectedGameId.value = null - } - } - } - - /** - * Build and record a CompletedGame directly from a LiveChessGameState without requiring the - * game to be inserted into activeGames first. Use this when a newly discovered game is already - * finished so we avoid the intermediate addActiveGame → moveToCompleted flicker. - */ - fun addCompletedGameDirectly( - gameId: String, - liveState: LiveChessGameState, - result: String, - termination: String?, - ) { val whitePubkey = - if (liveState.playerColor == Color.WHITE) liveState.playerPubkey else liveState.opponentPubkey + if (gameState.playerColor == Color.WHITE) gameState.playerPubkey else gameState.opponentPubkey val blackPubkey = - if (liveState.playerColor == Color.BLACK) liveState.playerPubkey else liveState.opponentPubkey + if (gameState.playerColor == Color.BLACK) gameState.playerPubkey else gameState.opponentPubkey val completed = CompletedGame( gameId = gameId, whitePubkey = whitePubkey, - whiteDisplayName = null, + whiteDisplayName = whiteDisplayName, blackPubkey = blackPubkey, - blackDisplayName = null, + blackDisplayName = blackDisplayName, result = result, termination = termination, - moveCount = liveState.moveHistory.value.size, + moveCount = gameState.moveHistory.value.size, completedAt = com.vitorpamplona.quartz.utils.TimeUtils .now(), @@ -489,6 +431,15 @@ class ChessLobbyState( _completedGames.update { current -> if (current.any { it.gameId == gameId }) current else listOf(completed) + current } + + // Remove from active/spectating + _activeGames.update { it - gameId } + _spectatingGames.update { it - gameId } + + // Clear selection if this game was selected + if (_selectedGameId.value == gameId) { + _selectedGameId.value = null + } } fun addSpectatingGame( diff --git a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/chess/ChessScreen.kt b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/chess/ChessScreen.kt index 3d83bdf29..b218acbc0 100644 --- a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/chess/ChessScreen.kt +++ b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/chess/ChessScreen.kt @@ -43,7 +43,6 @@ import androidx.compose.material.icons.automirrored.filled.ArrowBack import androidx.compose.material.icons.automirrored.filled.KeyboardArrowLeft import androidx.compose.material.icons.automirrored.filled.KeyboardArrowRight import androidx.compose.material.icons.filled.Add -import androidx.compose.material.icons.filled.ArrowBack import androidx.compose.material.icons.filled.Refresh import androidx.compose.material.icons.filled.Visibility import androidx.compose.material3.Button @@ -67,14 +66,22 @@ import androidx.compose.ui.Modifier import androidx.compose.ui.text.font.FontWeight import androidx.compose.ui.unit.dp import androidx.compose.ui.unit.min +import com.vitorpamplona.amethyst.commons.chess.ActiveGameCard +import com.vitorpamplona.amethyst.commons.chess.ChallengeCard import com.vitorpamplona.amethyst.commons.chess.ChessBroadcastBanner import com.vitorpamplona.amethyst.commons.chess.ChessChallenge import com.vitorpamplona.amethyst.commons.chess.ChessConfig +import com.vitorpamplona.amethyst.commons.chess.ChessPlayerVsHeader import com.vitorpamplona.amethyst.commons.chess.ChessSyncBanner import com.vitorpamplona.amethyst.commons.chess.CompletedGame +import com.vitorpamplona.amethyst.commons.chess.CompletedGameCard import com.vitorpamplona.amethyst.commons.chess.InteractiveChessBoard import com.vitorpamplona.amethyst.commons.chess.NewChessGameDialog +import com.vitorpamplona.amethyst.commons.chess.OutgoingChallengeCard +import com.vitorpamplona.amethyst.commons.chess.OverlappingAvatars import com.vitorpamplona.amethyst.commons.chess.PublicGame +import com.vitorpamplona.amethyst.commons.chess.PublicGameCard +import com.vitorpamplona.amethyst.commons.chess.SpectatingGameCard import com.vitorpamplona.amethyst.commons.data.UserMetadataCache import com.vitorpamplona.amethyst.commons.ui.components.UserAvatar import com.vitorpamplona.amethyst.desktop.account.AccountState @@ -415,13 +422,13 @@ private fun ChessLobby( } items(activeGames.entries.toList(), key = { "active-${it.key}" }) { (gameId, state) -> - com.vitorpamplona.amethyst.commons.chess.ActiveGameCard( + ActiveGameCard( gameId = gameId, opponentName = metadataCache.getDisplayName(state.opponentPubkey), isYourTurn = state.isPlayerTurn(), onClick = { onSelectGame(gameId) }, avatar = { - com.vitorpamplona.amethyst.commons.chess.OverlappingAvatars( + OverlappingAvatars( avatar1Hex = state.playerPubkey, avatar2Hex = state.opponentPubkey, avatar1Url = metadataCache.getPictureUrl(state.playerPubkey), @@ -446,14 +453,14 @@ private fun ChessLobby( } items(outgoingChallenges, key = { "outgoing-${it.eventId}" }) { challenge -> - com.vitorpamplona.amethyst.commons.chess.OutgoingChallengeCard( + OutgoingChallengeCard( opponentName = challenge.opponentPubkey?.let { metadataCache.getDisplayName(it) }, userPlaysWhite = challenge.challengerColor == ChessColor.WHITE, onClick = { onOpenOwnChallenge(challenge) }, avatar = challenge.opponentPubkey?.let { pubkey -> { - com.vitorpamplona.amethyst.commons.chess.OverlappingAvatars( + OverlappingAvatars( avatar1Hex = userPubkey, avatar2Hex = pubkey, avatar1Url = metadataCache.getPictureUrl(userPubkey), @@ -479,7 +486,7 @@ private fun ChessLobby( items(spectatingGames.entries.toList(), key = { "spectating-${it.key}" }) { (gameId, state) -> val moveCount by state.moveHistory.collectAsState() - com.vitorpamplona.amethyst.commons.chess.SpectatingGameCard( + SpectatingGameCard( moveCount = moveCount.size, onClick = { onSelectGame(gameId) }, ) @@ -500,13 +507,13 @@ private fun ChessLobby( } items(incomingChallenges, key = { "incoming-${it.eventId}" }) { challenge -> - com.vitorpamplona.amethyst.commons.chess.ChallengeCard( + ChallengeCard( challengerName = challenge.challengerDisplayName ?: metadataCache.getDisplayName(challenge.challengerPubkey), challengerPlaysWhite = challenge.challengerColor == ChessColor.WHITE, isIncoming = true, onAccept = { onAcceptChallenge(challenge) }, avatar = { - com.vitorpamplona.amethyst.commons.chess.OverlappingAvatars( + OverlappingAvatars( avatar1Hex = challenge.challengerPubkey, avatar2Hex = userPubkey, avatar1Url = challenge.challengerAvatarUrl ?: metadataCache.getPictureUrl(challenge.challengerPubkey), @@ -531,13 +538,13 @@ private fun ChessLobby( } items(openChallenges, key = { "open-${it.eventId}" }) { challenge -> - com.vitorpamplona.amethyst.commons.chess.ChallengeCard( + ChallengeCard( challengerName = challenge.challengerDisplayName ?: metadataCache.getDisplayName(challenge.challengerPubkey), challengerPlaysWhite = challenge.challengerColor == ChessColor.WHITE, isIncoming = false, onAccept = { onAcceptChallenge(challenge) }, avatar = { - com.vitorpamplona.amethyst.commons.chess.OverlappingAvatars( + OverlappingAvatars( avatar1Hex = challenge.challengerPubkey, avatar2Hex = userPubkey, avatar1Url = challenge.challengerAvatarUrl ?: metadataCache.getPictureUrl(challenge.challengerPubkey), @@ -561,7 +568,7 @@ private fun ChessLobby( } items(publicGames, key = { "public-${it.gameId}" }) { game -> - com.vitorpamplona.amethyst.commons.chess.PublicGameCard( + PublicGameCard( whiteName = game.whiteDisplayName ?: metadataCache.getDisplayName(game.whitePubkey), blackName = game.blackDisplayName ?: metadataCache.getDisplayName(game.blackPubkey), moveCount = game.moveCount, @@ -583,13 +590,13 @@ private fun ChessLobby( } items( - completedGames.distinctBy { it.gameId }.take(10), + completedGames.take(10), key = { "completed-${it.gameId}-${it.completedAt}" }, ) { game -> // Derive opponent pubkey based on who the user is val opponentPubkey = if (game.whitePubkey == userPubkey) game.blackPubkey else game.whitePubkey - com.vitorpamplona.amethyst.commons.chess.CompletedGameCard( + CompletedGameCard( opponentName = game.blackDisplayName ?: game.whiteDisplayName ?: metadataCache.getDisplayName(opponentPubkey), result = game.result, didUserWin = game.didUserWin(userPubkey), @@ -674,7 +681,7 @@ private fun DesktopChessGameLayout( ) { // Player vs Player header above board if (whiteHex.isNotEmpty() || blackHex.isNotEmpty()) { - com.vitorpamplona.amethyst.commons.chess.ChessPlayerVsHeader( + ChessPlayerVsHeader( whiteName = whiteName, whiteHex = whiteHex, whiteAvatarUrl = whiteAvatarUrl,