New wrapper 2026 #459
Merged
New wrapper 2026 #459
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
📝 WalkthroughWalkthroughAdds nine Compose dialog tabs backed by a new ContainerConfigState, JNI Vulkan vendorID enumeration and Java GPU JSON lookup utilities, Adreno/Turnip-specific default branch and resource/version updates, BCN "compute" mode and runtime env/config changes, plus small installer and env-var additions. Changes
Sequence Diagram(s)sequenceDiagram
participant Java as Java/GPUInformation
participant JNI as JNI (vulkan.c)
participant Loader as Vulkan Loader
participant Driver as Vulkan Driver
participant Device as Physical Device
Java->>JNI: Java_com_winlator_core_GPUInformation_getVendorID(driverName, context)
JNI->>Loader: dlopen / resolve vkGetInstanceProcAddr -> vkCreateInstance
Loader-->>JNI: VkInstance
JNI->>Driver: vkEnumeratePhysicalDevices(instance)
Driver-->>JNI: VkPhysicalDevice handles
JNI->>Device: vkGetPhysicalDeviceProperties(device)
Device-->>JNI: VkPhysicalDeviceProperties (includes vendorID)
JNI->>JNI: extract vendorID, vkDestroyInstance, dlclose
JNI-->>Java: return vendorID (int)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
Owner
Author
|
@coderabbitai is any functionality lost in the split of ContainerConfigDialog? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 16895 🏁 Script executed: Length of output: 8201 🏁 Script executed: Length of output: 72936 🏁 Script executed: Length of output: 28683
Evidence of Complete PreservationThe refactor shows 11 files changed with 1,943 insertions and 1,543 deletions (net +400 lines), suggesting reorganization rather than removal of features. All tab composables exist and are called:
Comprehensive test coverage confirms all functionality works:
Improvements made during refactor:
The new |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 532 🏁 Script executed: Length of output: 21664 🏁 Script executed: Length of output: 894 🏁 Script executed: Length of output: 18212 🏁 Script executed: Length of output: 28423 🏁 Script executed: Length of output: 1397 🏁 Script executed: Length of output: 1698
Summary of ChangesThe refactoring split the original 2,597-line monolithic dialog into:
Evidence That Functionality Was Preserved
The refactoring successfully improved code organization and stability without sacrificing any features. The increase in total lines and component count suggests the split may have even uncovered opportunities to surface additional settings that were previously difficult to manage in the monolithic structure. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In `@app/src/main/cpp/extras/vulkan.c`:
- Around line 274-298: The JNI function
Java_com_winlator_core_GPUInformation_getVendorID currently returns early on
create_instance and enumerate_physical_devices failures and skips cleanup;
refactor it to follow the file's established pattern by replacing the early
returns with jumps to a single cleanup label (e.g., "cleanup") so that
destroyInstance(instance, NULL) is always called and vulkan_handle is dlclosed
(and globals reset) on both error and success paths; ensure create_instance,
enumerate_physical_devices, getPhysicalDeviceProperties, and the final return
all flow through that cleanup section.
In `@app/src/main/java/app/gamenative/ui/component/dialog/DrivesTab.kt`:
- Around line 54-60: The current loop in DrivesTab rebuilds config.drives by
concatenating entries without any separator
(drivesBuilder.append("${existingDrive[0]}:${existingDrive[1]}")), which loses
delimiters expected by Container.drivesIterator and breaks parsing after
deletion; update the rebuild to use the same serializer/delimiter as the parser
by either calling the canonical serializer used elsewhere (preferred) or
inserting the correct delimiter between entries (e.g., append delimiter before
or after each entry using the parser's delimiter constant) when constructing the
string passed into config.copy(drives = ...); reference
Container.drivesIterator, drivesBuilder, existingDrive, driveLetter, and
state.config.value to locate and adjust the logic.
In `@app/src/main/java/app/gamenative/ui/component/dialog/WineTab.kt`:
- Around line 19-30: The renderer selection currently does two separate
state.config.value = config.copy(...) calls that both read the original config
snapshot so the second overwrites the first and loses the updated
videoPciDeviceID; update the onItemSelected handler in SettingsListDropdown so
you build the updated graphicsDriverConfig (use
com.winlator.core.KeyValueSet(config.graphicsDriverConfig), modify it) and then
perform a single state.config.value = config.copy(videoPciDeviceID =
gpuCardsValues[it].deviceId, graphicsDriverConfig = updatedCfg.toString()),
while still updating state.gpuNameIndex.value = it.
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt`:
- Around line 3280-3285: The code currently sets WRAPPER_DEVICE_NAME/ID/VENDOR
when gpuName != "Device" but gpuName can be an empty string; change the guard to
require a non-blank gpuName (e.g. check gpuName.isNullOrBlank()/isBlank() or
trim().isNotEmpty()) before calling envVars.put and before invoking
GPUInformation.getDeviceIdFromGPUName and getVendorIdFromGPUName with context so
you don't override autodetection with blank values.
- Around line 2987-2990: The loop currently does "if
(!container.wineVersion.contains("arm64ec") && identifier.contains("opengl") &&
useNative) continue" which skips processing and leaves stale native overrides;
instead detect the condition and set useNative = false so the method continues
and restores built‑ins/registry entries. Update the logic around useNative (the
variable derived from wincomponent[1]) so when container.wineVersion does not
contain "arm64ec" but identifier.contains("opengl"), assign useNative = false
(do not continue) before the rest of the override/registry update code that uses
useNative and identifier.
- Around line 3307-3322: Cache the vendor id from
GPUInformation.getVendorID(...) into a local val (e.g., vendorId) once and
change the conditional in both bcnEmulation "auto" and "full" branches to
require vendorId != 0 && vendorId != 0x5143 before setting ENABLE_BCN_COMPUTE
and BCN_COMPUTE_AUTO/BCN_COMPUTE_AUTO; reference the existing bcnEmulationType,
bcnEmulation and envVars variables and replace the two repeated
GPUInformation.getVendorID(...) calls with the single cached vendorId check.
In `@app/src/main/java/com/winlator/core/envvars/EnvVarInfo.kt`:
- Around line 241-245: The map contains a duplicate key "WINE_NEW_MEDIASOURCE"
that overwrites the earlier WINE_NEW_MEDIASOURCE entry; update the second map
entry so its key matches its identifier ("GALLIUM_HUD") instead of
"WINE_NEW_MEDIASOURCE" where the EnvVarInfo is created (the entry using
identifier = "GALLIUM_HUD" and EnvVarSelectionType.MULTI_SELECT, possibleValues
= listOf("simple", "fps", "frametime")). Ensure the unique map key is
"GALLIUM_HUD" so both toggles are accessible.
In `@app/src/main/java/com/winlator/core/GPUInformation.java`:
- Around line 47-62: getVendorIdFromGPUName currently assumes
FileUtils.readString returns non-null, swallows JSONExceptions, and assigns
vendorId for every match (returning the last); fix by validating gpuNameList is
non-null/empty before parsing, wrap the JSONArray/JSONObject parsing in a
try/catch that logs or handles the JSONException instead of leaving it empty,
and upon finding the first matching jobj.getString("name").contains(gpuName)
return jobj.getString("vendorID") immediately (or set vendorId and break) to
avoid returning the last match; reference getVendorIdFromGPUName and
FileUtils.readString and ensure the catch block records the exception (e.g., via
logging) and the method returns an empty string when input is invalid or on
parse error.
- Around line 30-45: In getDeviceIdFromGPUName, guard against
FileUtils.readString returning null before constructing the JSONArray, avoid
swallowing exceptions, and stop on the first match: first check if gpuNameList
is null/empty and immediately return ""; then wrap new JSONArray(gpuNameList) in
a try/catch that logs the JSONException (e.g., Log.e or your logger) and returns
"" on error; inside the for loop use jobj.getString("name").contains(gpuName)
safely and when a match is found set deviceId and break to return the first
match.
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/ui/component/dialog/GraphicsTab.kt (1)
389-395: Avoid mutating state during composition in the VKD3D branch.
Settingstate.config.valueinside the composable body is a side-effect and can cause recomposition churn. Move it to aLaunchedEffect(keyed on VKD3D + version) or perform this in the state holder.♻️ Suggested refactor
-import androidx.compose.runtime.Composable +import androidx.compose.runtime.Composable +import androidx.compose.runtime.LaunchedEffect} else { // Ensure default version for vortek-like when hidden val driverType = StringUtils.parseIdentifier(state.graphicsDrivers.value.getOrNull(state.graphicsDriverIndex.value).orEmpty()) - val isVortekLike = config.containerVariant.equals(Container.GLIBC) && (driverType == "vortek" || driverType == "adreno" || driverType == "sd-8-elite") + val isVortekLike = config.containerVariant.equals(Container.GLIBC) && + (driverType == "vortek" || driverType == "adreno" || driverType == "sd-8-elite") val version = if (isVortekLike) "1.10.3" else "2.4.1" - val currentConfig = KeyValueSet(config.dxwrapperConfig) - currentConfig.put("version", version) - state.config.value = config.copy(dxwrapperConfig = currentConfig.toString()) + LaunchedEffect(isVortekLike, version) { + val currentConfig = KeyValueSet(state.config.value.dxwrapperConfig) + currentConfig.put("version", version) + state.config.value = state.config.value.copy(dxwrapperConfig = currentConfig.toString()) + } }
Comment on lines
+54
to
+60
| val drivesBuilder = StringBuilder() | ||
| for (existingDrive in Container.drivesIterator(config.drives)) { | ||
| if (existingDrive[0] != driveLetter) { | ||
| drivesBuilder.append("${existingDrive[0]}:${existingDrive[1]}") | ||
| } | ||
| } | ||
| state.config.value = config.copy(drives = drivesBuilder.toString()) |
There was a problem hiding this comment.
Preserve drive delimiters when rebuilding config.drives.
drivesBuilder.append("${existingDrive[0]}:${existingDrive[1]}") concatenates entries without separators. If Container.drivesIterator expects a delimiter, this corrupts the drives string and breaks parsing after deletion. Rebuild using the same delimiter/serializer as the parser.
#!/bin/bash
# Locate drivesIterator implementation and delimiter/format used for serialization.
rg -n "drivesIterator|drive.*separator|DRIVE" -C3🤖 Prompt for AI Agents
In `@app/src/main/java/app/gamenative/ui/component/dialog/DrivesTab.kt` around
lines 54 - 60, The current loop in DrivesTab rebuilds config.drives by
concatenating entries without any separator
(drivesBuilder.append("${existingDrive[0]}:${existingDrive[1]}")), which loses
delimiters expected by Container.drivesIterator and breaks parsing after
deletion; update the rebuild to use the same serializer/delimiter as the parser
by either calling the canonical serializer used elsewhere (preferred) or
inserting the correct delimiter between entries (e.g., append delimiter before
or after each entry using the parser's delimiter constant) when constructing the
string passed into config.copy(drives = ...); reference
Container.drivesIterator, drivesBuilder, existingDrive, driveLetter, and
state.config.value to locate and adjust the logic.
Comment on lines
+3280
to
+3285
| val gpuName = graphicsDriverConfig.get("gpuName") | ||
| if (gpuName != "Device") { | ||
| envVars.put("WRAPPER_DEVICE_NAME", gpuName) | ||
| envVars.put("WRAPPER_DEVICE_ID", GPUInformation.getDeviceIdFromGPUName(context, gpuName)) | ||
| envVars.put("WRAPPER_VENDOR_ID", GPUInformation.getVendorIdFromGPUName(context, gpuName)) | ||
| } |
There was a problem hiding this comment.
Guard GPU override env vars against blank values.
gpuName defaults to an empty string; the current check still sets WRAPPER_* and overrides autodetect. Require non‑blank before applying.
🛠️ Proposed fix
- val gpuName = graphicsDriverConfig.get("gpuName")
- if (gpuName != "Device") {
+ val gpuName = graphicsDriverConfig.get("gpuName")
+ if (gpuName.isNotBlank() && gpuName != "Device") {
envVars.put("WRAPPER_DEVICE_NAME", gpuName)
envVars.put("WRAPPER_DEVICE_ID", GPUInformation.getDeviceIdFromGPUName(context, gpuName))
envVars.put("WRAPPER_VENDOR_ID", GPUInformation.getVendorIdFromGPUName(context, gpuName))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| val gpuName = graphicsDriverConfig.get("gpuName") | |
| if (gpuName != "Device") { | |
| envVars.put("WRAPPER_DEVICE_NAME", gpuName) | |
| envVars.put("WRAPPER_DEVICE_ID", GPUInformation.getDeviceIdFromGPUName(context, gpuName)) | |
| envVars.put("WRAPPER_VENDOR_ID", GPUInformation.getVendorIdFromGPUName(context, gpuName)) | |
| } | |
| val gpuName = graphicsDriverConfig.get("gpuName") | |
| if (gpuName.isNotBlank() && gpuName != "Device") { | |
| envVars.put("WRAPPER_DEVICE_NAME", gpuName) | |
| envVars.put("WRAPPER_DEVICE_ID", GPUInformation.getDeviceIdFromGPUName(context, gpuName)) | |
| envVars.put("WRAPPER_VENDOR_ID", GPUInformation.getVendorIdFromGPUName(context, gpuName)) | |
| } |
🤖 Prompt for AI Agents
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt` around
lines 3280 - 3285, The code currently sets WRAPPER_DEVICE_NAME/ID/VENDOR when
gpuName != "Device" but gpuName can be an empty string; change the guard to
require a non-blank gpuName (e.g. check gpuName.isNullOrBlank()/isBlank() or
trim().isNotEmpty()) before calling envVars.put and before invoking
GPUInformation.getDeviceIdFromGPUName and getVendorIdFromGPUName with context so
you don't override autodetection with blank values.
Comment on lines
+3307
to
+3322
| val bcnEmulationType = graphicsDriverConfig.get("bcnEmulationType") | ||
| when (bcnEmulation) { | ||
| "auto" -> envVars.put("WRAPPER_EMULATE_BCN", "3") | ||
| "full" -> envVars.put("WRAPPER_EMULATE_BCN", "2") | ||
| "auto" -> { | ||
| if (bcnEmulationType.equals("compute") && GPUInformation.getVendorID(null, null) != 0x5143) { | ||
| envVars.put("ENABLE_BCN_COMPUTE", "1"); | ||
| envVars.put("BCN_COMPUTE_AUTO", "1"); | ||
| } | ||
| envVars.put("WRAPPER_EMULATE_BCN", "3"); | ||
| } | ||
| "full" -> { | ||
| if (bcnEmulationType.equals("compute") && GPUInformation.getVendorID(null, null) != 0x5143) { | ||
| envVars.put("ENABLE_BCN_COMPUTE", "1"); | ||
| envVars.put("BCN_COMPUTE_AUTO", "0"); | ||
| } | ||
| envVars.put("WRAPPER_EMULATE_BCN", "2"); | ||
| } |
There was a problem hiding this comment.
Treat vendorId=0 as unknown before enabling BCN compute.
getVendorID returns 0 on failure; current logic enables compute for unknown devices. Cache vendorId once and require it to be non‑zero.
🛠️ Proposed fix
val bcnEmulation = graphicsDriverConfig.get("bcnEmulation")
val bcnEmulationType = graphicsDriverConfig.get("bcnEmulationType")
+ val vendorId = if (bcnEmulationType == "compute") GPUInformation.getVendorID(null, null) else 0
when (bcnEmulation) {
"auto" -> {
- if (bcnEmulationType.equals("compute") && GPUInformation.getVendorID(null, null) != 0x5143) {
+ if (bcnEmulationType == "compute" && vendorId != 0 && vendorId != 0x5143) {
envVars.put("ENABLE_BCN_COMPUTE", "1");
envVars.put("BCN_COMPUTE_AUTO", "1");
}
envVars.put("WRAPPER_EMULATE_BCN", "3");
}
"full" -> {
- if (bcnEmulationType.equals("compute") && GPUInformation.getVendorID(null, null) != 0x5143) {
+ if (bcnEmulationType == "compute" && vendorId != 0 && vendorId != 0x5143) {
envVars.put("ENABLE_BCN_COMPUTE", "1");
envVars.put("BCN_COMPUTE_AUTO", "0");
}
envVars.put("WRAPPER_EMULATE_BCN", "2");
}🤖 Prompt for AI Agents
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt` around
lines 3307 - 3322, Cache the vendor id from GPUInformation.getVendorID(...) into
a local val (e.g., vendorId) once and change the conditional in both
bcnEmulation "auto" and "full" branches to require vendorId != 0 && vendorId !=
0x5143 before setting ENABLE_BCN_COMPUTE and BCN_COMPUTE_AUTO/BCN_COMPUTE_AUTO;
reference the existing bcnEmulationType, bcnEmulation and envVars variables and
replace the two repeated GPUInformation.getVendorID(...) calls with the single
cached vendorId check.
Comment on lines
+30
to
+45
| public static String getDeviceIdFromGPUName(Context context, String gpuName) { | ||
| String gpuNameList = FileUtils.readString(context, "gpu_cards.json"); | ||
| String deviceId = ""; | ||
| try { | ||
| JSONArray jsonArray = new JSONArray(gpuNameList); | ||
| for (int i = 0; i < jsonArray.length(); i++) { | ||
| JSONObject jobj = jsonArray.getJSONObject(i); | ||
| if (jobj.getString("name").contains(gpuName)) { | ||
| deviceId = jobj.getString("deviceID"); | ||
| } | ||
| } | ||
| } | ||
| catch (JSONException e) { | ||
| } | ||
| return deviceId; | ||
| } |
There was a problem hiding this comment.
Silent error handling and potential null dereference.
FileUtils.readString()may returnnullif the asset file cannot be read, which would cause theJSONArrayconstructor to throw.- The empty catch block silently swallows JSON errors, making debugging difficult.
- The loop doesn't
breakon first match, returning the last matching entry instead of the first.
🛡️ Proposed fix
public static String getDeviceIdFromGPUName(Context context, String gpuName) {
String gpuNameList = FileUtils.readString(context, "gpu_cards.json");
+ if (gpuNameList == null) return "";
String deviceId = "";
try {
JSONArray jsonArray = new JSONArray(gpuNameList);
for (int i = 0; i < jsonArray.length(); i++) {
JSONObject jobj = jsonArray.getJSONObject(i);
if (jobj.getString("name").contains(gpuName)) {
deviceId = jobj.getString("deviceID");
+ break;
}
}
}
catch (JSONException e) {
+ Log.w("GPUInformation", "Failed to parse gpu_cards.json: " + e.getMessage());
}
return deviceId;
}🤖 Prompt for AI Agents
In `@app/src/main/java/com/winlator/core/GPUInformation.java` around lines 30 -
45, In getDeviceIdFromGPUName, guard against FileUtils.readString returning null
before constructing the JSONArray, avoid swallowing exceptions, and stop on the
first match: first check if gpuNameList is null/empty and immediately return "";
then wrap new JSONArray(gpuNameList) in a try/catch that logs the JSONException
(e.g., Log.e or your logger) and returns "" on error; inside the for loop use
jobj.getString("name").contains(gpuName) safely and when a match is found set
deviceId and break to return the first match.
Comment on lines
+47
to
+62
| public static String getVendorIdFromGPUName(Context context, String gpuName) { | ||
| String gpuNameList = FileUtils.readString(context, "gpu_cards.json"); | ||
| String vendorId = ""; | ||
| try { | ||
| JSONArray jsonArray = new JSONArray(gpuNameList); | ||
| for (int i = 0; i < jsonArray.length(); i++) { | ||
| JSONObject jobj = jsonArray.getJSONObject(i); | ||
| if (jobj.getString("name").contains(gpuName)) { | ||
| vendorId = jobj.getString("vendorID"); | ||
| } | ||
| } | ||
| } | ||
| catch (JSONException e) { | ||
| } | ||
| return vendorId; | ||
| } |
There was a problem hiding this comment.
Same issues as getDeviceIdFromGPUName.
This method has identical issues: potential null from FileUtils.readString(), empty catch block, and returns last match instead of first.
🛡️ Proposed fix
public static String getVendorIdFromGPUName(Context context, String gpuName) {
String gpuNameList = FileUtils.readString(context, "gpu_cards.json");
+ if (gpuNameList == null) return "";
String vendorId = "";
try {
JSONArray jsonArray = new JSONArray(gpuNameList);
for (int i = 0; i < jsonArray.length(); i++) {
JSONObject jobj = jsonArray.getJSONObject(i);
if (jobj.getString("name").contains(gpuName)) {
vendorId = jobj.getString("vendorID");
+ break;
}
}
}
catch (JSONException e) {
+ Log.w("GPUInformation", "Failed to parse gpu_cards.json: " + e.getMessage());
}
return vendorId;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public static String getVendorIdFromGPUName(Context context, String gpuName) { | |
| String gpuNameList = FileUtils.readString(context, "gpu_cards.json"); | |
| String vendorId = ""; | |
| try { | |
| JSONArray jsonArray = new JSONArray(gpuNameList); | |
| for (int i = 0; i < jsonArray.length(); i++) { | |
| JSONObject jobj = jsonArray.getJSONObject(i); | |
| if (jobj.getString("name").contains(gpuName)) { | |
| vendorId = jobj.getString("vendorID"); | |
| } | |
| } | |
| } | |
| catch (JSONException e) { | |
| } | |
| return vendorId; | |
| } | |
| public static String getVendorIdFromGPUName(Context context, String gpuName) { | |
| String gpuNameList = FileUtils.readString(context, "gpu_cards.json"); | |
| if (gpuNameList == null) return ""; | |
| String vendorId = ""; | |
| try { | |
| JSONArray jsonArray = new JSONArray(gpuNameList); | |
| for (int i = 0; i < jsonArray.length(); i++) { | |
| JSONObject jobj = jsonArray.getJSONObject(i); | |
| if (jobj.getString("name").contains(gpuName)) { | |
| vendorId = jobj.getString("vendorID"); | |
| break; | |
| } | |
| } | |
| } | |
| catch (JSONException e) { | |
| Log.w("GPUInformation", "Failed to parse gpu_cards.json: " + e.getMessage()); | |
| } | |
| return vendorId; | |
| } |
🤖 Prompt for AI Agents
In `@app/src/main/java/com/winlator/core/GPUInformation.java` around lines 47 -
62, getVendorIdFromGPUName currently assumes FileUtils.readString returns
non-null, swallows JSONExceptions, and assigns vendorId for every match
(returning the last); fix by validating gpuNameList is non-null/empty before
parsing, wrap the JSONArray/JSONObject parsing in a try/catch that logs or
handles the JSONException instead of leaving it empty, and upon finding the
first matching jobj.getString("name").contains(gpuName) return
jobj.getString("vendorID") immediately (or set vendorId and break) to avoid
returning the last match; reference getVendorIdFromGPUName and
FileUtils.readString and ensure the catch block records the exception (e.g., via
logging) and the method returns an empty string when input is invalid or on
parse error.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
8 issues found across 25 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="app/src/main/cpp/extras/vulkan.c">
<violation number="1" location="app/src/main/cpp/extras/vulkan.c:281">
P2: Early returns on error skip Vulkan cleanup, leaking the instance/handle and leaving global state uncleared. Use a cleanup path (like getVulkanVersion) so failures still destroy the instance and close the handle.</violation>
</file>
<file name="app/src/main/java/app/gamenative/ui/component/dialog/GeneralTab.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/component/dialog/GeneralTab.kt:255">
P2: User-facing language labels are hardcoded in code. Move these labels to string resources and reference them so they can be localized and maintained centrally.
(Based on your team's feedback about not hardcoding UI strings or colors.) [FEEDBACK_USED]</violation>
<violation number="2" location="app/src/main/java/app/gamenative/ui/component/dialog/GeneralTab.kt:343">
P2: Avoid hardcoded UI labels; use string resources for Steam type names so they can be localized and managed consistently.
(Based on your team's feedback about not hardcoding UI strings or colors.) [FEEDBACK_USED]</violation>
</file>
<file name="app/src/main/java/com/winlator/core/envvars/EnvVarInfo.kt">
<violation number="1" location="app/src/main/java/com/winlator/core/envvars/EnvVarInfo.kt:241">
P2: The new entry uses the "WINE_NEW_MEDIASOURCE" key but sets identifier to "GALLIUM_HUD", which duplicates the existing WINE_NEW_MEDIASOURCE mapping and overwrites it. Use "GALLIUM_HUD" as the map key so both vars can be resolved correctly.</violation>
</file>
<file name="app/src/main/java/app/gamenative/ui/component/dialog/WineTab.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/component/dialog/WineTab.kt:26">
P2: The second state.config.value assignment overwrites the videoPciDeviceID update because it copies from the original config again. Combine both updates in a single copy so GPU selection isn’t lost.</violation>
</file>
<file name="app/src/main/java/app/gamenative/ui/component/dialog/GraphicsTab.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/component/dialog/GraphicsTab.kt:372">
P2: Avoid mutating state during composition; move this default version update into a LaunchedEffect (keyed on wrapper selection) or guard it so it only updates when the config actually changes.</violation>
</file>
<file name="app/src/main/java/app/gamenative/ui/component/dialog/ControllerTab.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/component/dialog/ControllerTab.kt:48">
P3: Dropdown labels are hardcoded strings. Move them to string resources and reference them via stringResource for localization.
(Based on your team's feedback about avoiding hardcoded UI strings or colors.) [FEEDBACK_USED]</violation>
</file>
<file name="app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt:3281">
P2: Guard against empty gpuName before mapping device/vendor IDs; when gpuName is missing, KeyValueSet returns "" and the contains() match in GPUInformation will select the last GPU entry, leading to incorrect wrapper device/vendor values.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| onCheckedChange = { state.config.value = config.copy(allowSteamUpdates = it) }, | ||
| ) | ||
| } | ||
| val steamTypeItems = listOf("Normal", "Light", "Ultra Light") |
Contributor
There was a problem hiding this comment.
P2: Avoid hardcoded UI labels; use string resources for Steam type names so they can be localized and managed consistently.
(Based on your team's feedback about not hardcoding UI strings or colors.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/java/app/gamenative/ui/component/dialog/GeneralTab.kt, line 343:
<comment>Avoid hardcoded UI labels; use string resources for Steam type names so they can be localized and managed consistently.
(Based on your team's feedback about not hardcoding UI strings or colors.) </comment>
<file context>
@@ -0,0 +1,364 @@
+ onCheckedChange = { state.config.value = config.copy(allowSteamUpdates = it) },
+ )
+ }
+ val steamTypeItems = listOf("Normal", "Light", "Ultra Light")
+ val currentSteamTypeIndex = when (config.steamType.lowercase()) {
+ Container.STEAM_TYPE_LIGHT -> 1
</file context>
| ) | ||
| val displayNameForLanguage: (String) -> String = { code -> | ||
| when (code) { | ||
| "schinese" -> "Simplified Chinese" |
Contributor
There was a problem hiding this comment.
P2: User-facing language labels are hardcoded in code. Move these labels to string resources and reference them so they can be localized and maintained centrally.
(Based on your team's feedback about not hardcoding UI strings or colors.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/java/app/gamenative/ui/component/dialog/GeneralTab.kt, line 255:
<comment>User-facing language labels are hardcoded in code. Move these labels to string resources and reference them so they can be localized and maintained centrally.
(Based on your team's feedback about not hardcoding UI strings or colors.) </comment>
<file context>
@@ -0,0 +1,364 @@
+ )
+ val displayNameForLanguage: (String) -> String = { code ->
+ when (code) {
+ "schinese" -> "Simplified Chinese"
+ "tchinese" -> "Traditional Chinese"
+ "koreana" -> "Korean"
</file context>
app/src/main/java/app/gamenative/ui/component/dialog/WineTab.kt
Outdated
Show resolved
Hide resolved
| else currentConfig.put("async", "0") | ||
| if (selectedId.contains("gplasync", ignoreCase = true)) currentConfig.put("asyncCache", "1") | ||
| else currentConfig.put("asyncCache", "0") | ||
| state.config.value = config.copy(dxwrapperConfig = currentConfig.toString()) |
Contributor
There was a problem hiding this comment.
P2: Avoid mutating state during composition; move this default version update into a LaunchedEffect (keyed on wrapper selection) or guard it so it only updates when the config actually changes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/java/app/gamenative/ui/component/dialog/GraphicsTab.kt, line 372:
<comment>Avoid mutating state during composition; move this default version update into a LaunchedEffect (keyed on wrapper selection) or guard it so it only updates when the config actually changes.</comment>
<file context>
@@ -0,0 +1,455 @@
+ else currentConfig.put("async", "0")
+ if (selectedId.contains("gplasync", ignoreCase = true)) currentConfig.put("asyncCache", "1")
+ else currentConfig.put("asyncCache", "0")
+ state.config.value = config.copy(dxwrapperConfig = currentConfig.toString())
+ }
+ return@SettingsListDropdown
</file context>
| envVars.put("WRAPPER_EXTENSION_BLACKLIST", blacklistedExtensions) | ||
|
|
||
| val gpuName = graphicsDriverConfig.get("gpuName") | ||
| if (gpuName != "Device") { |
Contributor
There was a problem hiding this comment.
P2: Guard against empty gpuName before mapping device/vendor IDs; when gpuName is missing, KeyValueSet returns "" and the contains() match in GPUInformation will select the last GPU entry, leading to incorrect wrapper device/vendor values.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt, line 3281:
<comment>Guard against empty gpuName before mapping device/vendor IDs; when gpuName is missing, KeyValueSet returns "" and the contains() match in GPUInformation will select the last GPU entry, leading to incorrect wrapper device/vendor values.</comment>
<file context>
@@ -3264,6 +3277,13 @@ private fun extractGraphicsDriverFiles(
envVars.put("WRAPPER_EXTENSION_BLACKLIST", blacklistedExtensions)
+ val gpuName = graphicsDriverConfig.get("gpuName")
+ if (gpuName != "Device") {
+ envVars.put("WRAPPER_DEVICE_NAME", gpuName)
+ envVars.put("WRAPPER_DEVICE_ID", GPUInformation.getDeviceIdFromGPUName(context, gpuName))
</file context>
Suggested change
| if (gpuName != "Device") { | |
| if (gpuName.isNotEmpty() && gpuName != "Device") { |
| colors = settingsTileColors(), | ||
| title = { Text(text = stringResource(R.string.directinput_mapper_type)) }, | ||
| value = if (config.dinputMapperType == 1.toByte()) 0 else 1, | ||
| items = listOf("Standard", "XInput Mapper"), |
Contributor
There was a problem hiding this comment.
P3: Dropdown labels are hardcoded strings. Move them to string resources and reference them via stringResource for localization.
(Based on your team's feedback about avoiding hardcoded UI strings or colors.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/java/app/gamenative/ui/component/dialog/ControllerTab.kt, line 48:
<comment>Dropdown labels are hardcoded strings. Move them to string resources and reference them via stringResource for localization.
(Based on your team's feedback about avoiding hardcoded UI strings or colors.) </comment>
<file context>
@@ -0,0 +1,92 @@
+ colors = settingsTileColors(),
+ title = { Text(text = stringResource(R.string.directinput_mapper_type)) },
+ value = if (config.dinputMapperType == 1.toByte()) 0 else 1,
+ items = listOf("Standard", "XInput Mapper"),
+ onItemSelected = { index ->
+ state.config.value = config.copy(dinputMapperType = if (index == 0) 1 else 2)
</file context>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/src/main/java/app/gamenative/ui/component/dialog/WineTab.kt`:
- Around line 34-43: The dropdown starting at SettingsListDropdown duplicates
the "Renderer" dropdown by reusing state.gpuNameIndex and the same items,
causing synced behavior and incomplete updates (it only sets videoPciDeviceID).
Remove this duplicate SettingsListDropdown entirely so GPU selection is only
handled by the existing "Renderer" dropdown (which updates both state.config via
config.copy(...videoPciDeviceID, graphicsDriverConfig) and state.gpuNameIndex);
if you intended a distinct control instead, create a separate index (e.g.,
state.secondaryGpuIndex) and ensure its onItemSelected updates both
videoPciDeviceID and graphicsDriverConfig using config.copy and gpuCardsValues
for the chosen index.
Comment on lines
+34
to
+43
| SettingsListDropdown( | ||
| colors = settingsTileColors(), | ||
| title = { Text(text = stringResource(R.string.gpu_name)) }, | ||
| value = state.gpuNameIndex.value, | ||
| items = state.gpuCards.values.map { it.name }, | ||
| onItemSelected = { | ||
| state.gpuNameIndex.value = it | ||
| state.config.value = config.copy(videoPciDeviceID = gpuCardsValues[it].deviceId) | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Duplicate dropdown shares state with the "Renderer" dropdown above.
This dropdown uses the same state.gpuNameIndex.value and identical items list as the "Renderer" dropdown (lines 19-32), causing them to always sync. Additionally, this handler only updates videoPciDeviceID while the first updates both videoPciDeviceID and graphicsDriverConfig, creating an inconsistent state if this dropdown is modified independently.
If these are intended to be separate settings, they should use different state indices. If they represent the same setting, consider removing this duplicate.
🛠️ Suggested fix if this is a duplicate
Remove the duplicate dropdown (lines 34-43) since the "Renderer" dropdown already handles GPU selection with full state updates.
- SettingsListDropdown(
- colors = settingsTileColors(),
- title = { Text(text = stringResource(R.string.gpu_name)) },
- value = state.gpuNameIndex.value,
- items = state.gpuCards.values.map { it.name },
- onItemSelected = {
- state.gpuNameIndex.value = it
- state.config.value = config.copy(videoPciDeviceID = gpuCardsValues[it].deviceId)
- },
- )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| SettingsListDropdown( | |
| colors = settingsTileColors(), | |
| title = { Text(text = stringResource(R.string.gpu_name)) }, | |
| value = state.gpuNameIndex.value, | |
| items = state.gpuCards.values.map { it.name }, | |
| onItemSelected = { | |
| state.gpuNameIndex.value = it | |
| state.config.value = config.copy(videoPciDeviceID = gpuCardsValues[it].deviceId) | |
| }, | |
| ) | |
| SettingsListDropdown( | |
| colors = settingsTileColors(), | |
| title = { Text(text = stringResource(R.string.renderer)) }, | |
| value = state.gpuNameIndex.value, | |
| items = state.gpuCards.values.map { it.name }, | |
| onItemSelected = { | |
| state.gpuNameIndex.value = it | |
| // Handle both videoPciDeviceID and graphicsDriverConfig updates | |
| }, | |
| ) |
🤖 Prompt for AI Agents
In `@app/src/main/java/app/gamenative/ui/component/dialog/WineTab.kt` around lines
34 - 43, The dropdown starting at SettingsListDropdown duplicates the "Renderer"
dropdown by reusing state.gpuNameIndex and the same items, causing synced
behavior and incomplete updates (it only sets videoPciDeviceID). Remove this
duplicate SettingsListDropdown entirely so GPU selection is only handled by the
existing "Renderer" dropdown (which updates both state.config via
config.copy(...videoPciDeviceID, graphicsDriverConfig) and state.gpuNameIndex);
if you intended a distinct control instead, create a separate index (e.g.,
state.secondaryGpuIndex) and ensure its onItemSelected updates both
videoPciDeviceID and graphicsDriverConfig using config.copy and gpuCardsValues
for the chosen index.
Contributor
There was a problem hiding this comment.
1 issue found across 8 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="app/src/main/cpp/extras/vulkan.c">
<violation number="1" location="app/src/main/cpp/extras/vulkan.c:281">
P2: Early return on create_instance failure skips Vulkan cleanup. This can leak the instance/handle and leave globals set. Clean up before returning, as in the other JNI helpers.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
3 issues found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt:3242">
P1: Leading '/' makes this path absolute, so rootDir is ignored and the delete targets the host path instead of the container. Use a relative path (no leading slash) so cleanup applies under rootDir.</violation>
<violation number="2" location="app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt:3243">
P1: This absolute path ignores rootDir; remove the leading '/' so the file is deleted under the container root.</violation>
<violation number="3" location="app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt:3244">
P1: Absolute child path ignores rootDir; use a relative path so the Vulkan ICD directory is created inside the container root.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@app/src/main/java/app/gamenative/ui/component/dialog/GraphicsTab.kt`:
- Around line 300-313: The hardcoded memValues/memLabels in GraphicsTab.kt are
missing the new larger options; replace these inline lists with the centralized
resource/constant (the string-array in arrays.xml or a shared constant) used
elsewhere so both paths stay consistent: read the memory value labels and
underlying values into the same lists used by SettingsListDropdown, use that
list's lastIndex when computing state.maxDeviceMemoryIndex.value.coerceIn(...),
and keep the onItemSelected logic (updating state.maxDeviceMemoryIndex, building
cfg with KeyValueSet and setting maxDeviceMemory) unchanged.
- Around line 388-396: The else branch mutates state during composition by
unconditionally setting a default DXVK version (creating KeyValueSet, putting
"version", and updating state.config) based on driverType derived from
state.graphicsDrivers/state.graphicsDriverIndex and
config.containerVariant/Container.GLIBC; move that defaulting logic into the DX
Wrapper dropdown's onItemSelected callback (the handler that runs when VKD3D is
selected) so the KeyValueSet manipulation and
state.config.copy(dxwrapperConfig=...) only runs as a side-effect in response to
user selection, and remove the composition-time else branch that performs the
state mutation.
- Around line 94-109: The hardcoded memory value list in GraphicsTab.kt is out
of sync with arrays.xml; replace the inline memValues/memLabels with the
string-array resources (e.g., load R.array.dxvk_max_device_memory_values and
R.array.dxvk_max_device_memory_entries via context.resources.getStringArray or
stringArrayResource) and use those arrays for SettingsListDropdown, ensuring you
coerce state.maxDeviceMemoryIndex.value against the loaded values' lastIndex and
continue to write the selected value into config.graphicsDriverConfig via
KeyValueSet with key "maxDeviceMemory".
🧹 Nitpick comments (3)
app/src/main/java/app/gamenative/utils/ContainerUtils.kt (1)
41-69: Consider extracting common defaults to reduce duplication.All three branches share significant overlap. Extracting the common assignments could improve maintainability:
Optional refactor suggestion
fun setContainerDefaults(context: Context) { // Common defaults for all variants DefaultVersion.VARIANT = Container.BIONIC DefaultVersion.WINE_VERSION = "proton-9.0-arm64ec" DefaultVersion.DEFAULT_GRAPHICS_DRIVER = "Wrapper" DefaultVersion.VKD3D = "2.14.1" // Variant-specific overrides when { GPUInformation.isAdreno8Elite(context) -> { DefaultVersion.DXVK = "2.4.1-gplasync" DefaultVersion.WRAPPER = "Turnip_Gen8_V23" DefaultVersion.STEAM_TYPE = Container.STEAM_TYPE_NORMAL DefaultVersion.ASYNC_CACHE = "1" } GPUInformation.isTurnipCapable(context) -> { DefaultVersion.DXVK = "2.4.1-gplasync" DefaultVersion.WRAPPER = "turnip26.0.0_R8" DefaultVersion.STEAM_TYPE = Container.STEAM_TYPE_NORMAL DefaultVersion.ASYNC_CACHE = "1" } else -> { DefaultVersion.DXVK = "async-1.10.3" DefaultVersion.STEAM_TYPE = Container.STEAM_TYPE_LIGHT DefaultVersion.ASYNC_CACHE = "0" } } }app/src/main/java/app/gamenative/utils/ManifestComponentHelper.kt (1)
134-139: Simplify the redundantisInstalledcheck to improve code clarity.The
isInstalledcomputation at line 137 will always evaluate tofalsebecause:
availableIdsis set tooptions.keys(line 134)- The block only executes when
!options.containsKey(entry.id)(line 136)- Therefore
entry.idcan never be inavailableIdsReplace the variable with the literal
false:♻️ Suggested simplification
val availableIds = options.keys.toSet() manifest.forEach { entry -> if (!options.containsKey(entry.id)) { - val isInstalled = availableIds.contains(entry.id) - options[entry.id] = VersionOption(entry.id, entry.id, isManifest = true, isInstalled = isInstalled) + options[entry.id] = VersionOption(entry.id, entry.id, isManifest = true, isInstalled = false) } }app/src/main/java/app/gamenative/ui/component/dialog/GraphicsTab.kt (1)
379-385: Unused EnvVars round-trip.
envVarsSetis created fromconfig.envVarsbut no modifications are made to it before converting back to string. This appears to be dead code or a remnant from a previous implementation.Proposed cleanup
val version = selectedId.ifEmpty { StringUtils.parseIdentifier(items.getOrNull(it).orEmpty()) } val currentConfig = KeyValueSet(config.dxwrapperConfig) currentConfig.put("version", version) - val envVarsSet = EnvVars(config.envVars) if (version.contains("async", ignoreCase = true)) currentConfig.put("async", "1") else currentConfig.put("async", "0") if (version.contains("gplasync", ignoreCase = true)) currentConfig.put("asyncCache", "1") else currentConfig.put("asyncCache", "0") - state.config.value = - config.copy(dxwrapperConfig = currentConfig.toString(), envVars = envVarsSet.toString()) + state.config.value = config.copy(dxwrapperConfig = currentConfig.toString())
Comment on lines
+94
to
+109
| run { | ||
| val memValues = listOf("0", "512", "1024", "2048", "4096") | ||
| val memLabels = listOf("0 MB", "512 MB", "1024 MB", "2048 MB", "4096 MB") | ||
| SettingsListDropdown( | ||
| colors = settingsTileColors(), | ||
| title = { Text(text = stringResource(R.string.max_device_memory)) }, | ||
| value = state.maxDeviceMemoryIndex.value.coerceIn(0, memValues.lastIndex), | ||
| items = memLabels, | ||
| onItemSelected = { idx -> | ||
| state.maxDeviceMemoryIndex.value = idx | ||
| val cfg = KeyValueSet(config.graphicsDriverConfig) | ||
| cfg.put("maxDeviceMemory", memValues[idx]) | ||
| state.config.value = config.copy(graphicsDriverConfig = cfg.toString()) | ||
| }, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Memory options are out of sync with arrays.xml.
The hardcoded memory values ["0", "512", "1024", "2048", "4096"] don't include the new larger VRAM options (8192 MB, 12288 MB, 16384 MB) that were added to dxvk_max_device_memory_entries in arrays.xml. Users won't see the expanded memory options in the Bionic variant UI.
Consider loading these values from the string array resource for consistency:
Proposed fix
- run {
- val memValues = listOf("0", "512", "1024", "2048", "4096")
- val memLabels = listOf("0 MB", "512 MB", "1024 MB", "2048 MB", "4096 MB")
+ run {
+ val memValues = listOf("0", "512", "1024", "2048", "4096", "8192", "12288", "16384")
+ val memLabels = listOf("0 MB", "512 MB", "1024 MB", "2048 MB", "4096 MB", "8192 MB", "12288 MB", "16384 MB")🤖 Prompt for AI Agents
In `@app/src/main/java/app/gamenative/ui/component/dialog/GraphicsTab.kt` around
lines 94 - 109, The hardcoded memory value list in GraphicsTab.kt is out of sync
with arrays.xml; replace the inline memValues/memLabels with the string-array
resources (e.g., load R.array.dxvk_max_device_memory_values and
R.array.dxvk_max_device_memory_entries via context.resources.getStringArray or
stringArrayResource) and use those arrays for SettingsListDropdown, ensuring you
coerce state.maxDeviceMemoryIndex.value against the loaded values' lastIndex and
continue to write the selected value into config.graphicsDriverConfig via
KeyValueSet with key "maxDeviceMemory".
Comment on lines
+300
to
+313
| val memValues = listOf("0", "512", "1024", "2048", "4096") | ||
| val memLabels = listOf("0 MB", "512 MB", "1024 MB", "2048 MB", "4096 MB") | ||
| SettingsListDropdown( | ||
| colors = settingsTileColors(), | ||
| title = { Text(text = stringResource(R.string.max_device_memory)) }, | ||
| value = state.maxDeviceMemoryIndex.value.coerceIn(0, memValues.lastIndex), | ||
| items = memLabels, | ||
| onItemSelected = { idx -> | ||
| state.maxDeviceMemoryIndex.value = idx | ||
| val cfg = KeyValueSet(config.graphicsDriverConfig) | ||
| cfg.put("maxDeviceMemory", memValues[idx]) | ||
| state.config.value = config.copy(graphicsDriverConfig = cfg.toString()) | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Same memory options inconsistency in non-Bionic path.
This section also uses the same hardcoded memory values missing the new larger options. For consistency, update to match the values in arrays.xml or extract to a shared constant.
Proposed fix
- val memValues = listOf("0", "512", "1024", "2048", "4096")
- val memLabels = listOf("0 MB", "512 MB", "1024 MB", "2048 MB", "4096 MB")
+ val memValues = listOf("0", "512", "1024", "2048", "4096", "8192", "12288", "16384")
+ val memLabels = listOf("0 MB", "512 MB", "1024 MB", "2048 MB", "4096 MB", "8192 MB", "12288 MB", "16384 MB")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| val memValues = listOf("0", "512", "1024", "2048", "4096") | |
| val memLabels = listOf("0 MB", "512 MB", "1024 MB", "2048 MB", "4096 MB") | |
| SettingsListDropdown( | |
| colors = settingsTileColors(), | |
| title = { Text(text = stringResource(R.string.max_device_memory)) }, | |
| value = state.maxDeviceMemoryIndex.value.coerceIn(0, memValues.lastIndex), | |
| items = memLabels, | |
| onItemSelected = { idx -> | |
| state.maxDeviceMemoryIndex.value = idx | |
| val cfg = KeyValueSet(config.graphicsDriverConfig) | |
| cfg.put("maxDeviceMemory", memValues[idx]) | |
| state.config.value = config.copy(graphicsDriverConfig = cfg.toString()) | |
| }, | |
| ) | |
| val memValues = listOf("0", "512", "1024", "2048", "4096", "8192", "12288", "16384") | |
| val memLabels = listOf("0 MB", "512 MB", "1024 MB", "2048 MB", "4096 MB", "8192 MB", "12288 MB", "16384 MB") | |
| SettingsListDropdown( | |
| colors = settingsTileColors(), | |
| title = { Text(text = stringResource(R.string.max_device_memory)) }, | |
| value = state.maxDeviceMemoryIndex.value.coerceIn(0, memValues.lastIndex), | |
| items = memLabels, | |
| onItemSelected = { idx -> | |
| state.maxDeviceMemoryIndex.value = idx | |
| val cfg = KeyValueSet(config.graphicsDriverConfig) | |
| cfg.put("maxDeviceMemory", memValues[idx]) | |
| state.config.value = config.copy(graphicsDriverConfig = cfg.toString()) | |
| }, | |
| ) |
🤖 Prompt for AI Agents
In `@app/src/main/java/app/gamenative/ui/component/dialog/GraphicsTab.kt` around
lines 300 - 313, The hardcoded memValues/memLabels in GraphicsTab.kt are missing
the new larger options; replace these inline lists with the centralized
resource/constant (the string-array in arrays.xml or a shared constant) used
elsewhere so both paths stay consistent: read the memory value labels and
underlying values into the same lists used by SettingsListDropdown, use that
list's lastIndex when computing state.maxDeviceMemoryIndex.value.coerceIn(...),
and keep the onItemSelected logic (updating state.maxDeviceMemoryIndex, building
cfg with KeyValueSet and setting maxDeviceMemory) unchanged.
Comment on lines
+388
to
+396
| } else { | ||
| // Ensure default version for vortek-like when hidden | ||
| val driverType = StringUtils.parseIdentifier(state.graphicsDrivers.value.getOrNull(state.graphicsDriverIndex.value).orEmpty()) | ||
| val isVortekLike = config.containerVariant.equals(Container.GLIBC) && (driverType == "vortek" || driverType == "adreno" || driverType == "sd-8-elite") | ||
| val version = if (isVortekLike) "1.10.3" else "2.4.1" | ||
| val currentConfig = KeyValueSet(config.dxwrapperConfig) | ||
| currentConfig.put("version", version) | ||
| state.config.value = config.copy(dxwrapperConfig = currentConfig.toString()) | ||
| } |
There was a problem hiding this comment.
State mutation during composition causes potential recomposition loop.
Lines 393-395 execute unconditionally during composition when VKD3D is selected. This violates Compose's rule against side effects during composition and can cause infinite recomposition or state being overwritten unexpectedly.
The intent appears to be setting a default DXVK version when the dropdown is hidden, but this should be handled in the onItemSelected callback of the DX Wrapper dropdown (line 337-340) when VKD3D is selected, not during composition.
Proposed fix: Move default version logic to the selection callback
SettingsListDropdown(
colors = settingsTileColors(),
title = { Text(text = stringResource(R.string.dx_wrapper)) },
value = state.dxWrapperIndex.value,
items = state.dxWrappers,
onItemSelected = {
state.dxWrapperIndex.value = it
- state.config.value = config.copy(dxwrapper = StringUtils.parseIdentifier(state.dxWrappers[it]))
+ val newWrapper = StringUtils.parseIdentifier(state.dxWrappers[it])
+ var newConfig = config.copy(dxwrapper = newWrapper)
+ // Set default DXVK version when switching to VKD3D
+ if (newWrapper == "vkd3d") {
+ val driverType = StringUtils.parseIdentifier(state.graphicsDrivers.value.getOrNull(state.graphicsDriverIndex.value).orEmpty())
+ val isVortekLike = config.containerVariant.equals(Container.GLIBC) && (driverType == "vortek" || driverType == "adreno" || driverType == "sd-8-elite")
+ val version = if (isVortekLike) "1.10.3" else "2.4.1"
+ val currentConfig = KeyValueSet(config.dxwrapperConfig)
+ currentConfig.put("version", version)
+ newConfig = newConfig.copy(dxwrapperConfig = currentConfig.toString())
+ }
+ state.config.value = newConfig
},
)Then remove the else branch (lines 388-396) that mutates state during composition:
}
- } else {
- // Ensure default version for vortek-like when hidden
- val driverType = StringUtils.parseIdentifier(state.graphicsDrivers.value.getOrNull(state.graphicsDriverIndex.value).orEmpty())
- val isVortekLike = config.containerVariant.equals(Container.GLIBC) && (driverType == "vortek" || driverType == "adreno" || driverType == "sd-8-elite")
- val version = if (isVortekLike) "1.10.3" else "2.4.1"
- val currentConfig = KeyValueSet(config.dxwrapperConfig)
- currentConfig.put("version", version)
- state.config.value = config.copy(dxwrapperConfig = currentConfig.toString())
}
}🤖 Prompt for AI Agents
In `@app/src/main/java/app/gamenative/ui/component/dialog/GraphicsTab.kt` around
lines 388 - 396, The else branch mutates state during composition by
unconditionally setting a default DXVK version (creating KeyValueSet, putting
"version", and updating state.config) based on driverType derived from
state.graphicsDrivers/state.graphicsDriverIndex and
config.containerVariant/Container.GLIBC; move that defaulting logic into the DX
Wrapper dropdown's onItemSelected callback (the handler that runs when VKD3D is
selected) so the KeyValueSet manipulation and
state.config.copy(dxwrapperConfig=...) only runs as a side-effect in response to
user selection, and remove the composition-time else branch that performs the
state mutation.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt`:
- Around line 3241-3254: GPUInformation.getRenderer(...) can return null, so the
call to .contains("Mali") may NPE; update the conditional around
container.wineVersion and GPUInformation.getRenderer to perform a null-safe
check (e.g., store GPUInformation.getRenderer(...) result in a local variable
like renderer = GPUInformation.getRenderer(...) and check renderer != null &&
!renderer.contains("Mali")) before invoking TarCompressorUtils.extract for the
zink_dlls path; ensure you reference container.wineVersion and
GPUInformation.getRenderer in the new guard to preserve the original logic.
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.
Summary by cubic
Updated graphics wrapper and defaults, added Adreno 8 Elite support and DXVK 1.11.1-sarek, and rebuilt the container config UI into tabs to fix crashes and make settings easier.
New Features
Refactors
Written for commit 7942242. Summary will update on new commits.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.