D7vk support #338
D7vk support #338
Conversation
📝 WalkthroughWalkthroughAdds first-class D7VK support across UI, setup/extraction, container core, enums/constants, and resources: version lists, per-wrapper state, validation/defaulting, extraction logging, and unconditional config updates when wrapper/version changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as ContainerConfigDialog (UI)
participant Config as dxwrapperConfig
participant Setup as XServerScreen
participant Container as Container core
participant FS as Filesystem/Extraction
Note over UI,Config: User selects wrapper and per-wrapper version
UI->>Config: set dxwrapper (e.g., "d7vk-1.1"), set version indices
Config-->>UI: confirm selection/state
alt selected wrapper starts with "d7vk-"
UI->>Setup: trigger setupWineSystemFiles(dxwrapper)
Setup->>Config: read/validate version (default to DefaultVersion.D7VK if missing)
Setup->>Container: request extraction for dxwrapper (d7vk-*)
Container->>FS: extract DXVK/D7VK/D8VK DLLs
FS-->>Container: extraction result
Container-->>Setup: extraction applied
else other dx wrapper variants
UI->>Setup: trigger setupWineSystemFiles(dxwrapper=...)
Setup->>Container: request extraction for dxwrapper
end
Note over UI,Config: UI keeps per-wrapper version indices synchronized with config
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
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 |
There was a problem hiding this comment.
Added in the option to choose D7VK versions (and 1.1). Added some logic to fix an issue where it was auto-forcing the D7VK to an invalid version (2.4.1) which is due to the VK3D having a DXVK version shipped with it.
|
Going to update this PR tomorrow to add the 1.1 instead as that has large improvements over 1.0 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/app/gamenative/ui/component/dialog/ContainerConfigDialog.kt (1)
41-47: Duplicate import ofTextButton.
TextButtonis imported twice (lines 41 and 47). Remove one of the duplicates.🔎 Proposed fix
import androidx.compose.material3.TextButton import androidx.compose.material3.OutlinedTextField import androidx.compose.material3.Scaffold import androidx.compose.material3.Slider import androidx.compose.material3.CircularProgressIndicator import androidx.compose.material3.Text -import androidx.compose.material3.TextButton
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/ui/component/dialog/ContainerConfigDialog.kt (1)
482-516: Consider usingDefaultVersionconstants instead of hardcoded index 0.When resetting to a default version, using
DefaultVersion.D7VKandDefaultVersion.DXVKwould be more robust and consistent with how defaults are handled elsewhere in the codebase (e.g., line 709 usesDefaultVersion.DXVK).🔎 Proposed fix
when (config.dxwrapper) { "d7vk" -> { // Check if version is valid for D7VK val isValidD7VKVersion = d7vkVersionsAll.any { StringUtils.parseIdentifier(it) == savedVersion } if (!isValidD7VKVersion && d7vkVersionsAll.isNotEmpty()) { - // Reset to first D7VK version - val defaultVersion = StringUtils.parseIdentifier(d7vkVersionsAll[0]) + // Reset to default D7VK version + val defaultIdx = d7vkVersionsAll.indexOfFirst { + StringUtils.parseIdentifier(it) == DefaultVersion.D7VK + }.coerceAtLeast(0) + val defaultVersion = StringUtils.parseIdentifier(d7vkVersionsAll[defaultIdx]) currentConfig.put("version", defaultVersion) config = config.copy(dxwrapperConfig = currentConfig.toString()) - d7vkVersionIndex = 0 + d7vkVersionIndex = defaultIdx } } "dxvk" -> { // Check if version is valid for DXVK val isValidDXVKVersion = dxvkVersionsAll.any { StringUtils.parseIdentifier(it) == savedVersion } if (!isValidDXVKVersion && dxvkVersionsAll.isNotEmpty()) { - // Reset to first DXVK version - val defaultVersion = StringUtils.parseIdentifier(dxvkVersionsAll[0]) + // Reset to default DXVK version + val defaultIdx = dxvkVersionsAll.indexOfFirst { + StringUtils.parseIdentifier(it) == DefaultVersion.DXVK + }.coerceAtLeast(0) + val defaultVersion = StringUtils.parseIdentifier(dxvkVersionsAll[defaultIdx]) currentConfig.put("version", version) config = config.copy(dxwrapperConfig = currentConfig.toString()) - dxvkVersionIndex = 0 + dxvkVersionIndex = defaultIdx } } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/src/main/assets/dxwrapper/d7vk-1.1.tzstapp/src/main/java/app/gamenative/ui/component/dialog/ContainerConfigDialog.ktapp/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/com/winlator/container/Container.javaapp/src/main/java/com/winlator/contents/ContentProfile.javaapp/src/main/java/com/winlator/core/DefaultVersion.javaapp/src/main/res/values/arrays.xml
🧰 Additional context used
🧬 Code graph analysis (1)
app/src/main/java/app/gamenative/ui/component/dialog/ContainerConfigDialog.kt (3)
app/src/main/java/com/winlator/core/KeyValueSet.java (1)
KeyValueSet(7-169)app/src/main/java/app/gamenative/ui/component/settings/SettingsListDropdown.kt (1)
SettingsListDropdown(40-125)app/src/main/java/app/gamenative/ui/theme/Color.kt (1)
settingsTileColors(27-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (8)
app/src/main/java/com/winlator/core/DefaultVersion.java (1)
18-18: LGTM!The D7VK version constant follows the established pattern and is consistent with other wrapper version constants in the class.
app/src/main/java/com/winlator/contents/ContentProfile.java (1)
28-28: LGTM!The D7VK content type enum addition is properly placed alongside other DirectX wrapper types and follows the established naming convention.
app/src/main/java/com/winlator/container/Container.java (1)
850-850: LGTM!The extension of the dxwrapper eligibility check to include
d7vk-*prefixes is consistent with the existing handling ofd8vk-*anddxvk-*variants.app/src/main/res/values/arrays.xml (1)
79-79: LGTM!The D7VK resource entries are properly integrated:
- "D7VK" added to wrapper selection list
- Version array initialized with "1.1" (matching DefaultVersion.D7VK)
- Logical placement alongside other wrapper options
Also applies to: 83-85
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt (2)
2145-2155: Verify the intentional version enforcement for D7VK.The D7VK version normalization logic differs from the DXVK implementation:
- D7VK (lines 2145-2155): Forces
DefaultVersion.D7VK("1.1") when the version is null or doesn't match the default- DXVK (lines 2140-2144): Simply appends the version from config without validation
This inconsistency means:
- Users cannot select custom D7VK versions (e.g., future 1.2 or custom builds)
- Any existing D7VK 1.0 configurations will be automatically upgraded to 1.1
If this forced versioning is intentional (e.g., to ensure users benefit from 1.1 improvements mentioned in the PR), consider:
- Adding a comment explaining why D7VK differs from DXVK
- Evaluating whether DXVK should have similar validation
If unintentional, align with DXVK behavior by removing the
version != DefaultVersion.D7VKcondition.🔎 Suggested refactor to align with DXVK behavior
if (xServerState.value.dxwrapper == "d7vk") { - // Set version for d7vk or use default if not found. - var version = xServerState.value.dxwrapperConfig?.get("version") - if (version == null || version != DefaultVersion.D7VK) { - version = DefaultVersion.D7VK - xServerState.value.dxwrapperConfig?.put("version", version) - } xServerState.value = xServerState.value.copy( - dxwrapper = "d7vk-" + version, + dxwrapper = "d7vk-" + xServerState.value.dxwrapperConfig?.get("version"), ) }
2335-2341: LGTM!The updated comments and log messages appropriately reflect the expanded support for D7VK alongside DXVK and D8VK wrappers.
app/src/main/java/app/gamenative/ui/component/dialog/ContainerConfigDialog.kt (2)
149-149: LGTM!The D7VK version list initialization follows the established pattern used for DXVK and VKD3D, correctly initializing base versions from resources, creating mutable state, and augmenting with installed content profiles.
Also applies to: 191-191, 231-231
714-742: LGTM!Good addition of the guard clause to ensure this DXVK-specific async version handling only runs when the DXVK wrapper is selected, preventing unintended side effects when D7VK or other wrappers are active.
app/src/main/java/app/gamenative/ui/component/dialog/ContainerConfigDialog.kt
Show resolved
Hide resolved
app/src/main/java/app/gamenative/ui/component/dialog/ContainerConfigDialog.kt
Show resolved
Hide resolved
|
Will do some final tests to ensure that everything is working correctly. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
@app/src/main/java/app/gamenative/ui/component/dialog/ContainerConfigDialog.kt:
- Around line 591-597: Remove the composition-time mutation of config: delete
the else-if block that checks isVKD3D and writes version into
config.dxwrapperConfig (the block that constructs
KeyValueSet(config.dxwrapperConfig), puts "version", and assigns config =
config.copy(...)). This logic is already handled in
LaunchedEffect(graphicsDriverIndex, dxWrapperIndex), so stop performing
side-effectful updates to config during composition (remove references to
isVKD3D/isVortekLike in that block).
- Around line 535-554: The code sets version fields by directly indexing
dxvkVersionsAll[0] and d7vkVersionsAll[0] which can throw
IndexOutOfBoundsException; update the dxwrapper switch in
ContainerConfigDialog.kt (symbols: dxvkVersionIndex, d7vkVersionIndex,
dxvkVersionsAll, d7vkVersionsAll, currentConfig.put) to first check that the
respective list is not empty (e.g., if (dxvkVersionsAll.isNotEmpty()) {
dxvkVersionIndex = 0; val newVersion =
StringUtils.parseIdentifier(dxvkVersionsAll[0]); currentConfig.put("version",
newVersion) } else { /* set a safe default or skip putting version */ }) and do
the equivalent for d7vkVersionsAll so you only parse and store index 0 when
present and avoid updating the index when the list is empty.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/main/java/app/gamenative/ui/component/dialog/ContainerConfigDialog.ktapp/src/main/res/values/strings.xml
🔇 Additional comments (7)
app/src/main/res/values/strings.xml (1)
117-117: LGTM! String resource properly added.The D7VK version string resource follows the same pattern as the existing DXVK version string (line 116) and is correctly placed in the Win Components section for proper internationalization.
app/src/main/java/app/gamenative/ui/component/dialog/ContainerConfigDialog.kt (6)
149-149: LGTM! D7VK version lists properly initialized.The D7VK version base and runtime lists follow the same pattern as DXVK versions, ensuring consistency across wrapper implementations.
Also applies to: 191-191
231-231: LGTM! D7VK content profiles properly loaded.The D7VK versions are correctly augmented with installed content profiles, following the same pattern as DXVK and VKD3D.
479-479: LGTM! D7VK version index state properly initialized.The D7VK version index state variable follows the same pattern as the DXVK version index.
482-516: Good addition, but bounds checking still needed.The validation logic properly fixes version mismatches when loading containers, which prevents crashes from stale or invalid version references. However, the bounds checking issue flagged in past reviews (lines 494-500 and 507-513) still applies: accessing
d7vkVersionsAll[0]ordxvkVersionsAll[0]without checking for empty lists can throwIndexOutOfBoundsException.Add defensive checks before accessing index 0:
"d7vk" -> { val isValidD7VKVersion = d7vkVersionsAll.any { StringUtils.parseIdentifier(it) == savedVersion } if (!isValidD7VKVersion && d7vkVersionsAll.isNotEmpty()) { val defaultVersion = StringUtils.parseIdentifier(d7vkVersionsAll[0]) currentConfig.put("version", defaultVersion) config = config.copy(dxwrapperConfig = currentConfig.toString()) d7vkVersionIndex = 0 } else if (d7vkVersionsAll.isEmpty()) { // Handle empty list case - perhaps log warning or use a hardcoded default timber.log.Timber.w("D7VK versions list is empty, cannot reset version") } }Similar pattern for the "dxvk" case.
599-618: LGTM! D7VK version dropdown properly implemented.The D7VK version dropdown is correctly implemented with:
- Proper use of string resource for the title (line 606)
- Bounds checking via
coerceIn(line 607)- Correct state management and config updates
- Conditional visibility based on wrapper selection
715-743: Good improvement to scope DXVK version handling.The addition of the wrapper type check (lines 715-718) ensures DXVK version logic only runs when the DXVK wrapper is actually selected, preventing unintended version updates for D7VK or VKD3D wrappers. This makes the version management more robust and prevents conflicts between different wrapper types.
app/src/main/java/app/gamenative/ui/component/dialog/ContainerConfigDialog.kt
Show resolved
Hide resolved
app/src/main/java/app/gamenative/ui/component/dialog/ContainerConfigDialog.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/app/gamenative/ui/component/dialog/ContainerConfigDialog.kt (1)
706-715: Missing D7VK version index synchronization.DXVK has this
LaunchedEffectto sync the version index fromconfig.dxwrapperConfigwhen the dialog loads, ensuring the UI displays the correct saved version. D7VK lacks equivalent logic. When the dialog loads with a saved D7VK container,d7vkVersionIndexremains at its initial value of 0 (line 479) instead of reflecting the actual saved version, causing the dropdown at line 610 to display the first version in the list rather than the configured version.🔄 Proposed fix: Add D7VK version index sync
Add a similar effect for D7VK after line 715:
LaunchedEffect(versionsLoaded, d7vkVersionsAll, config.dxwrapper, config.dxwrapperConfig) { if (!versionsLoaded) return@LaunchedEffect if (config.dxwrapper != "d7vk") return@LaunchedEffect val kvs = KeyValueSet(config.dxwrapperConfig) val configuredVersion = kvs.get("version") val foundIndex = d7vkVersionsAll.indexOfFirst { StringUtils.parseIdentifier(it) == configuredVersion } val defaultIndex = d7vkVersionsAll.indexOfFirst { StringUtils.parseIdentifier(it) == DefaultVersion.D7VK }.coerceAtLeast(0) val newIdx = if (foundIndex >= 0) foundIndex else defaultIndex if (d7vkVersionIndex != newIdx) d7vkVersionIndex = newIdx }
🤖 Fix all issues with AI agents
In
@app/src/main/java/app/gamenative/ui/component/dialog/ContainerConfigDialog.kt:
- Around line 595-600: The code block that builds and mutates `config` using
`val version = if (isVortekLike) "1.10.3" else "2.4.1"`,
`KeyValueSet(config.dxwrapperConfig)` and `config = config.copy(dxwrapperConfig
= currentConfig.toString())` must be removed because it performs a
composition-side effect that is already handled in
`LaunchedEffect(graphicsDriverIndex, dxWrapperIndex)` and incorrectly runs for
D7VK; delete that entire else-branch so DXVK/VKD3D versioning is only set by the
existing `LaunchedEffect` and D7VK retains its own handling in the D7VK-specific
code paths.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/main/java/app/gamenative/ui/component/dialog/ContainerConfigDialog.ktapp/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/main/res/values/strings.xml
- app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt
🧰 Additional context used
🧬 Code graph analysis (1)
app/src/main/java/app/gamenative/ui/component/dialog/ContainerConfigDialog.kt (3)
app/src/main/java/com/winlator/core/KeyValueSet.java (1)
KeyValueSet(7-169)app/src/main/java/app/gamenative/ui/component/settings/SettingsListDropdown.kt (1)
SettingsListDropdown(40-125)app/src/main/java/app/gamenative/ui/theme/Color.kt (1)
settingsTileColors(27-32)
🔇 Additional comments (4)
app/src/main/java/app/gamenative/ui/component/dialog/ContainerConfigDialog.kt (4)
149-149: LGTM: D7VK state initialization follows established patterns.The D7VK version lists and state variables are initialized consistently with the existing DXVK implementation.
Also applies to: 191-191, 231-231, 479-479
482-516: LGTM: Validation logic properly handles empty version lists.The bounds checks (
isNotEmpty()) correctly preventIndexOutOfBoundsExceptionwhen version lists are empty, addressing concerns from previous reviews.
535-558: LGTM: Wrapper switching safely resets versions with bounds checking.The addition of
isNotEmpty()guards before accessing[0]correctly addresses theIndexOutOfBoundsExceptionconcerns raised in previous reviews.
602-621: LGTM: D7VK version dropdown properly implemented.The dropdown uses proper string resources (addressing the previous review comment) and safely handles bounds with
coerceIn. The version selection logic correctly updatesdxwrapperConfig.
| } else { | ||
| // Ensure default version for vortek-like when hidden | ||
| val version = if (isVortekLike) "1.10.3" else "2.4.1" | ||
| val currentConfig = KeyValueSet(config.dxwrapperConfig) | ||
| currentConfig.put("version", version) | ||
| config = config.copy(dxwrapperConfig = currentConfig.toString()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Refactor: Remove composition side effect (duplicate issue).
This block mutates config state during composition and was flagged in previous reviews as redundant. The logic is already handled in LaunchedEffect(graphicsDriverIndex, dxWrapperIndex) at line 691. Additionally, this block incorrectly runs when D7VK is selected (due to the condition at line 575), setting a DXVK/VKD3D version when D7VK should use its own version handling at lines 602-621.
♻️ Remove redundant block
)
- } else {
- val version = if (isVortekLike) "1.10.3" else "2.4.1"
- val currentConfig = KeyValueSet(config.dxwrapperConfig)
- currentConfig.put("version", version)
- config = config.copy(dxwrapperConfig = currentConfig.toString())
}
}🤖 Prompt for AI Agents
In
@app/src/main/java/app/gamenative/ui/component/dialog/ContainerConfigDialog.kt
around lines 595 - 600, The code block that builds and mutates `config` using
`val version = if (isVortekLike) "1.10.3" else "2.4.1"`,
`KeyValueSet(config.dxwrapperConfig)` and `config = config.copy(dxwrapperConfig
= currentConfig.toString())` must be removed because it performs a
composition-side effect that is already handled in
`LaunchedEffect(graphicsDriverIndex, dxWrapperIndex)` and incorrectly runs for
D7VK; delete that entire else-branch so DXVK/VKD3D versioning is only set by the
existing `LaunchedEffect` and D7VK retains its own handling in the D7VK-specific
code paths.
1 participant
Added D7VK Support
This PR adds support for D7VK as a graphics wrapper option alongside DXVK and VKD3D.
What is D7VK?
D7VK is a Vulkan-based translation layer for DirectX 7/8/9, designed to improve compatibility and performance for older Windows games on ARM devices. Unlike DXVK (which focuses on DirectX 10/11), D7VK specifically targets legacy DirectX APIs commonly used by games from the late 1990s to mid-2000s.
Benefits (According to D7VK Repository):
Changes:
Note: D7VK is still in a decently early stage of development, so games will still be a while before in a good state.
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.