Skip to content
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
# Implementation Plan - Fix Crashes, ANRs, and UI Stability in Library List View

This plan aims to implement the changes and feedback from [PR #1758](https://github.com/utkarshdalal/GameNative/pull/1758) to resolve critical application crashes and ANRs, especially when games are stored on external storage.

## User Review Required

> [!IMPORTANT]
> The `ContainerManager` will be converted to a strict singleton. This involves making its constructor private and updating all instantiation sites to use `getInstance(Context)`.

## Proposed Changes

### Core Infrastructure

#### [MODIFY] [ContainerManager.java](file:///E:/workspace/StudioProjects/GameNative/app/src/main/java/com/winlator/container/ContainerManager.java)
Comment thread
Meloon33 marked this conversation as resolved.
Outdated
- Make the constructor `private`.
- Make `getInstance(Context)` thread-safe using a synchronized block.

#### [MODIFY] [AdrenotoolsManager.java](file:///E:/workspace/StudioProjects/GameNative/app/src/main/java/com/winlator/contents/AdrenotoolsManager.java)
- Replace `new ContainerManager(context)` with `ContainerManager.getInstance(context)`.

#### [MODIFY] [ImageFsInstaller.java](file:///E:/workspace/StudioProjects/GameNative/app/src/main/java/com/winlator/xenvironment/ImageFsInstaller.java)
- Replace `new ContainerManager(context)` with `ContainerManager.getInstance(context)`.

#### [MODIFY] [PluviaMain.kt](file:///E:/workspace/StudioProjects/GameNative/app/src/main/java/app/gamenative/ui/PluviaMain.kt)
- Replace `ContainerManager(context)` with `ContainerManager.getInstance(context)`.

#### [MODIFY] [XServerScreen.kt](file:///E:/workspace/StudioProjects/GameNative/app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt)
- Replace `ContainerManager(context)` with `ContainerManager.getInstance(context)`.

#### [MODIFY] [ContainerUtils.kt](file:///E:/workspace/StudioProjects/GameNative/app/src/main/java/app/gamenative/utils/ContainerUtils.kt)
- Replace all `ContainerManager(context)` calls with `ContainerManager.getInstance(context)`.
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

---

### Library UI Components

#### [MODIFY] [LibraryListCard.kt](file:///E:/workspace/StudioProjects/GameNative/app/src/main/java/app/gamenative/ui/screen/library/components/LibraryListCard.kt)
- Add `imageRefreshCounter: Long` parameter to `ListViewCard`.
- Add `imageRefreshCounter` to the `produceState` keys for icon loading. This ensures icons are re-fetched if the refresh counter changes (e.g., when external storage becomes ready).

#### [MODIFY] [LibraryAppItem.kt](file:///E:/workspace/StudioProjects/GameNative/app/src/main/java/app/gamenative/ui/screen/library/components/LibraryAppItem.kt)
- Pass the `imageRefreshCounter` from `AppItem` to `ListViewCard`.

#### [MODIFY] [LibraryListPane.kt](file:///E:/workspace/StudioProjects/GameNative/app/src/main/java/app/gamenative/ui/screen/library/components/LibraryListPane.kt)
- Move the `HorizontalDivider` logic out of the animated `Box` and `Column` to prevent it from being part of the item cell's animated alpha and touch area.
- Position the divider above the item's animated `Box`.

---

### Verification Plan

### Automated Tests
- Run a build to ensure all `ContainerManager` references are correctly updated and the private constructor doesn't break anything.
- `gradlew :app:assembleDebug`

### Manual Verification
- Verify that the Library screen loads correctly.
- Verify that switching between List and Grid layouts works as expected.
- Verify that icons load correctly in List view.
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
- `[/]` Step 1: Core Infrastructure (`ContainerManager` Singleton)
Comment thread
cubic-dev-ai[bot] marked this conversation as resolved.
Outdated
- `[ ]` Modify `ContainerManager.java` (private constructor, synchronized `getInstance`)
- `[ ]` Update `AdrenotoolsManager.java`
- `[ ]` Update `ImageFsInstaller.java`
- `[ ]` Update `PluviaMain.kt`
- `[ ]` Update `XServerScreen.kt`
- `[ ]` Update `ContainerUtils.kt`
- `[ ]` Update `EpicAppScreen.kt` (clean up import if needed)
- `[ ]` Step 2: Asynchronous Icon loading in `LibraryListCard`
- `[ ]` Step 3: UI Layout Fix in `LibraryListPane`
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
# Walkthrough - Core Infrastructure (Step 1)
Comment thread
cubic-dev-ai[bot] marked this conversation as resolved.
Outdated

I have completed the refactoring of `ContainerManager` to a singleton pattern. This ensures that only one instance of the manager exists, preventing redundant disk scans of the internal home directory and improving thread safety across the application.

## Changes Made

### Core Infrastructure

#### [ContainerManager.java](file:///E:/workspace/StudioProjects/GameNative/app/src/main/java/com/winlator/container/ContainerManager.java)
- Converted `ContainerManager` to a strict singleton.
- Made the constructor `private`.
- Added a synchronized `getInstance(Context)` method to ensure thread safety during initialization.

### Singleton Migration
Updated the following classes to use `ContainerManager.getInstance(context)` instead of creating new instances:
- [AdrenotoolsManager.java](file:///E:/workspace/StudioProjects/GameNative/app/src/main/java/com/winlator/contents/AdrenotoolsManager.java)
- [ImageFsInstaller.java](file:///E:/workspace/StudioProjects/GameNative/app/src/main/java/com/winlator/xenvironment/ImageFsInstaller.java)
- [PluviaMain.kt](file:///E:/workspace/StudioProjects/GameNative/app/src/main/java/app/gamenative/ui/PluviaMain.kt)
- [XServerScreen.kt](file:///E:/workspace/StudioProjects/GameNative/app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt)
- [ContainerUtils.kt](file:///E:/workspace/StudioProjects/GameNative/app/src/main/java/app/gamenative/utils/ContainerUtils.kt)
- [EpicAppScreen.kt](file:///E:/workspace/StudioProjects/GameNative/app/src/main/java/app/gamenative/ui/screen/library/appscreen/EpicAppScreen.kt) (removed redundant import)

## Verification Results

### Automated Tests
- Verified that all compilation errors related to the `private` constructor were resolved by updating all instantiation sites.

> [!NOTE]
> I have committed these changes locally to the branch `fix/list-layout-external-storage-crash`. However, I do not have permissions to push directly to the remote repository `utkarshdalal/GameNative`. Please push the changes to GitHub to make them available for review in the PR.
11 changes: 3 additions & 8 deletions app/src/main/java/app/gamenative/data/LibraryItem.kt
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package app.gamenative.data

import app.gamenative.Constants
import app.gamenative.utils.CustomGameScanner

enum class GameSource {
STEAM,
Expand Down Expand Up @@ -55,13 +54,9 @@ data class LibraryItem(
""
}
GameSource.CUSTOM_GAME -> {
// Attempt to resolve a local icon from the selected/unique exe folder
val localPath = CustomGameScanner.findIconFileForCustomGame(appId)
if (!localPath.isNullOrEmpty()) {
if (localPath.startsWith("file://")) localPath else "file://$localPath"
} else {
""
}
// Return empty; icons are fetched asynchronously in UI components
// to avoid blocking the main thread with filesystem scans.
""
}
GameSource.GOG -> {
// GoG Images are typically the full URL, but have fallback just in case.
Expand Down
2 changes: 1 addition & 1 deletion app/src/main/java/app/gamenative/service/SteamService.kt
Original file line number Diff line number Diff line change
Expand Up @@ -1410,7 +1410,7 @@ class SteamService : Service(), IChallengeUrlChanged {
fun downloadApp(appId: Int, dlcAppIds: List<Int>, branch: String = "public", isUpdateOrVerify: Boolean): DownloadInfo? {
if (!checkWifiOrNotify()) return null
return getAppInfoOf(appId)?.let { appInfo ->
val container = ContainerManager(instance!!.applicationContext).getContainerById("STEAM_${appId}")
val container = ContainerManager.getInstance(instance!!.applicationContext).getContainerById("STEAM_${appId}")
val containerLanguage = if (container != null) {
container.language
} else {
Expand Down
2 changes: 1 addition & 1 deletion app/src/main/java/app/gamenative/ui/PluviaMain.kt
Original file line number Diff line number Diff line change
Expand Up @@ -1613,7 +1613,7 @@ fun preLaunchApp(

// create container if it does not already exist
// TODO: combine somehow with container creation in HomeLibraryAppScreen
val containerManager = ContainerManager(context)
val containerManager = ContainerManager.getInstance(context)
val container = if (useTemporaryOverride) {
ContainerUtils.getOrCreateContainerWithOverride(context, appId)
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@ class CustomGameAppScreen : BaseAppScreen() {

if (shouldExtract) {
// First, try using the container's selected executable if available
val containerManager = com.winlator.container.ContainerManager(context)
val containerManager = com.winlator.container.ContainerManager.getInstance(context)
val hasContainer = containerManager.hasContainer(libraryItem.appId)
Timber.tag("CustomGameAppScreen").d("Container exists: $hasContainer")

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ import app.gamenative.utils.ContainerUtils
import app.gamenative.utils.ContainerUtils.extractGameIdFromContainerId
import app.gamenative.utils.MarkerUtils
import com.winlator.container.ContainerData
import com.winlator.container.ContainerManager
import com.winlator.core.StringUtils
import java.io.File
import java.util.Locale
Expand All @@ -48,13 +47,22 @@ import app.gamenative.ui.util.SnackbarManager
import timber.log.Timber

// TODO: Verify all tests and do DLC auto-install with base game.
/**
* Implementation of [BaseAppScreen] for Epic Games.
* Handles game information display, installation, and management for the Epic Games Store.
*/
class EpicAppScreen : BaseAppScreen() {

companion object {
private const val TAG = "EpicAppScreen"

private val uninstallDialogAppIds = mutableStateListOf<String>()

/**
* Triggers the uninstall confirmation dialog for a specific app.
*
* @param appId The ID of the app to uninstall.
*/
fun showUninstallDialog(appId: String) {
Timber.tag(TAG).d("showUninstallDialog: appId=$appId")
if (!uninstallDialogAppIds.contains(appId)) {
Expand All @@ -63,11 +71,22 @@ class EpicAppScreen : BaseAppScreen() {
}
}

/**
* Hides the uninstall confirmation dialog for a specific app.
*
* @param appId The ID of the app.
*/
fun hideUninstallDialog(appId: String) {
Timber.tag(TAG).d("hideUninstallDialog: appId=$appId")
uninstallDialogAppIds.remove(appId)
}

/**
* Checks if the uninstall confirmation dialog should be shown for an app.
*
* @param appId The ID of the app.
* @return true if the dialog should be shown.
*/
fun shouldShowUninstallDialog(appId: String): Boolean {
val result = uninstallDialogAppIds.contains(appId)
Timber.tag(TAG).d("shouldShowUninstallDialog: appId=$appId, result=$result")
Expand All @@ -80,6 +99,11 @@ class EpicAppScreen : BaseAppScreen() {
// Shared state for install dialog - list of appIds that should show the dialog
private val installDialogAppIds = mutableStateListOf<String>()

/**
* Triggers the install dialog for a specific app.
*
* @param appId The ID of the app to install.
*/
fun showInstallDialog(appId: String) {
Timber.tag(TAG).d("showInstallDialog: appId=$appId")
if (!installDialogAppIds.contains(appId)) {
Expand All @@ -88,11 +112,22 @@ class EpicAppScreen : BaseAppScreen() {
}
}

/**
* Hides the install dialog for a specific app.
*
* @param appId The ID of the app.
*/
fun hideInstallDialog(appId: String) {
Timber.tag(TAG).d("hideInstallDialog: appId=$appId")
installDialogAppIds.remove(appId)
}

/**
* Checks if the install dialog should be shown for an app.
*
* @param appId The ID of the app.
* @return true if the dialog should be shown.
*/
fun shouldShowInstallDialog(appId: String): Boolean {
val result = installDialogAppIds.contains(appId)
Timber.tag(TAG).d("shouldShowInstallDialog: appId=$appId, result=$result")
Expand All @@ -102,21 +137,39 @@ class EpicAppScreen : BaseAppScreen() {
// Shared state for game manager dialog - map of gameId to GameManagerDialogState
private val gameManagerDialogStates = mutableStateMapOf<Int, app.gamenative.ui.component.dialog.state.GameManagerDialogState>()

/**
* Triggers the game manager dialog (e.g., for DLC selection) for a specific game.
*
* @param gameId The ID of the game.
* @param state The state of the dialog.
*/
fun showGameManagerDialog(gameId: Int, state: app.gamenative.ui.component.dialog.state.GameManagerDialogState) {
Timber.tag(TAG).d("showGameManagerDialog: gameId=$gameId")
gameManagerDialogStates[gameId] = state
}

/**
* Hides the game manager dialog for a specific game.
*
* @param gameId The ID of the game.
*/
fun hideGameManagerDialog(gameId: Int) {
Timber.tag(TAG).d("hideGameManagerDialog: gameId=$gameId")
gameManagerDialogStates.remove(gameId)
}

/**
* Retrieves the state of the game manager dialog for a specific game.
*
* @param gameId The ID of the game.
* @return The dialog state, or null if not found.
*/
fun getGameManagerDialogState(gameId: Int): app.gamenative.ui.component.dialog.state.GameManagerDialogState? {
return gameManagerDialogStates[gameId]
}
}


@Composable
override fun getGameDisplayInfo(
context: Context,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1015,7 +1015,7 @@ class SteamAppScreen : BaseAppScreen() {
}
try {
val info = withContext(Dispatchers.IO) {
val container = ContainerManager(context).getContainerById("STEAM_$gameId")
val container = ContainerManager.getInstance(context).getContainerById("STEAM_$gameId")
val language = container?.language ?: PrefManager.containerLanguage
val depots = SteamService.getDownloadableDepots(gameId, language)
Timber.i("There are ${depots.size} depots belonging to ${libraryItem.appId}")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ internal fun AppItem(
isFocused = isFocused,
onFocusChanged = { isFocused = it },
isRefreshing = isRefreshing,
imageRefreshCounter = imageRefreshCounter,
compatibilityStatus = compatibilityStatus,
gameStats = gameStats,
context = context,
Expand Down Expand Up @@ -152,8 +153,17 @@ internal fun AppItem(
}
}

/**
* Composable that displays an icon representing the source of a game (e.g., Steam, Epic).
*
* @param gameSource The source of the game.
* @param modifier The modifier to be applied to the layout.
* @param iconSize The size of the icon in dp.
* @param alignmentBoxSize The size of the containing box in dp, used for alignment.
*/
@Composable
fun GameSourceIcon(

gameSource: GameSource,
modifier: Modifier = Modifier,
iconSize: Int = 12,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,16 +55,30 @@ import kotlinx.coroutines.withContext

/**
* List view card with compact layout.
*
* @param modifier The modifier to be applied to the layout.
* @param appInfo Information about the library item (game/app).
* @param onClick Callback when the card is clicked.
* @param onFocus Callback when the card receives focus.
* @param isFocused Whether the card is currently focused.
* @param onFocusChanged Callback when focus state changes.
* @param isRefreshing Whether the library is currently refreshing.
* @param imageRefreshCounter Counter to trigger icon reloads.
* @param compatibilityStatus The compatibility status of the game.
* @param gameStats Statistics for the game (e.g., play time).
* @param context The Android context.
*/
@Composable
internal fun ListViewCard(

modifier: Modifier,
appInfo: LibraryItem,
onClick: () -> Unit,
onFocus: () -> Unit,
isFocused: Boolean,
onFocusChanged: (Boolean) -> Unit,
isRefreshing: Boolean,
imageRefreshCounter: Long,
compatibilityStatus: GameCompatibilityStatus?,
gameStats: GameCardStats?,
context: Context,
Expand Down Expand Up @@ -115,11 +129,12 @@ internal fun ListViewCard(
verticalAlignment = Alignment.CenterVertically,
horizontalArrangement = Arrangement.spacedBy(12.dp),
) {
// Game icon
// Game icon - start with empty to avoid synchronous LibraryItem getter
val iconUrl by produceState(
initialValue = appInfo.clientIconUrl,
initialValue = "",
Comment thread
cubic-dev-ai[bot] marked this conversation as resolved.
key1 = appInfo.appId,
key2 = appInfo.clientIconUrl,
key3 = imageRefreshCounter,
) {
Comment thread
coderabbitai[bot] marked this conversation as resolved.
value = withContext(Dispatchers.IO) {
getListIconUrl(context, appInfo)
Expand Down Expand Up @@ -220,13 +235,7 @@ private fun InstallStatusBadge(
}
val isDownloading = downloadInfo != null && downloadProgress < 1f
var isInstalled by remember(appInfo.appId) {
mutableStateOf(
if (isSteam) {
SteamService.isAppInstalled(appInfo.gameId)
} else {
true // Custom Games always installed
},
)
mutableStateOf(appInfo.isInstalled)
}

LaunchedEffect(isRefreshing) {
Expand Down
Loading