Fix Steam depot filtering for unlicensed region/platform depots - #929
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Steam Deck metadata to DepotInfo, adds chunked license queries in the DAO, builds licensed-depot maps in SteamService, enforces license- and Steam-Deck-aware depot filtering with tightened manifest checks, updates UI size calc to use filtered depots, and updates tests accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as LibraryViewModel
participant Svc as SteamService
participant DAO as SteamLicenseDao
participant DB as Database
UI->>Svc: request downloadable depots (apps, prefs)
Svc->>DAO: findLicenses(packageIds)
DAO->>DB: _findLicenses(packageIds chunk)
DB-->>DAO: license rows
DAO-->>Svc: aggregated license rows
Svc->>Svc: buildLicensedDepotMap(apps)
Svc->>Svc: compute eligibleDepots (manifests, language, arch)
Svc->>Svc: resolveDownloadableDepots (apply isWindowsCompatible, steamDeck, license gating)
Svc-->>UI: filtered depot list per app
UI-->>UI: compute sizes from filtered depot manifests
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/src/test/java/app/gamenative/service/SdCardDetectionTest.kt (1)
52-103: Add regression coverage for the new license/Deck branches.These updates only re-thread the new boolean argument through the old manifest/shared-install cases. The actual fix in this PR lives in the
licensedDepotIdsandsteamDeck && hasNonDeckWindowsbranches, so please add at least one licensed-vs-unlicensed case and one Deck-vs-regular-Windows case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/test/java/app/gamenative/service/SdCardDetectionTest.kt` around lines 52 - 103, Existing tests only exercise manifest/shared-install branches; add unit tests calling SteamService.filterForDownloadableDepots that cover the licensedDepotIds branch and the steamDeck vs hasNonDeckWindows behavior: (1) a pair of tests that pass a depot id present in the licensedDepotIds set and one not present to assert licensed depots are accepted/rejected appropriately, referencing the licensedDepotIds behavior in SteamService; and (2) tests that toggle the steamDeck boolean and the hasNonDeckWindows condition (simulate a depot with Windows manifests and set the steamDeck flag true/false) to assert Deck-specific logic vs regular-Windows logic in filterForDownloadableDepots. Ensure tests create depots with the same manifest shapes used elsewhere in this file so they exercise the new branches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/app/gamenative/service/SteamService.kt`:
- Around line 735-737: Compute the arch/Deck preference flags from the
already-filtered eligible depots rather than from the raw depots map: after you
apply the license/language/manifest/DLC filtering that produces the eligible
depots collection used by getMainAppDepots() and getDownloadableDepots(), derive
has64Bit and hasNonDeckWindowsDepot (and any similar checks used in the blocks
around getMainAppDepots/getDownloadableDepots) from that eligible set (e.g.,
eligibleDepots.values.any { it.is64Bit } and eligibleDepots.values.any {
!it.steamDeck && it.isWindowsCompatible }) so an unlicensed or rejected depot
cannot suppress a valid installable depot. Ensure you replace uses of the
original depots map in functions/members like has64Bit and
hasNonDeckWindowsDepot with the filtered/eligible depot collection before making
decisions.
---
Nitpick comments:
In `@app/src/test/java/app/gamenative/service/SdCardDetectionTest.kt`:
- Around line 52-103: Existing tests only exercise manifest/shared-install
branches; add unit tests calling SteamService.filterForDownloadableDepots that
cover the licensedDepotIds branch and the steamDeck vs hasNonDeckWindows
behavior: (1) a pair of tests that pass a depot id present in the
licensedDepotIds set and one not present to assert licensed depots are
accepted/rejected appropriately, referencing the licensedDepotIds behavior in
SteamService; and (2) tests that toggle the steamDeck boolean and the
hasNonDeckWindows condition (simulate a depot with Windows manifests and set the
steamDeck flag true/false) to assert Deck-specific logic vs regular-Windows
logic in filterForDownloadableDepots. Ensure tests create depots with the same
manifest shapes used elsewhere in this file so they exercise the new branches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7e17973b-af45-4b67-8575-9027804b26b4
📒 Files selected for processing (6)
app/src/main/java/app/gamenative/data/DepotInfo.ktapp/src/main/java/app/gamenative/db/dao/SteamLicenseDao.ktapp/src/main/java/app/gamenative/service/SteamService.ktapp/src/main/java/app/gamenative/ui/model/LibraryViewModel.ktapp/src/main/java/app/gamenative/utils/KeyValueUtils.ktapp/src/test/java/app/gamenative/service/SdCardDetectionTest.kt
6a505e5 to
f9cc6f9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/src/main/java/app/gamenative/service/SteamService.kt (1)
540-559:⚠️ Potential issue | 🟠 MajorDon't collapse depot grants to one package.
getLicensedDepotIds()andbuildLicensedDepotMap()still key off the singleSteamApp.packageId, but the package-sync path later rewrites that field every time another owned package references the app. If an app is granted by multiple packages, the last writer wins and depots from the other packages disappear from both download filtering and library-size calculations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/app/gamenative/service/SteamService.kt`:
- Around line 763-765: The code currently computes has64Bit and hasNonDeckWin
from the full eligibleDepots set, which can cancel out depots when one filter
influences the other; instead, first filter by architecture (compute the
arch-eligible set using eligibleDepots and then set has64Bit based on that set),
and only after that compute the Deck preference (hasNonDeckWin) from the already
arch-filtered collection; apply the same change to the DLC branch where you
compute arch eligibility for ownedDlc/licensedDepots so that arch choice is
applied before evaluating steamDeck/non-Deck compatibility (references:
eligibleDepots, has64Bit, hasNonDeckWin, ownedDlc, licensedDepots,
containerLanguage, appInfo.depots).
In `@app/src/main/java/app/gamenative/ui/model/LibraryViewModel.kt`:
- Around line 430-439: The totalSizeBytes computation is summing all licensed
depots and can overcount mutually-exclusive/variant depots; instead, use the
same depot-eligibility logic as SteamService.getDownloadableDepots to pick the
actual downloadable subset. Replace the current filter { licensedDepots == null
|| it.depotId in licensedDepots } and subsequent sum with a call to
SteamService.getDownloadableDepots(item, licensedDepots) (or the appropriate
overload) to produce the list of depots to sum, then compute sizes from those
depots using the existing manifest size resolution logic so the displayed size
matches actual downloadable size.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5bc025a8-194b-4783-9285-12685008d661
📒 Files selected for processing (6)
app/src/main/java/app/gamenative/data/DepotInfo.ktapp/src/main/java/app/gamenative/db/dao/SteamLicenseDao.ktapp/src/main/java/app/gamenative/service/SteamService.ktapp/src/main/java/app/gamenative/ui/model/LibraryViewModel.ktapp/src/main/java/app/gamenative/utils/KeyValueUtils.ktapp/src/test/java/app/gamenative/service/SdCardDetectionTest.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- app/src/main/java/app/gamenative/utils/KeyValueUtils.kt
- app/src/main/java/app/gamenative/db/dao/SteamLicenseDao.kt
- app/src/main/java/app/gamenative/data/DepotInfo.kt
f9cc6f9 to
c28dbbd
Compare
|
Note that I could only test Blue Prince. I believe that @utkarshdalal has RE4 for testing, though, so that should be verified in a local test pre-merge, as well. There was another game reporting TB(!) of storage necessary, we should check that if possible. |
c28dbbd to
66fa0b1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/app/gamenative/service/SteamService.kt`:
- Around line 807-814: The DLC filtering incorrectly reuses the base app's
has64Bit; compute a DLC-specific flag (e.g., val dlcHas64Bit = dlcEligible.any {
it.is64Bit } or similar based on the eligibleDepot model) after calling
eligibleDepots(dlcApp.depots, ...) and pass dlcHas64Bit into
filterForDownloadableDepots instead of the outer has64Bit; keep existing
dlcHasNonDeckWin and dlcLicensedDepots logic (from getLicensedDepotIds and
eligibleDepots) and replace the has64Bit argument in the
dlcApp.depots.asSequence().filter { ... filterForDownloadableDepots(...) } call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 682c2110-8bd6-4543-8fb6-7039222d89e6
📒 Files selected for processing (6)
app/src/main/java/app/gamenative/data/DepotInfo.ktapp/src/main/java/app/gamenative/db/dao/SteamLicenseDao.ktapp/src/main/java/app/gamenative/service/SteamService.ktapp/src/main/java/app/gamenative/ui/model/LibraryViewModel.ktapp/src/main/java/app/gamenative/utils/KeyValueUtils.ktapp/src/test/java/app/gamenative/service/SdCardDetectionTest.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- app/src/main/java/app/gamenative/db/dao/SteamLicenseDao.kt
- app/src/main/java/app/gamenative/data/DepotInfo.kt
- app/src/test/java/app/gamenative/service/SdCardDetectionTest.kt
| private fun eligibleDepots( | ||
| depots: Map<Int, DepotInfo>, | ||
| preferredLanguage: String, | ||
| ownedDlc: Map<Int, DepotInfo>?, | ||
| licensedDepotIds: Set<Int>?, | ||
| ): Collection<DepotInfo> = depots.values.filter { depot -> | ||
| if (depot.manifests.isEmpty() && depot.encryptedManifests.isNotEmpty()) return@filter false | ||
| if (depot.manifests.isEmpty() && !depot.sharedInstall) return@filter false | ||
| if (depot.manifests.isNotEmpty() && depot.manifests.values.all { it.size == 0L && it.download == 0L }) return@filter false | ||
| if (!depot.isWindowsCompatible) return@filter false | ||
| if (depot.dlcAppId != INVALID_APP_ID && ownedDlc != null && !ownedDlc.containsKey(depot.depotId)) return@filter false | ||
| if (depot.language.isNotEmpty() && depot.language != preferredLanguage) return@filter false | ||
| if (licensedDepotIds != null && depot.depotId !in licensedDepotIds) return@filter false | ||
| true | ||
| } |
There was a problem hiding this comment.
Are these the same conditions as what's in filterForDownloadableDepots?
There was a problem hiding this comment.
I kept them separate to avoid a chicken/egg issue, but on second thought, I think I can combine them and use flags to get the appropriate behavior for callers.
| return appInfo.depots.asSequence() | ||
| .filter { (depotId, depot) -> | ||
| return@filter filterForDownloadableDepots(depot, has64Bit, containerLanguage, ownedDlc) | ||
| return@filter filterForDownloadableDepots(depot, has64Bit, hasNonDeckWin, containerLanguage, ownedDlc, licensedDepots) |
There was a problem hiding this comment.
Not seeing why this change is necessary since we seem to be doing the identical filtering in filterForDownloadableDepots?
| .asSequence() | ||
| .filter { (depotId, depot) -> | ||
| return@filter filterForDownloadableDepots(depot, has64Bit, preferredLanguage, ownedDlc) | ||
| return@filter filterForDownloadableDepots(depot, has64Bit, hasNonDeckWin, preferredLanguage, ownedDlc, licensedDepots) |
| .asSequence() | ||
| .filter { (depotId, depot) -> | ||
| return@filter filterForDownloadableDepots(depot, has64Bit, preferredLanguage, null) | ||
| return@filter filterForDownloadableDepots(depot, has64Bit, dlcHasNonDeckWin, preferredLanguage, null, dlcLicensedDepots) |
| // mirror SteamService depot filtering: license, OS, arch, deck, base-game only | ||
| val licensedDepots = licensedDepotMap[item.id] | ||
| val eligible = item.depots.values.filter { d -> | ||
| d.isWindowsCompatible && | ||
| d.dlcAppId == SteamService.INVALID_APP_ID && | ||
| (licensedDepots == null || d.depotId in licensedDepots) && | ||
| d.manifests.isNotEmpty() | ||
| } | ||
| val has64Bit = eligible.any { it.osArch == OSArch.Arch64 } | ||
| val hasNonDeckWin = eligible.any { !it.steamDeck && it.isWindowsCompatible } | ||
| val totalSizeBytes = eligible | ||
| .filter { d -> | ||
| (d.osArch == OSArch.Arch64 || d.osArch == OSArch.Unknown || (!has64Bit && d.osArch == OSArch.Arch32)) && | ||
| !(d.steamDeck && hasNonDeckWin) | ||
| } | ||
| .sumOf { depot -> | ||
| depot.manifests["public"]?.size ?: depot.manifests.values.firstOrNull()?.size ?: 0L |
There was a problem hiding this comment.
seems like repetition of the eligibleDepots logic? can we centralize it?
There was a problem hiding this comment.
yes, I'll push a cleanup
| language = currentDepot["config"]["language"].value.orEmpty(), | ||
| realm = currentDepot["config"]["realm"].value.orEmpty(), | ||
| optionalDlcId = currentDepot["config"]["optionaldlc"].asInteger(INVALID_APP_ID), | ||
| steamDeck = currentDepot["config"]["steamdeck"].asBoolean(), |
There was a problem hiding this comment.
make sure to add a default here, as this won't get populated until someone logs out of steam and back in. Will break for users if there's a value expected here.
There was a problem hiding this comment.
this default to false if there's no value, but I'll add the explicit default.
66fa0b1 to
70be10c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/app/gamenative/service/SteamService.kt`:
- Around line 777-778: The code is using the base app's licensedDepots for all
depots but appInfo.depots can include inline DLC depots that are licensed via
their DLC package; to fix, compute a per-depot license union before calling
resolveDownloadableDepots: iterate appInfo.depots and for any depot with
dlcAppId != INVALID_APP_ID call getLicensedDepotIds(dlcAppId) and union those
results with the base licensedDepots (or build a map of depotId -> licensed),
then pass that union (or the map) into resolveDownloadableDepots (or adjust
resolveDownloadableDepots signature to accept per-depot licenses); ensure
getMainAppDepots/getDownloadableDepots and downloadApp use the same per-depot
license union logic so inline DLC depots granted by ownedDlc are not filtered
out.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f3b85615-bd06-4b72-a7ed-8dbaeea78950
📒 Files selected for processing (6)
app/src/main/java/app/gamenative/data/DepotInfo.ktapp/src/main/java/app/gamenative/db/dao/SteamLicenseDao.ktapp/src/main/java/app/gamenative/service/SteamService.ktapp/src/main/java/app/gamenative/ui/model/LibraryViewModel.ktapp/src/main/java/app/gamenative/utils/KeyValueUtils.ktapp/src/test/java/app/gamenative/service/SdCardDetectionTest.kt
🚧 Files skipped from review as they are similar to previous changes (5)
- app/src/main/java/app/gamenative/utils/KeyValueUtils.kt
- app/src/main/java/app/gamenative/db/dao/SteamLicenseDao.kt
- app/src/main/java/app/gamenative/data/DepotInfo.kt
- app/src/main/java/app/gamenative/ui/model/LibraryViewModel.kt
- app/src/test/java/app/gamenative/service/SdCardDetectionTest.kt
70be10c to
36ff728
Compare
|
@utkarshdalal addressed concerns, I hope. Let me know if there's anything else to address. |
36ff728 to
f5dc9fe
Compare
| @Transaction | ||
| suspend fun findLicenses(packageIds: List<Int>): List<SteamLicense> { | ||
| if (packageIds.isEmpty()) return emptyList() | ||
| val out = mutableListOf<SteamLicense>() | ||
| for (i in packageIds.indices step SQLITE_MAX_VARS) { | ||
| val end = min(i + SQLITE_MAX_VARS, packageIds.size) | ||
| out += _findLicenses(packageIds.subList(i, end)) | ||
| } | ||
| return out | ||
| } | ||
|
|
There was a problem hiding this comment.
So what I understand here is this is essentially iterating through here and getting all licences based on packages.
Would it be a bit easier on the eyes if this was a full SQL query with joins instead?
Maybe it's just the general naming and shorthands that's throwing off readability for me here.
There was a problem hiding this comment.
We have a hard limit on bind variables (the WHERE id IN (:list) clause creates these), so we have to do this in batches to avoid potential crashes (we could have a special branch for users with < 999 games, but... maybe overengineering IMO). I can improve the variable names to see if it's clearer, and add a comment explaining why.
| * Common Filter for downloadable depots | ||
| * Common filter for downloadable depots. | ||
| * | ||
| * [has64Bit] and [hasNonDeckWindows] are exclusion flags, not selection |
There was a problem hiding this comment.
If that's the case, could we not adjust to "exclude64Bit" and "excludeNonDeckWindows" as params? That way it's easier from an interfacing perspective to understand how these are used?
There was a problem hiding this comment.
adjusting to prefer64Bit and preferNonDeckWindows which I think works, too?
| assertTrue(SteamService.filterForDownloadableDepots(d, true, false, "english", null)) | ||
| } | ||
|
|
||
| // -- licensedDepotIds filtering -- | ||
|
|
||
| @Test | ||
| fun `depot in licensed set passes`() { | ||
| val d = depot(depotId = 100, manifests = mapOf("public" to manifest())) | ||
| assertTrue(SteamService.filterForDownloadableDepots(d, true, false, "english", null, setOf(100, 200))) | ||
| } | ||
|
|
||
| @Test | ||
| fun `depot not in licensed set is rejected`() { | ||
| val d = depot(depotId = 100, manifests = mapOf("public" to manifest())) | ||
| assertFalse(SteamService.filterForDownloadableDepots(d, true, false, "english", null, setOf(200, 300))) | ||
| } | ||
|
|
||
| @Test | ||
| fun `null licensedDepotIds skips license check`() { | ||
| val d = depot(depotId = 100, manifests = mapOf("public" to manifest())) | ||
| assertTrue(SteamService.filterForDownloadableDepots(d, true, false, "english", null, null)) | ||
| } | ||
|
|
||
| // -- Steam Deck depot filtering -- | ||
|
|
||
| @Test | ||
| fun `deck depot rejected when non-deck windows depot exists`() { | ||
| val d = depot(manifests = mapOf("public" to manifest()), steamDeck = true) | ||
| assertFalse(SteamService.filterForDownloadableDepots(d, true, true, "english", null)) | ||
| } | ||
|
|
||
| @Test | ||
| fun `deck depot passes when no non-deck windows depot exists`() { | ||
| val d = depot(manifests = mapOf("public" to manifest()), steamDeck = true) | ||
| assertTrue(SteamService.filterForDownloadableDepots(d, true, false, "english", null)) | ||
| } | ||
|
|
||
| @Test | ||
| fun `non-deck depot passes regardless of hasNonDeckWindows`() { | ||
| val d = depot(manifests = mapOf("public" to manifest()), steamDeck = false) | ||
| assertTrue(SteamService.filterForDownloadableDepots(d, true, true, "english", null)) |
There was a problem hiding this comment.
Oddly, these tests are in the wrong test file to me.
in my opinion they should be in a SteamServiceTest file rather than sdcarddetection
There was a problem hiding this comment.
lol. will fix, this was a rebasing error.
…sed region depots games like Black Desert (582660) and RE4 (2050650) ship multiple region/language depots with no distinguishing config metadata. without checking the user's license, all depots pass the OS/arch/language filters, inflating download size 5-10x. adds getLicensedDepotIds() to cross-reference depot IDs against the package's depotIds list from PICS. applied in both the download filter and library list size calculation. also excludes Steam Deck-specific depots (steamdeck=1) when a regular Windows depot exists, since we run on Android not Deck.
single DB query for all package IDs instead of 2×N per-game queries. adds findLicenses() batch DAO query with chunking, buildLicensedDepotMap() bulk helper on SteamService.
- eligibleDepots now delegates to filterForDownloadableDepots with has64Bit=false, hasNonDeckWindows=false — these are exclusion flags, so false is permissive (all archs/deck states pass through), giving us the pre-filter pool without duplicating logic - LibraryViewModel reuses eligibleDepots + filterForDownloadableDepots instead of inline copy - steamDeck asBoolean(false) — already the default, made explicit. DepotInfo.steamDeck defaults to false so existing users without the field are unaffected (non-deck, which is correct)
…g, split test file
f5dc9fe to
1c49c30
Compare
| val results = mutableListOf<SteamLicense>() | ||
| for (chunkStart in packageIds.indices step SQLITE_MAX_VARS) { | ||
| val chunkEnd = min(chunkStart + SQLITE_MAX_VARS, packageIds.size) | ||
| val chunkResult = _findStaleLicences(packageIds.subList(chunkStart, chunkEnd)) | ||
| if (results.isEmpty()) { | ||
| results += chunkResult | ||
| } else { | ||
| out.retainAll(chunkResult) // Intersect to only keep entries in both lists | ||
| results.retainAll(chunkResult) | ||
| } | ||
| } | ||
| return out.distinct() | ||
| return results.distinct() | ||
| } | ||
|
|
||
| @Transaction | ||
| suspend fun deleteStaleLicenses(packageIds: List<Int>) { | ||
| for (i in packageIds.indices step SQLITE_MAX_VARS) { | ||
| val end = min(i + SQLITE_MAX_VARS, packageIds.size) | ||
| _deleteStaleLicenses(packageIds.subList(i, end)) | ||
| for (chunkStart in packageIds.indices step SQLITE_MAX_VARS) { | ||
| val chunkEnd = min(chunkStart + SQLITE_MAX_VARS, packageIds.size) | ||
| _deleteStaleLicenses(packageIds.subList(chunkStart, chunkEnd)) |
There was a problem hiding this comment.
as far as i can see these are just renamed variables right?
There was a problem hiding this comment.
correct, @phobos665 suggested that the names were too opaque
| * Common filter for downloadable depots. | ||
| * | ||
| * [prefer64Bit] and [preferNonDeckWindows] are preference flags: | ||
| * `true` filters OUT the lesser variant (32-bit / Deck-only), while | ||
| * `false` is permissive and lets all architectures or Deck states through. | ||
| * [eligibleDepots] passes both as `false` to skip preference checks | ||
| * when computing the flags themselves. |
There was a problem hiding this comment.
I think we can hardcode prefer64Bit and preferNonDeckWindows to true, there's no situation in which they'll be used tbh
There was a problem hiding this comment.
Nope, because false, false is used deliberately in eligibleDepots():
fun eligibleDepots(...): Collection<DepotInfo> = depots.values.filter { depot ->
filterForDownloadableDepots(depot, prefer64Bit = false, preferNonDeckWindows = false, ...)
}
This is the first pass: it computes the pool of depots that pass all checks except arch/Deck preference. From that pool, we then derive whether 64-bit or non-Deck depots exist. If we hardcoded both to true, this first pass would already filter out 32-bit and Deck depots, so we'd never know if those were the only options available.
For instance, a game ships only 32-bit depots. With a hardcoded prefer64Bit = true, eligibleDepots() would return nothing (with all 32-bit depots filtered out), then has64Bit would be false, but we'd have already lost the depot list.
The two-pass design exists exactly because these flags depend on the result of the first pass.
There was a problem hiding this comment.
Sorry, I meant hardcode them both to false
| val archOk = when (depot.osArch) { | ||
| OSArch.Arch64, OSArch.Unknown -> true | ||
| OSArch.Arch32 -> !has64Bit | ||
| OSArch.Arch32 -> !prefer64Bit |
There was a problem hiding this comment.
this would be a problem - has64Bit was added because some games downloaded both 64 bit and 32 bit depots, which would break the game. axyz was an example I believe.
There was a problem hiding this comment.
This was just a variable rename: the exclusion logic remains the same -- if 64-bit depots exist, 32-bit depots are excluded.
| // 6. Package grants this depot — prevents grabbing region depots the user has no license for. | ||
| // Skip for DLC depots: they're licensed via their own package, already validated by check 4. | ||
| if (depot.dlcAppId == INVALID_APP_ID && licensedDepotIds != null && depot.depotId !in licensedDepotIds) | ||
| return false |
There was a problem hiding this comment.
is this change necessary? from what i understand we just need to filter out depot.steamDeck, right? or have I misunderstood? Because I was pretty sure we were filtering out depots that we didn't have license to already.
There was a problem hiding this comment.
This is the core fix! The license filtering is the whole point. The steamDeck filtering was a secondary addition, because we were erroneously also including those depots if a valid, non-steamDeck Windows depot was available.
Pre-PR behavior, there was no depot-level license check. The app checked if you owned the game (app-level license), but not which specific depots your package grants. That's the crux: different packages for the same game can grant different depot sets! So a Japanese regional package might include region-specific depots that a US package doesn't, and vice versa (this is the RE4 case, for example). Without this check, the downloader grabs all depots that passed the OS/arch/language filters, including ones the user's package doesn't actually allow, which would then (probably) fail during download or (worse) produce broken installs.
The existing check nr. 4 (DLC ownership) only validates DLC apps, not individual depot grants within a package.
| if (depot.dlcAppId == INVALID_APP_ID && licensedDepotIds != null && depot.depotId !in licensedDepotIds) | ||
| return false | ||
| // 7. Prefer non-Steam-Deck depot when both exist (we're on Android, not Deck) | ||
| if (depot.steamDeck && preferNonDeckWindows) |
There was a problem hiding this comment.
maybe instead of filtering out steamDeck always, we do it if there are non steam deck depots?
I can imagine a case where a game only ships steam deck depots.
There was a problem hiding this comment.
Yeah, that's what this does: preferNonDeckWindows is only true when non-Deck Windows depots exist in the eligible pool:
val hasNonDeckWin = eligible.any { !it.steamDeck && it.isWindowsCompatible }
If a game only ships Deck depots, hasNonDeckWin is false, so preferNonDeckWindows is false, and the check is skipped, and Deck depots are permitted. It's the same two-pass filter as the 64-bit logic.
| language = currentDepot["config"]["language"].value.orEmpty(), | ||
| realm = currentDepot["config"]["realm"].value.orEmpty(), | ||
| optionalDlcId = currentDepot["config"]["optionaldlc"].asInteger(INVALID_APP_ID), | ||
| steamDeck = currentDepot["config"]["steamdeck"].asBoolean(false), |
There was a problem hiding this comment.
should be noted that this will be only populated once someone signs out of steam and back in (I believe)
There was a problem hiding this comment.
The PICS data is fetched every session, no? I think only the license data would require a new login. But we can note this either in the code or in the release note.
| val map = appInfo.depots | ||
| .asSequence() | ||
| .filter { (depotId, depot) -> | ||
| return@filter filterForDownloadableDepots(depot, has64Bit, preferredLanguage, ownedDlc) |
There was a problem hiding this comment.
confirming that the logic change here is required for the change
There was a problem hiding this comment.
This was changed to address a valid CR finding, IIRC. The old code computed has64Bit from the unfiltered depot map, then used that to filter. The new code computes it from the eligible pool (after all license/language/manifest/DLC checks).
This would matter if an unlicensed 64-bit depot exists. The old code would see it, set has64Bit = true, and suppress all 32-bit depots, potentially leaving nothing downloadable. The new code only considers depots the user actually has access to when deciding preferences. So this should be more accurate.
3 participants
Summary
SteamLicense.depotIds) to exclude region depots the user has no access tosteamdeck=1config key to prefer regular Windows depots over Deck-specific buildsTest plan
Fixes #928
Summary by cubic
Centralized depot filtering to include only licensed base‑game depots and prefer non‑Deck Windows builds. Fixes #928, cuts inflated download sizes, and speeds up the library view.
Bug Fixes
steamdeck=1and drop Deck depots when a non‑Deck Windows depot exists (base game and DLC).Performance
findLicenses) and a sharedbuildLicensedDepotMap().Written for commit 1c49c30. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements
Tests