feat: add manual pause for running game#542
Open
xXJSONDeruloXx wants to merge 6 commits intoutkarshdalal:masterfrom
Open
feat: add manual pause for running game#542xXJSONDeruloXx wants to merge 6 commits intoutkarshdalal:masterfrom
xXJSONDeruloXx wants to merge 6 commits intoutkarshdalal:masterfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a user-controlled pause/resume flow: new manual pause flag, lifecycle guards in MainActivity, NavigationDialog action and constructor change, in-screen pause state and overlay, audio component pause/resume methods and XEnvironment lifecycle calls, new pause/play drawables, and localized pause/resume strings. Changes
Sequence DiagramsequenceDiagram
actor User
participant XServerScreen
participant NavigationDialog
participant MainActivity
participant XEnvironment
participant PluviaApp
User->>XServerScreen: open menu
XServerScreen->>NavigationDialog: show(menu, isPaused)
NavigationDialog->>User: display pause/resume item
User->>NavigationDialog: select ACTION_PAUSE_RESUME
NavigationDialog->>XServerScreen: notify ACTION_PAUSE_RESUME
XServerScreen->>PluviaApp: set/clear isManuallyPaused
XServerScreen->>MainActivity: request lifecycle pause/resume
MainActivity->>XEnvironment: call onPause/onResume
XEnvironment->>PulseAudioComponent: pause()/resume()
XEnvironment->>ALSAServerComponent: pause()/resume()
XServerScreen->>XServerScreen: toggle isGamePaused / render overlay
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt`:
- Around line 1144-1168: The pause overlay currently hardcodes visible strings;
replace the literal "PAUSED" and the Icon contentDescription "Game Paused" with
string resources (add e.g. <string name="game_paused">Paused</string>) and use
androidx.compose.ui.res.stringResource(R.string.game_paused) in XServerScreen.kt
where the Text and Icon contentDescription are set (look for isGamePaused block
and the composables Icon and Text) so UI text is localized and accessible.
- Line 329: The local mutable state isGamePaused is reinitialized to false on
recomposition causing UI desync with the global PluviaApp.isManuallyPaused;
replace its initializer with a saveable one seeded from
PluviaApp.isManuallyPaused (use rememberSaveable {
mutableStateOf(PluviaApp.isManuallyPaused) }) and ensure any toggles of
isGamePaused also update PluviaApp.isManuallyPaused so the global and UI stay in
sync (change references to isGamePaused in XServerScreen to maintain this
two‑way update).
| var showElementEditor by remember { mutableStateOf(false) } | ||
| var elementToEdit by remember { mutableStateOf<com.winlator.inputcontrols.ControlElement?>(null) } | ||
| var showPhysicalControllerDialog by remember { mutableStateOf(false) } | ||
| var isGamePaused by remember { mutableStateOf(false) } |
Contributor
There was a problem hiding this comment.
Initialize pause state from the global flag to avoid UI desync.
Line 329 resets isGamePaused on recreation, while PluviaApp.isManuallyPaused can remain true, causing the menu/overlay to show the wrong state after config changes. Initialize from the global flag (and make it saveable).
🔧 Proposed fix
- var isGamePaused by remember { mutableStateOf(false) }
+ var isGamePaused by rememberSaveable { mutableStateOf(PluviaApp.isManuallyPaused) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt` at line
329, The local mutable state isGamePaused is reinitialized to false on
recomposition causing UI desync with the global PluviaApp.isManuallyPaused;
replace its initializer with a saveable one seeded from
PluviaApp.isManuallyPaused (use rememberSaveable {
mutableStateOf(PluviaApp.isManuallyPaused) }) and ensure any toggles of
isGamePaused also update PluviaApp.isManuallyPaused so the global and UI stay in
sync (change references to isGamePaused in XServerScreen to maintain this
two‑way update).
Comment on lines
1144
to
1168
| // Pause indicator overlay | ||
| if (isGamePaused) { | ||
| androidx.compose.foundation.layout.Box( | ||
| modifier = Modifier | ||
| .fillMaxSize() | ||
| .background(androidx.compose.ui.graphics.Color.Black.copy(alpha = 0.5f)), | ||
| contentAlignment = Alignment.Center | ||
| ) { | ||
| androidx.compose.foundation.layout.Column( | ||
| horizontalAlignment = Alignment.CenterHorizontally, | ||
| verticalArrangement = Arrangement.Center | ||
| ) { | ||
| androidx.compose.material3.Icon( | ||
| painter = androidx.compose.ui.res.painterResource(R.drawable.icon_pause), | ||
| contentDescription = "Game Paused", | ||
| modifier = Modifier.size(128.dp), | ||
| tint = androidx.compose.ui.graphics.Color.White | ||
| ) | ||
| androidx.compose.foundation.layout.Spacer(modifier = Modifier.height(16.dp)) | ||
| androidx.compose.material3.Text( | ||
| text = "PAUSED", | ||
| style = androidx.compose.material3.MaterialTheme.typography.displayMedium, | ||
| color = androidx.compose.ui.graphics.Color.White, | ||
| fontWeight = androidx.compose.ui.text.font.FontWeight.Bold | ||
| ) |
Contributor
There was a problem hiding this comment.
Localize pause overlay strings.
Line 1157-1164 hardcodes user-facing text; use string resources for i18n/accessibility.
🌐 Proposed fix
- contentDescription = "Game Paused",
+ contentDescription = stringResource(R.string.game_paused),
@@
- text = "PAUSED",
+ text = stringResource(R.string.game_paused),Add a string resource, e.g.:
<string name="game_paused">Paused</string>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt` around
lines 1144 - 1168, The pause overlay currently hardcodes visible strings;
replace the literal "PAUSED" and the Icon contentDescription "Game Paused" with
string resources (add e.g. <string name="game_paused">Paused</string>) and use
androidx.compose.ui.res.stringResource(R.string.game_paused) in XServerScreen.kt
where the Text and Icon contentDescription are set (look for isGamePaused block
and the composables Icon and Text) so UI text is localized and accessible.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt (1)
1144-1171: Unnecessary fully-qualified names clutter the overlay composable.Most types used here (
Box,Column,Spacer,Icon,Text,MaterialTheme,FontWeight,painterResource) are already imported. Onlyandroidx.compose.ui.graphics.Colorgenuinely conflicts with the existingandroid.graphics.Colorimport.Consider adding an import alias for the Compose
Colorand using short names throughout:Proposed fix
Add at the top of the file with the other imports:
import androidx.compose.ui.graphics.Color as ComposeColorThen simplify the overlay block:
// Pause indicator overlay if (isGamePaused) { - androidx.compose.foundation.layout.Box( + Box( modifier = Modifier .fillMaxSize() - .background(androidx.compose.ui.graphics.Color.Black.copy(alpha = 0.5f)), + .background(ComposeColor.Black.copy(alpha = 0.5f)), contentAlignment = Alignment.Center ) { - androidx.compose.foundation.layout.Column( + Column( horizontalAlignment = Alignment.CenterHorizontally, verticalArrangement = Arrangement.Center ) { - androidx.compose.material3.Icon( - painter = androidx.compose.ui.res.painterResource(R.drawable.icon_pause), + Icon( + painter = painterResource(R.drawable.icon_pause), contentDescription = stringResource(R.string.game_paused), modifier = Modifier.size(128.dp), - tint = androidx.compose.ui.graphics.Color.White + tint = ComposeColor.White ) - androidx.compose.foundation.layout.Spacer(modifier = Modifier.height(16.dp)) - androidx.compose.material3.Text( + Spacer(modifier = Modifier.height(16.dp)) + Text( text = stringResource(R.string.game_paused).uppercase(), - style = androidx.compose.material3.MaterialTheme.typography.displayMedium, - color = androidx.compose.ui.graphics.Color.White, - fontWeight = androidx.compose.ui.text.font.FontWeight.Bold + style = MaterialTheme.typography.displayMedium, + color = ComposeColor.White, + fontWeight = FontWeight.Bold ) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt` around lines 1144 - 1171, The overlay composable uses unnecessary fully-qualified names; add an import alias for Compose Color (e.g., import androidx.compose.ui.graphics.Color as ComposeColor) and then replace occurrences of androidx.compose.ui.graphics.Color with ComposeColor and simplify other fully-qualified types to their short names (Box, Column, Spacer, Icon, Text, MaterialTheme, FontWeight, painterResource) in the isGamePaused block so the code uses the existing imports and the new ComposeColor alias for color references.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt`:
- Line 329: The local mutable state isGamePaused is initialized to false causing
UI desync; change its initializer to use the global PluviaApp.isManuallyPaused
(e.g. var isGamePaused by remember { mutableStateOf(PluviaApp.isManuallyPaused)
}) so recompositions reflect the current global pause state, and if
PluviaApp.isManuallyPaused can change at runtime consider also syncing updates
(e.g. observe/collect or use LaunchedEffect/key on PluviaApp.isManuallyPaused)
to keep isGamePaused in sync with the global value.
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt`:
- Around line 1144-1171: The overlay composable uses unnecessary fully-qualified
names; add an import alias for Compose Color (e.g., import
androidx.compose.ui.graphics.Color as ComposeColor) and then replace occurrences
of androidx.compose.ui.graphics.Color with ComposeColor and simplify other
fully-qualified types to their short names (Box, Column, Spacer, Icon, Text,
MaterialTheme, FontWeight, painterResource) in the isGamePaused block so the
code uses the existing imports and the new ComposeColor alias for color
references.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/res/values-pt-rBR/strings.xml`:
- Line 132: The Brazilian Portuguese locale is missing the pause_game and
resume_game string resources, so add translations for the keys pause_game and
resume_game to the strings.xml locale (matching the existing game_paused entry);
specifically add entries for pause_game with "Pausar Jogo" and resume_game with
"Retomar Jogo" so the UI buttons use localized text instead of English.
In `@app/src/main/res/values-ro/strings.xml`:
- Line 197: The Romanian locale is missing translations for the string resources
pause_game and resume_game, causing mixed-language UI; add two new entries
<string name="pause_game">...</string> and <string
name="resume_game">...</string> to the file alongside the existing <string
name="game_paused">Pausat</string>, providing appropriate Romanian translations
(e.g., "Pauză" for pause_game and "Reia" or "Continuă" for resume_game) so the
pause button labels match the localized overlay text.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/src/main/res/values-pt-rBR/strings.xml (1)
108-109: Minor capitalization inconsistency with nearby strings.Line 107 uses "Sair do Jogo" (uppercase J), while these new strings use lowercase "jogo". Consider aligning to "Pausar Jogo" / "Retomar Jogo" for consistency within this section.
Suggested fix
- <string name="pause_game">Pausar jogo</string> - <string name="resume_game">Retomar jogo</string> + <string name="pause_game">Pausar Jogo</string> + <string name="resume_game">Retomar Jogo</string>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/res/values-pt-rBR/strings.xml` around lines 108 - 109, Update the two Portuguese strings to match the capitalization convention used nearby: change the values for string resources pause_game and resume_game to use an uppercase "J" in "Jogo" (i.e., "Pausar Jogo" and "Retomar Jogo") so they are consistent with "Sair do Jogo".app/src/main/res/values-da/strings.xml (1)
134-134: Nit: consider "Pauseret" instead of "Pause" for the overlay state.The English overlay displays "PAUSED" (a state), while "Pause" is a noun. "Pauseret" would more accurately convey the paused state in Danish, though the current value is understandable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/res/values-da/strings.xml` at line 134, Update the string resource named "game_paused" so the Danish overlay shows the paused state correctly by replacing the current value "Pause" with "Pauseret"; modify the <string name="game_paused"> value to "Pauseret" to reflect the state (string resource identifier: game_paused).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/res/values-ro/strings.xml`:
- Line 199: The string resource named "game_paused" uses the non-standard
Romanian term "Pausat"; update this resource to a more natural phrasing such as
"Pauză" or "În pauză" (to match the existing "pause_game" wording). Locate the
<string name="game_paused"> entry and replace its value with one of the
suggested translations to ensure consistency and standard Romanian usage across
the full-screen overlay.
---
Duplicate comments:
In `@app/src/main/res/values-ro/strings.xml`:
- Around line 173-174: No change required: the translations for pause_game
("Pauză") and resume_game ("Reia") are consistent with existing pause_download
and resume_download entries; keep <string name="pause_game"> and <string
name="resume_game"> as-is and do not modify these resource entries.
---
Nitpick comments:
In `@app/src/main/res/values-da/strings.xml`:
- Line 134: Update the string resource named "game_paused" so the Danish overlay
shows the paused state correctly by replacing the current value "Pause" with
"Pauseret"; modify the <string name="game_paused"> value to "Pauseret" to
reflect the state (string resource identifier: game_paused).
In `@app/src/main/res/values-pt-rBR/strings.xml`:
- Around line 108-109: Update the two Portuguese strings to match the
capitalization convention used nearby: change the values for string resources
pause_game and resume_game to use an uppercase "J" in "Jogo" (i.e., "Pausar
Jogo" and "Retomar Jogo") so they are consistent with "Sair do Jogo".
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt (1)
1144-1171: Remove unnecessary fully-qualified names in the pause overlay.Most types used here (
Box,Column,Spacer,Icon,Text,MaterialTheme,painterResource,FontWeight) are already imported at the top of the file. Onlyandroidx.compose.ui.graphics.Colorgenuinely needs the FQN (sinceandroid.graphics.Coloris imported on line 5). Consider adding a ComposeColorimport alias and using short names for the rest.♻️ Suggested cleanup
Add an import alias at the top:
import androidx.compose.ui.graphics.Color as ComposeColorThen simplify the overlay:
// Pause indicator overlay if (isGamePaused) { - androidx.compose.foundation.layout.Box( + Box( modifier = Modifier .fillMaxSize() - .background(androidx.compose.ui.graphics.Color.Black.copy(alpha = 0.5f)), + .background(ComposeColor.Black.copy(alpha = 0.5f)), contentAlignment = Alignment.Center ) { - androidx.compose.foundation.layout.Column( + Column( horizontalAlignment = Alignment.CenterHorizontally, verticalArrangement = Arrangement.Center ) { - androidx.compose.material3.Icon( - painter = androidx.compose.ui.res.painterResource(R.drawable.icon_pause), + Icon( + painter = painterResource(R.drawable.icon_pause), contentDescription = stringResource(R.string.game_paused), modifier = Modifier.size(128.dp), - tint = androidx.compose.ui.graphics.Color.White + tint = ComposeColor.White ) - androidx.compose.foundation.layout.Spacer(modifier = Modifier.height(16.dp)) - androidx.compose.material3.Text( + Spacer(modifier = Modifier.height(16.dp)) + Text( text = stringResource(R.string.game_paused).uppercase(), - style = androidx.compose.material3.MaterialTheme.typography.displayMedium, - color = androidx.compose.ui.graphics.Color.White, - fontWeight = androidx.compose.ui.text.font.FontWeight.Bold + style = MaterialTheme.typography.displayMedium, + color = ComposeColor.White, + fontWeight = FontWeight.Bold ) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt` around lines 1144 - 1171, The pause overlay block using isGamePaused should be cleaned up to remove unnecessary fully-qualified names: add an import alias like "import androidx.compose.ui.graphics.Color as ComposeColor" at the top, replace androidx.compose.ui.graphics.Color with ComposeColor, and shorten usages of Box, Column, Spacer, Icon, Text, MaterialTheme, painterResource and FontWeight to their simple names (they are already imported) so the pause overlay code uses concise identifiers while preserving the existing behavior and tint/color values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt`:
- Line 329: No change required: keeping var isGamePaused by remember {
mutableStateOf(PluviaApp.isManuallyPaused) } is acceptable because
PluviaApp.isManuallyPaused is a static field and config changes are already
handled; if you do want state to survive process death, replace remember with
rememberSaveable for isGamePaused, otherwise leave the current usage as-is.
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt`:
- Around line 1144-1171: The pause overlay block using isGamePaused should be
cleaned up to remove unnecessary fully-qualified names: add an import alias like
"import androidx.compose.ui.graphics.Color as ComposeColor" at the top, replace
androidx.compose.ui.graphics.Color with ComposeColor, and shorten usages of Box,
Column, Spacer, Icon, Text, MaterialTheme, painterResource and FontWeight to
their simple names (they are already imported) so the pause overlay code uses
concise identifiers while preserving the existing behavior and tint/color
values.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
app/src/main/java/com/winlator/xconnector/XConnectorEpoll.java (1)
214-223: Thread-safety:connectedClientsis accessed without synchronization.
connectedClientsis mutated on the epoll thread (via JNI callbacks likehandleNewConnection/killConnection), while these new public accessors will likely be called from the UI thread (for the pause/resume feature).SparseArrayis not thread-safe, and there's a TOCTOU gap ingetConnectedClientAtbetween the bounds check on line 219 andvalueAton line 220 — a concurrent removal could shift indices.This mirrors the existing unsynchronized access pattern in the class, so it's not a regression, but since these methods widen the surface to external callers, consider guarding access with a
synchronizedblock or iterating over a snapshot.♻️ Suggested synchronized accessors
- public int getConnectedClientsCount() { - return this.connectedClients.size(); - } - - public Client getConnectedClientAt(int index) { - if (index >= 0 && index < this.connectedClients.size()) { - return this.connectedClients.valueAt(index); - } - return null; - } + public synchronized int getConnectedClientsCount() { + return this.connectedClients.size(); + } + + public synchronized Client getConnectedClientAt(int index) { + if (index >= 0 && index < this.connectedClients.size()) { + return this.connectedClients.valueAt(index); + } + return null; + }Note: this only helps if the mutation sites (
handleNewConnection,killConnection) are also synchronized on the same monitor, which they currently are not. A broader thread-safety pass onconnectedClientsaccess would be the complete fix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/winlator/xconnector/XConnectorEpoll.java` around lines 214 - 223, getConnectedClientsCount() and getConnectedClientAt(int) access the shared SparseArray connectedClients without synchronization, causing TOCTOU and race issues with mutations from JNI callbacks like handleNewConnection and killConnection; fix by guarding reads with the same monitor used for mutations (wrap body of getConnectedClientsCount and getConnectedClientAt in synchronized(this) or a dedicated lock) or by returning/iterating over a shallow snapshot (clone or copy of connectedClients) to avoid index shifts—ensure you also apply the same synchronization to mutation sites (handleNewConnection/killConnection) so all access uses the same lock.app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt (2)
1144-1171: Use short imports instead of fully-qualified names for already-imported composables.
Box,Column,Spacer,Icon,Text,MaterialTheme,Color,painterResource, andFontWeightare all already imported (lines 14, 24, 26, 28, 50-51, etc.), but lines 1146-1167 use fully-qualified references likeandroidx.compose.foundation.layout.Box,androidx.compose.ui.graphics.Color, etc. This hurts readability.♻️ Suggested cleanup
- if (isGamePaused) { - androidx.compose.foundation.layout.Box( - modifier = Modifier - .fillMaxSize() - .background(androidx.compose.ui.graphics.Color.Black.copy(alpha = 0.5f)), - contentAlignment = Alignment.Center - ) { - androidx.compose.foundation.layout.Column( - horizontalAlignment = Alignment.CenterHorizontally, - verticalArrangement = Arrangement.Center - ) { - androidx.compose.material3.Icon( - painter = androidx.compose.ui.res.painterResource(R.drawable.icon_pause), - contentDescription = stringResource(R.string.game_paused), - modifier = Modifier.size(128.dp), - tint = androidx.compose.ui.graphics.Color.White - ) - androidx.compose.foundation.layout.Spacer(modifier = Modifier.height(16.dp)) - androidx.compose.material3.Text( - text = stringResource(R.string.game_paused).uppercase(), - style = androidx.compose.material3.MaterialTheme.typography.displayMedium, - color = androidx.compose.ui.graphics.Color.White, - fontWeight = androidx.compose.ui.text.font.FontWeight.Bold - ) - } - } - } + if (isGamePaused) { + Box( + modifier = Modifier + .fillMaxSize() + .background(androidx.compose.ui.graphics.Color.Black.copy(alpha = 0.5f)), + contentAlignment = Alignment.Center + ) { + Column( + horizontalAlignment = Alignment.CenterHorizontally, + verticalArrangement = Arrangement.Center + ) { + Icon( + painter = painterResource(R.drawable.icon_pause), + contentDescription = stringResource(R.string.game_paused), + modifier = Modifier.size(128.dp), + tint = androidx.compose.ui.graphics.Color.White + ) + Spacer(modifier = Modifier.height(16.dp)) + Text( + text = stringResource(R.string.game_paused).uppercase(), + style = MaterialTheme.typography.displayMedium, + color = androidx.compose.ui.graphics.Color.White, + fontWeight = FontWeight.Bold + ) + } + } + }Note: For
Color.Black/Color.White, you'd need to addimport androidx.compose.ui.graphics.Color(or alias it) to avoid shadowing withandroid.graphics.Coloralready imported on line 5. Alternatively, keep the FQN only forColor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt` around lines 1144 - 1171, Replace the fully-qualified Compose references in the pause overlay with the short imported names (use Box, Column, Spacer, Icon, Text, MaterialTheme, painterResource, and FontWeight) so the composable code is more readable; also add an import for androidx.compose.ui.graphics.Color (or alias it) to avoid shadowing with android.graphics.Color, then update Color.Black/Color.White usages accordingly while keeping painterResource(...) and other existing identifiers unchanged.
2295-2300: Exit cleanup: unconditionalonResume()beforestopEnvironmentComponents()is safe but could be guarded.Line 2296 calls
environment?.onResume()regardless of whether the game was actually paused. While this is likely harmless (resuming already-running processes should be a no-op), adding a guard makes the intent explicit and avoids sending unnecessary SIGCONT signals to processes.♻️ Optional: guard the resume call
winHandler?.stop() // Resume any paused processes before stopping to ensure clean shutdown - environment?.onResume() + if (PluviaApp.isManuallyPaused) { + environment?.onResume() + } environment?.stopEnvironmentComponents() SteamService.keepAlive = false // Clear manual pause state on exit PluviaApp.isManuallyPaused = false🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt` around lines 2295 - 2300, The unconditional call to environment?.onResume() should be guarded so we only resume if the environment is actually paused; update the exit cleanup to check the environment's paused state (e.g., environment?.isPaused or a similar property/method on the Environment class) before calling environment?.onResume(), then proceed to environment?.stopEnvironmentComponents() and the existing SteamService/PluviaApp cleanup; this makes intent explicit and avoids sending SIGCONT when not needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/src/main/java/com/winlator/xenvironment/components/ALSAServerComponent.java`:
- Around line 47-71: The iteration over XConnectorEpoll in
ALSAServerComponent.pause() and resume() can miss clients if the connected
client list changes during the loop; fix both methods by first snapshotting the
current clients into a local List<ALSAClient> (call getConnectedClientsCount()
once, loop i=0..count-1 calling getConnectedClientAt(i) and collecting non-null
client.getTag() instances that are ALSAClient), then iterate that local list to
call ALSAClient.pause() or ALSAClient.start(); also add a short clarifying
comment in resume() that ALSAClient.start() is intentionally the counterpart to
ALSAServerComponent.resume() (or consider renaming resume() if you prefer naming
symmetry) so future maintainers understand the mapping.
---
Duplicate comments:
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt`:
- Line 329: The initialization of the composable state variable isGamePaused
using remember { mutableStateOf(PluviaApp.isManuallyPaused) } is correct and
keeps the UI in sync with the global flag across activity recreation; no code
changes required—leave the isGamePaused declaration as-is in XServerScreen
(remember/mutableStateOf with PluviaApp.isManuallyPaused).
- Around line 1158-1164: The same stringResource(R.string.game_paused) is being
called twice for contentDescription and Text; cache it once in a local val
(e.g., val paused = stringResource(R.string.game_paused)) in XServerScreen so
both contentDescription and the Text use that value, and when uppercasing call
paused.uppercase(Locale.current) to respect locale; update the references for
the Image contentDescription and the androidx.compose.material3.Text to use the
cached val.
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt`:
- Around line 1144-1171: Replace the fully-qualified Compose references in the
pause overlay with the short imported names (use Box, Column, Spacer, Icon,
Text, MaterialTheme, painterResource, and FontWeight) so the composable code is
more readable; also add an import for androidx.compose.ui.graphics.Color (or
alias it) to avoid shadowing with android.graphics.Color, then update
Color.Black/Color.White usages accordingly while keeping painterResource(...)
and other existing identifiers unchanged.
- Around line 2295-2300: The unconditional call to environment?.onResume()
should be guarded so we only resume if the environment is actually paused;
update the exit cleanup to check the environment's paused state (e.g.,
environment?.isPaused or a similar property/method on the Environment class)
before calling environment?.onResume(), then proceed to
environment?.stopEnvironmentComponents() and the existing SteamService/PluviaApp
cleanup; this makes intent explicit and avoids sending SIGCONT when not needed.
In `@app/src/main/java/com/winlator/xconnector/XConnectorEpoll.java`:
- Around line 214-223: getConnectedClientsCount() and getConnectedClientAt(int)
access the shared SparseArray connectedClients without synchronization, causing
TOCTOU and race issues with mutations from JNI callbacks like
handleNewConnection and killConnection; fix by guarding reads with the same
monitor used for mutations (wrap body of getConnectedClientsCount and
getConnectedClientAt in synchronized(this) or a dedicated lock) or by
returning/iterating over a shallow snapshot (clone or copy of connectedClients)
to avoid index shifts—ensure you also apply the same synchronization to mutation
sites (handleNewConnection/killConnection) so all access uses the same lock.
Comment on lines
+47
to
+71
| public void pause() { | ||
| XConnectorEpoll xConnectorEpoll = this.connector; | ||
| if (xConnectorEpoll != null) { | ||
| // Pause all connected ALSA clients | ||
| for (int i = 0; i < xConnectorEpoll.getConnectedClientsCount(); i++) { | ||
| com.winlator.xconnector.Client client = xConnectorEpoll.getConnectedClientAt(i); | ||
| if (client != null && client.getTag() instanceof ALSAClient) { | ||
| ((ALSAClient) client.getTag()).pause(); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| public void resume() { | ||
| XConnectorEpoll xConnectorEpoll = this.connector; | ||
| if (xConnectorEpoll != null) { | ||
| // Resume all connected ALSA clients | ||
| for (int i = 0; i < xConnectorEpoll.getConnectedClientsCount(); i++) { | ||
| com.winlator.xconnector.Client client = xConnectorEpoll.getConnectedClientAt(i); | ||
| if (client != null && client.getTag() instanceof ALSAClient) { | ||
| ((ALSAClient) client.getTag()).start(); | ||
| } | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find ALSAClient class and check for pause/resume/start methods
fd -t f -e java -i alsa | head -20Repository: utkarshdalal/GameNative
Length of output: 345
🏁 Script executed:
# Search for ALSAClient definition and its methods
rg -n "class ALSAClient" --type javaRepository: utkarshdalal/GameNative
Length of output: 302
🏁 Script executed:
# Check ALSAClient's pause, resume, start methods
rg -n "(void pause|void resume|void start)\(" --type java --glob "*ALSAClient*"Repository: utkarshdalal/GameNative
Length of output: 242
🏁 Script executed:
# Find XConnectorEpoll and check getConnectedClientAt implementation
rg -n "getConnectedClientAt|getConnectedClientsCount" --type java -A 5 -B 2Repository: utkarshdalal/GameNative
Length of output: 3582
Concurrent-modification risk during client iteration and asymmetric method naming.
Both pause() and resume() iterate over connected clients by index without synchronization. If a client connects or disconnects concurrently (changing the count between the loop condition check and getConnectedClientAt(i)), entries could be skipped or the loop could terminate prematurely, even though getConnectedClientAt() safely returns null rather than throwing an exception. The null check on lines 53 and 66 prevents crashes but does not prevent incomplete iteration.
Additionally, resume() calls ALSAClient.start() while pause() calls ALSAClient.pause() — note that ALSAClient has no resume() method, only start(). While start() is the correct counterpart, this naming asymmetry between the component's resume() method and the client's start() method could confuse future maintainers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/main/java/com/winlator/xenvironment/components/ALSAServerComponent.java`
around lines 47 - 71, The iteration over XConnectorEpoll in
ALSAServerComponent.pause() and resume() can miss clients if the connected
client list changes during the loop; fix both methods by first snapshotting the
current clients into a local List<ALSAClient> (call getConnectedClientsCount()
once, loop i=0..count-1 calling getConnectedClientAt(i) and collecting non-null
client.getTag() instances that are ALSAClient), then iterate that local list to
call ALSAClient.pause() or ALSAClient.start(); also add a short clarifying
comment in resume() that ALSAClient.start() is intentionally the counterpart to
ALSAServerComponent.resume() (or consider renaming resume() if you prefer naming
symmetry) so future maintainers understand the mapping.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
in back button menu, add a pause/play button that suspends wine procs similar to how sleeping android does, but this can be user controlled.
Inspired by this decky plugin I love for Steam Deck:
https://github.com/popsUlfr/SDH-PauseGames
A few use cases I find this nice for:
Summary by cubic
Add a manual Pause/Resume control in the back button menu to suspend and resume the game on demand. Manual pause persists across background/foreground, pauses audio, and shows a localized pause overlay.
Written for commit ec807ee. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes