Use container language for GOG downloads#627
Use container language for GOG downloads#627utkarshdalal merged 5 commits intoutkarshdalal:masterfrom
Conversation
📝 WalkthroughWalkthroughLanguage resolution for GOG downloads was refactored: a hardcoded default was removed and replaced by a container-language → ordered GOG code mapping with English fallbacks. The manifest parser now returns (filteredDepots, effectiveLang) and effectiveLang is propagated through UI, service, download manager, and manifest saving. Changes
Sequence DiagramsequenceDiagram
participant UI as GOGAppScreen
participant Service as GOGService
participant Manager as GOGDownloadManager
participant Parser as GOGManifestParser
participant Constants as GOGConstants
UI->>Service: downloadGame(context, gameId, path, containerLanguage)
Service->>Manager: downloadGame(..., containerLanguage)
Manager->>Constants: containerLanguageToGogCodes(containerLanguage)
Constants-->>Manager: ["code1","code2",...,"english"] (ordered fallbacks)
Manager->>Parser: filterDepotsByLanguage(manifest, containerLanguage)
Parser->>Constants: containerLanguageToGogCodes(containerLanguage)
Constants-->>Parser: ["code1","code2",...,"english"]
Parser->>Parser: try codes in order (include '*' depots)
Parser-->>Manager: Pair(languageDepots, effectiveLang)
Manager->>Manager: apply bitness/ownership filters, skip existing files
Manager->>Manager: saveManifestToGameDir(manifest, effectiveLang)
Manager-->>Service: Result<Unit>
Service-->>UI: Result<Unit>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
6c93178 to
0dae3ba
Compare
0dae3ba to
ebc9d08
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/src/main/java/app/gamenative/service/gog/GOGDownloadManager.kt (1)
671-672: RedundantfilterDepotsByLanguagecall in Gen 1 path — computeeffectiveLangdirectly.The Gen 1 flow deliberately skips language-based depot filtering (lines 523–524), yet
filterDepotsByLanguageis called here solely to extracteffectiveLang. This re-runs the full filtering logic unnecessarily. Since you only need the effective language code, derive it directly from the mapping:♻️ Suggested fix
- val (_, effectiveLang) = parser.filterDepotsByLanguage(gameManifest, language) - saveManifestToGameDir(installPath, gameManifest, selectedBuild.buildId, selectedBuild.versionName, effectiveLang) + val effectiveLang = GOGConstants.containerLanguageToGogCodes(language).firstOrNull() ?: "en" + saveManifestToGameDir(installPath, gameManifest, selectedBuild.buildId, selectedBuild.versionName, effectiveLang)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/service/gog/GOGDownloadManager.kt` around lines 671 - 672, The call to filterDepotsByLanguage is redundant in the Gen 1 path because we only need the effectiveLang value; remove the extra call and compute effectiveLang directly from the manifest-language mapping instead of re-running filtering. Locate where filterDepotsByLanguage(gameManifest, language) is invoked and used to extract effectiveLang, and replace that with the logic that reads the resolved language code from the same mapping used earlier (e.g., the depots/language map on gameManifest or the previously-determined language fallback), then pass that effectiveLang into saveManifestToGameDir(selectedBuild.buildId, selectedBuild.versionName, effectiveLang) while leaving gameManifest unchanged. Ensure you keep the same variable name effectiveLang so downstream calls (saveManifestToGameDir) are unaffected.app/src/test/java/app/gamenative/service/gog/api/GOGManifestParserTest.kt (1)
128-164: Consider adding test coverage for star (*) depots and the "no match at all" fallback.The current tests cover the primary and English-fallback paths well. Two additional scenarios would strengthen coverage:
- Star depots: Verify that depots with language
"*"are included alongside language-matched depots (the(matched + starDepots).distinct()path in the parser).- No match fallback: When neither the requested language nor English has matching depots, all depots should be returned.
🤖 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/gog/api/GOGManifestParserTest.kt` around lines 128 - 164, Add two unit tests for parser.filterDepotsByLanguage: one that verifies star (`"*"`) depots are included with language-matched depots (e.g., create a manifest with a matching depot, a "*" depot and a non-matching depot; assert returned depots contain both the matched and the "*" depot and are distinct), and another that verifies the "no match at all" fallback returns all depots (e.g., request a language and ensure neither it nor English matches any depot, then assert all manifest.depots are returned and effectiveLang equals the configured fallback in GOGConstants). Reference parser.filterDepotsByLanguage, GOGManifestMeta, createTestDepot and GOGConstants in the tests.
🤖 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/gog/GOGConstants.kt`:
- Around line 44-45: The KDoc for GOG_FALLBACK_DOWNLOAD_LANGUAGE is incorrect:
it claims the value is a short depot language code (e.g. "en") but the constant
value "english" is a container language name; update the comment to reflect that
this constant represents a container/manifest language name (full name like
"english") used as a fallback when depot short codes are unavailable, and remove
the example of a short code; keep the constant name
GOG_FALLBACK_DOWNLOAD_LANGUAGE unchanged.
- Around line 51-81: Remove the two non-standard codes from
CONTAINER_LANGUAGE_TO_GOG_CODES: delete "bl" from the "bulgarian" value and
delete "el-GK" from the "greek" value; then ensure "bulgarian" only contains the
official codes listOf("bg-BG", "bg") and "greek" contains the official code
listOf("el-GR") so the mapping uses only GOG-documented language codes.
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/service/gog/GOGDownloadManager.kt`:
- Around line 671-672: The call to filterDepotsByLanguage is redundant in the
Gen 1 path because we only need the effectiveLang value; remove the extra call
and compute effectiveLang directly from the manifest-language mapping instead of
re-running filtering. Locate where filterDepotsByLanguage(gameManifest,
language) is invoked and used to extract effectiveLang, and replace that with
the logic that reads the resolved language code from the same mapping used
earlier (e.g., the depots/language map on gameManifest or the
previously-determined language fallback), then pass that effectiveLang into
saveManifestToGameDir(selectedBuild.buildId, selectedBuild.versionName,
effectiveLang) while leaving gameManifest unchanged. Ensure you keep the same
variable name effectiveLang so downstream calls (saveManifestToGameDir) are
unaffected.
In `@app/src/test/java/app/gamenative/service/gog/api/GOGManifestParserTest.kt`:
- Around line 128-164: Add two unit tests for parser.filterDepotsByLanguage: one
that verifies star (`"*"`) depots are included with language-matched depots
(e.g., create a manifest with a matching depot, a "*" depot and a non-matching
depot; assert returned depots contain both the matched and the "*" depot and are
distinct), and another that verifies the "no match at all" fallback returns all
depots (e.g., request a language and ensure neither it nor English matches any
depot, then assert all manifest.depots are returned and effectiveLang equals the
configured fallback in GOGConstants). Reference parser.filterDepotsByLanguage,
GOGManifestMeta, createTestDepot and GOGConstants in the tests.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/src/main/java/app/gamenative/service/gog/GOGConstants.ktapp/src/main/java/app/gamenative/service/gog/GOGDownloadManager.ktapp/src/main/java/app/gamenative/service/gog/GOGService.ktapp/src/main/java/app/gamenative/service/gog/api/GOGDataModels.ktapp/src/main/java/app/gamenative/service/gog/api/GOGManifestParser.ktapp/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.ktapp/src/test/java/app/gamenative/service/gog/api/GOGManifestParserTest.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/app/gamenative/service/gog/api/GOGDataModels.kt
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/gog/GOGDownloadManager.kt`:
- Around line 586-587: The calculated totalSize currently adds supportFiles
sizes even when supportDir is null (support files are skipped later), causing
incorrect expected bytes; change the totalSize computation in GOGDownloadManager
to only include supportFiles.sumOf { it.file.size } when supportDir != null (or
when support files will actually be downloaded) before calling
downloadInfo.setTotalExpectedBytes, adjusting any related logic in the same
method that uses totalSize or downloadInfo so the progress/expected bytes match
the actual download path.
- Around line 1415-1418: The current fileExistsWithCorrectSize(outputFile: File,
expectedSize: Long) only checks length and can falsely accept corrupted files;
modify it to optionally verify a content hash (e.g., SHA-256) when an expected
hash is available: change the signature to accept an optional expectedHash:
String? (or overload) and return false if the file size mismatches or, when
expectedHash is non-null, compute the file's checksum (streaming, not reading
whole file into memory) and compare to expectedHash. Update callers in the Gen 1
and Gen 2 incremental paths to pass the known expected checksum from metadata
when present, and fall back to size-only when no hash is available. Ensure
hashing uses a stable algorithm (SHA-256) and short-circuits on size mismatch
before hashing for performance.
In
`@app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt`:
- Around line 433-437: The current handler launches a download on language
change unconditionally and uses
GOGConstants.getGameInstallPath(libraryItem.name), which can start downloads for
uninstalled games or write to the wrong location; update the block that checks
previousLanguage != config.language to first verify the game is actually
installed (e.g., an isInstalled flag or equivalent on libraryItem) and that a
persisted install path (e.g., libraryItem.installPath) exists and is non-empty,
then launch the CoroutineScope(Dispatchers.IO).launch to call
GOGService.downloadGame using the persisted install path
(libraryItem.installPath) and only when both guards pass; otherwise skip the
download and optionally log the skipped condition.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/main/java/app/gamenative/service/gog/GOGDownloadManager.ktapp/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt
app/src/main/java/app/gamenative/service/gog/GOGDownloadManager.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/app/gamenative/service/gog/GOGDownloadManager.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt
Show resolved
Hide resolved
Falls back to english
…, and to only download files that don't exist.
f888e6e to
267ba9f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
app/src/main/java/app/gamenative/service/gog/GOGConstants.kt (2)
44-45: Stale KDoc: value is a container language name, not a short depot code.The KDoc reads "short code as used in manifest depots, e.g. 'en'" but the constant's value
"english"is a container language name.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/service/gog/GOGConstants.kt` around lines 44 - 45, The KDoc for GOG_FALLBACK_DOWNLOAD_LANGUAGE is incorrect—its value "english" is a container language name, not a short depot code—so update the documentation to describe that this constant holds a container language name (e.g., "english") used for GOG download fallback, rather than claiming it is a short manifest depot code like "en"; locate the constant GOG_FALLBACK_DOWNLOAD_LANGUAGE and adjust its KDoc accordingly.
53-63: Non-standard GOG language codes"bl"and"el-GK"remain in the map.
"bl"is a LGOGDownloader CLI alias, not an official GOG code;"el-GK"violates BCP 47 (Greece =GR, notGK). Official codes are"bg-BG"and"el-GR".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/service/gog/GOGConstants.kt` around lines 53 - 63, In the GOGConstants language mapping (inside GOGConstants.kt), remove the non-standard alias "bl" from the "bulgarian" entry and replace the invalid "el-GK" in the "greek" entry with the correct BCP‑47 region code "el-GR" (i.e., update the list for "bulgarian" to only include "bg-BG" and "bg", and update the list for "greek" to include "el-GR" instead of "el-GK").
🧹 Nitpick comments (3)
app/src/main/java/app/gamenative/service/gog/GOGConstants.kt (1)
88-89:getValueonGOG_FALLBACK_DOWNLOAD_LANGUAGEwill throw if the key is ever removed from the map.
CONTAINER_LANGUAGE_TO_GOG_CODES.getValue(GOG_FALLBACK_DOWNLOAD_LANGUAGE)throwsNoSuchElementExceptionif"english"is ever removed from the map (e.g., during a refactor). The same.getValuecall is also mirrored inGOGManifestParserline 73. Consider using a hardcoded constant list as the ultimate fallback to break this implicit dependency:🛡️ Suggested hardening
+ private val ENGLISH_FALLBACK_CODES = listOf("en-US", "en") fun containerLanguageToGogCodes(containerLanguage: String): List<String> = - CONTAINER_LANGUAGE_TO_GOG_CODES[containerLanguage.lowercase()] ?: CONTAINER_LANGUAGE_TO_GOG_CODES.getValue(GOG_FALLBACK_DOWNLOAD_LANGUAGE) + CONTAINER_LANGUAGE_TO_GOG_CODES[containerLanguage.lowercase()] + ?: CONTAINER_LANGUAGE_TO_GOG_CODES[GOG_FALLBACK_DOWNLOAD_LANGUAGE] + ?: ENGLISH_FALLBACK_CODES🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/service/gog/GOGConstants.kt` around lines 88 - 89, The current containerLanguageToGogCodes implementation uses CONTAINER_LANGUAGE_TO_GOG_CODES.getValue(GOG_FALLBACK_DOWNLOAD_LANGUAGE) which will throw if that key is ever removed; change both containerLanguageToGogCodes and the similar lookup in GOGManifestParser to fall back to a hardcoded constant List<String> (e.g., DEFAULT_GOG_FALLBACK_CODES) instead of calling getValue on the map, or use getOrDefault with that constant list, ensuring you reference CONTAINER_LANGUAGE_TO_GOG_CODES and GOG_FALLBACK_DOWNLOAD_LANGUAGE but eliminate the direct getValue dependency.app/src/test/java/app/gamenative/service/gog/api/GOGManifestParserTest.kt (1)
128-182: Consider adding test coverage for "*"-depot merging and the last-resort fallback path.The refactored
filterDepotsByLanguagehas two untested branches:
- Star-depot merge (parser line 79/88):
(matched + starDepots).distinct()— no test verifies that a universal"*"depot is included alongside a language-matched depot.- Last-resort path (parser line 96): when neither the requested language nor any English fallback has depots, the function returns
manifest.depotsin full — no test covers this scenario.Would you like me to draft the two missing test cases?
🤖 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/gog/api/GOGManifestParserTest.kt` around lines 128 - 182, Add two unit tests for parser.filterDepotsByLanguage: one that verifies star-depot merging by creating a manifest with a language-matching depot (e.g., languages=["en"]) plus a universal depot (languages=["*"]) and asserting the returned depots include both (distinct) and effectiveLang is set; and another that verifies the last-resort path by creating a manifest whose depots contain neither the requested language nor any English fallback (use createTestDepot to make e.g. ["fr"] only, call filterDepotsByLanguage with a language like "greek") and assert the function returns the full manifest.depots and an effectiveLang from GOGConstants.GOG_FALLBACK_DOWNLOAD_LANGUAGE as appropriate.app/src/main/java/app/gamenative/service/gog/api/GOGManifestParser.kt (1)
93-97: Last-resort path returns all depots (including language-restricted ones) without a warning.When no depot matches any requested or English fallback code, the function silently returns the full
manifest.depotslist. For games that only contain"*"depots this is intentional, but for games whose depots all carry explicit language tags (e.g."de","fr") this would return content in unexpected languages. ATimber.w()would at least surface this to diagnostics:📝 Suggested improvement
- // No language-specific match: return all depots with first requested language as effective val effectiveLang = requestedCodes.firstOrNull() ?: "en" - Timber.tag(TAG).d("No language match for $containerLanguage, using all ${manifest.depots.size} depots with effective: $effectiveLang") + Timber.tag(TAG).w("No depot matched requested language '$containerLanguage' or English fallback — returning all ${manifest.depots.size} depots (may include unintended languages); effectiveLang=$effectiveLang") return manifest.depots to effectiveLangAdditionally,
requestedCodes.firstOrNull() ?: "en"—containerLanguageToGogCodesalways returns a non-empty list, so?: "en"is unreachable dead code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/service/gog/api/GOGManifestParser.kt` around lines 93 - 97, When no depot matches requested or "en" codes, add a diagnostic warning before returning manifest.depots: check manifest.depots for any language-restricted entries (e.g., any depot.language != "*") and if found call Timber.w(...) to warn that all depots are being returned and may be in unexpected languages; otherwise skip the warning for the intentional all-"*" case. Also remove the unreachable fallback in the effectiveLang assignment—use requestedCodes.first() (or requestedCodes[0]) instead of requestedCodes.firstOrNull() ?: "en" since containerLanguageToGogCodes always returns a non-empty list; keep the variable name effectiveLang and the returned pair manifest.depots to effectiveLang.
🤖 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/gog/GOGDownloadManager.kt`:
- Around line 693-694: The second call to parser.filterDepotsByLanguage just to
get effectiveLang is redundant for Gen1; either compute effectiveLang once and
pass it into downloadGameGen1 or extend downloadGameGen1’s signature to accept
effectiveLang (e.g., add parameter effectiveLang: String) and use
GOGConstants.containerLanguageToGogCodes(language).first() at the call site in
downloadGame to produce that value, then remove the extra
parser.filterDepotsByLanguage invocation and use the passed/locally-computed
effectiveLang when calling saveManifestToGameDir; this avoids rebuilding depot
lists and emitting needless logs.
- Around line 257-261: The incremental pre-download filter currently calls
fileExistsWithCorrectSize(outputFile, expectedSize, file.md5) which triggers
expensive MD5 hashing when file.md5 is present; change the predicate to only
perform a size-only existence check (e.g., outputFile.exists() &&
outputFile.length() == expectedSize) so the MD5 pass is skipped during the
pre-download filter and leave full hash verification to assembleFile where it
already runs.
---
Duplicate comments:
In `@app/src/main/java/app/gamenative/service/gog/GOGConstants.kt`:
- Around line 44-45: The KDoc for GOG_FALLBACK_DOWNLOAD_LANGUAGE is
incorrect—its value "english" is a container language name, not a short depot
code—so update the documentation to describe that this constant holds a
container language name (e.g., "english") used for GOG download fallback, rather
than claiming it is a short manifest depot code like "en"; locate the constant
GOG_FALLBACK_DOWNLOAD_LANGUAGE and adjust its KDoc accordingly.
- Around line 53-63: In the GOGConstants language mapping (inside
GOGConstants.kt), remove the non-standard alias "bl" from the "bulgarian" entry
and replace the invalid "el-GK" in the "greek" entry with the correct BCP‑47
region code "el-GR" (i.e., update the list for "bulgarian" to only include
"bg-BG" and "bg", and update the list for "greek" to include "el-GR" instead of
"el-GK").
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/service/gog/api/GOGManifestParser.kt`:
- Around line 93-97: When no depot matches requested or "en" codes, add a
diagnostic warning before returning manifest.depots: check manifest.depots for
any language-restricted entries (e.g., any depot.language != "*") and if found
call Timber.w(...) to warn that all depots are being returned and may be in
unexpected languages; otherwise skip the warning for the intentional all-"*"
case. Also remove the unreachable fallback in the effectiveLang assignment—use
requestedCodes.first() (or requestedCodes[0]) instead of
requestedCodes.firstOrNull() ?: "en" since containerLanguageToGogCodes always
returns a non-empty list; keep the variable name effectiveLang and the returned
pair manifest.depots to effectiveLang.
In `@app/src/main/java/app/gamenative/service/gog/GOGConstants.kt`:
- Around line 88-89: The current containerLanguageToGogCodes implementation uses
CONTAINER_LANGUAGE_TO_GOG_CODES.getValue(GOG_FALLBACK_DOWNLOAD_LANGUAGE) which
will throw if that key is ever removed; change both containerLanguageToGogCodes
and the similar lookup in GOGManifestParser to fall back to a hardcoded constant
List<String> (e.g., DEFAULT_GOG_FALLBACK_CODES) instead of calling getValue on
the map, or use getOrDefault with that constant list, ensuring you reference
CONTAINER_LANGUAGE_TO_GOG_CODES and GOG_FALLBACK_DOWNLOAD_LANGUAGE but eliminate
the direct getValue dependency.
In `@app/src/test/java/app/gamenative/service/gog/api/GOGManifestParserTest.kt`:
- Around line 128-182: Add two unit tests for parser.filterDepotsByLanguage: one
that verifies star-depot merging by creating a manifest with a language-matching
depot (e.g., languages=["en"]) plus a universal depot (languages=["*"]) and
asserting the returned depots include both (distinct) and effectiveLang is set;
and another that verifies the last-resort path by creating a manifest whose
depots contain neither the requested language nor any English fallback (use
createTestDepot to make e.g. ["fr"] only, call filterDepotsByLanguage with a
language like "greek") and assert the function returns the full manifest.depots
and an effectiveLang from GOGConstants.GOG_FALLBACK_DOWNLOAD_LANGUAGE as
appropriate.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/src/main/java/app/gamenative/service/gog/GOGConstants.ktapp/src/main/java/app/gamenative/service/gog/GOGDownloadManager.ktapp/src/main/java/app/gamenative/service/gog/GOGService.ktapp/src/main/java/app/gamenative/service/gog/api/GOGDataModels.ktapp/src/main/java/app/gamenative/service/gog/api/GOGManifestParser.ktapp/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.ktapp/src/test/java/app/gamenative/service/gog/api/GOGManifestParserTest.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt
Checks the container language, and checks if depots for it are available. If so, downloads them. Falls back to english when desired language is not available.
IMPORTANT, we discussed dynamically changing the container language to only show depots available for the game. While I feel like that's a good change, it would be a separate PR.
Summary by cubic
Selects GOG depots using the container language, trying mapped codes in order and falling back to English. Adds incremental downloads and auto‑redownloads language files when the container language changes, with guardrails.
New Features
Bug Fixes
Written for commit 267ba9f. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests