attribute-changer@11.40b: Fix languages handling#17583
Conversation
📝 WalkthroughWalkthroughReplaces the previous InnoSetup-dependent install branching and architecture-specific rename/removal steps with a custom extraction and parser. The new flow expands the Inno archive with Innounp, parses install_script.iss to relocate architecture-compatible payloads, collects language-specific Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bucket/attribute-changer.json`:
- Around line 20-21: The code currently hardcodes the default language ID by
writing '4' in Set-Content for "$persist_dir\\langCode"/"$dir\\langCode";
instead, parse install_script.iss to determine the actual default from $langs
and write that resolved ID. Change the branch guarded by Test-Path to call the
parser that reads install_script.iss (or reuse existing $langs parsing logic),
compute the default language index/ID from $langs, and pass that value to
Set-Content rather than the literal '4', ensuring the persisted langCode matches
the installer-provided ordering.
- Around line 132-142: The current invocation reuses the persisted langCode
without validating it, causing the helper to exit if the saved index is out of
range; before calling "& \"$dir\\change-ac-language.ps1\" (Get-Content
\"$persist_dir\\langCode\" -Raw)" read and parse the persisted value, validate
it against `$langs.Count` (and `$index` semantics used in the script), and if
it's stale/out-of-range, replace it with a safe fallback (e.g., 0 or a known
default index), write the corrected value back to `$persist_dir\langCode`, then
call `change-ac-language.ps1` with that validated index so
`messages.ini`/`ac.pdf` get recreated instead of aborting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fe09daab-a56b-4f55-b9e6-30f61122acf1
📒 Files selected for processing (1)
bucket/attribute-changer.json
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
bucket/attribute-changer.json (1)
138-143:⚠️ Potential issue | 🟡 MinorStale persisted langCode: partially addressed, but non-numeric values could still cause failures.
The past review suggested validating persisted langCode before invoking the script. The current approach handles out-of-range numeric values via the fallback on Line 124. However, if the persisted
langCodefile is corrupted or contains whitespace/non-numeric data,[int]$args[0]in the generated script will throw an error.Consider adding a numeric validation wrapper in
post_install, or use atry/catchin the generated script:Option 1: Validate in post_install
"post_install": [ "if (!(is_admin)) { error \"$app requires admin rights to $cmd\"; break }", - "& \"$dir\\change-ac-language.ps1\" (Get-Content \"$dir\\langCode\" -Raw)", + "$langCode = (Get-Content \"$dir\\langCode\" -Raw).Trim()", + "if ($langCode -notmatch '^\\d+$') { $langCode = '1' }", + "& \"$dir\\change-ac-language.ps1\" $langCode",Option 2: Handle in generated script (line 123)
-"`$index = [int]`$args[0] - 1", +"`$index = 0; try { `$index = [int]`$args[0] - 1 } catch { `$index = -1 }",,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bucket/attribute-changer.json` around lines 138 - 143, post_install currently pipes the raw langCode file into change-ac-language.ps1 which assumes a numeric value; add a validation step in post_install that reads and trims "$dir\\langCode", checks it is a valid integer (and within expected range), and if not replaces it with the fallback/default before invoking "& \"$dir\\change-ac-language.ps1\" ..."; alternatively (or additionally) harden change-ac-language.ps1 by wrapping the existing [int]$args[0] parse in a try/catch or use a safe parse (e.g., TryParse) to fall back to the default when the value is non-numeric or corrupted so the script never throws on bad input.
🧹 Nitpick comments (1)
bucket/attribute-changer.json (1)
25-27: Consider adding error handling for extraction failures.If
innounpfails to extractinstall_script.iss, the script continues and Line 27'sGet-Contentwill fail with a confusing "file not found" error. Consider checking the return value or file existence.Suggested improvement
"Expand-InnoArchive -Path \"$dir\\$fname\"", -"Invoke-ExternalCommand -FilePath (Get-HelperPath -Helper Innounp) -ArgumentList '-x', \"$dir\\$fname\", \"-d$dir\", 'install_script.iss' -LogPath \"$dir\\innounp-iss.log\" | Out-Null", +"$extracted = Invoke-ExternalCommand -FilePath (Get-HelperPath -Helper Innounp) -ArgumentList '-x', \"$dir\\$fname\", \"-d$dir\", 'install_script.iss' -LogPath \"$dir\\innounp-iss.log\"", +"if (-not $extracted -or !(Test-Path \"$dir\\install_script.iss\")) { error 'Failed to extract install_script.iss'; break }", "$installScript = Get-Content -Path \"$dir\\install_script.iss\" -Raw",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bucket/attribute-changer.json` around lines 25 - 27, The extraction sequence using Expand-InnoArchive/Invoke-ExternalCommand then reading $installScript with Get-Content can fail silently if innounp fails; modify the block around Expand-InnoArchive/Invoke-ExternalCommand (the commands that call Get-HelperPath -Helper Innounp and set $installScript) to verify extraction succeeded by checking the external command's exit status or by testing Test-Path for "$dir\install_script.iss" before calling Get-Content, and on failure log a descriptive error (including stdout/stderr if available) and abort/return/throw to avoid the confusing "file not found" later.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bucket/attribute-changer.json`:
- Around line 7-14: In the JSON "notes" array update the grammatical error by
changing the string fragment "Scoop persist the selected language, but the
language IDs could be changed at any time by the developer." to use the correct
verb form "persists" (i.e., "Scoop persists the selected language, but the
language IDs could be changed at any time by the developer.") so the "notes"
entry reflects proper grammar.
- Line 124: The current fallback uses `$langs.IndexOf('english')` which can
return -1 and lead to negative indexing; update the logic that sets `$index`
(the `if (`$index -lt 0 -or `$index -ge `$langs.Count) { `$index =
`$langs.IndexOf('english') }` block) to detect when `IndexOf('english')` returns
-1 and in that case set `$index` to 0 (or `$langs[0]`) as the ultimate fallback,
ensuring `$index` is always within 0..`$langs.Count-1`.
- Around line 133-135: The default langCode write uses
($langs.IndexOf('english') + 1) which yields 0 when 'english' isn't found,
causing a -1 index later; change this to compute the index into a variable (e.g.
$pos = $langs.IndexOf('english')), check if $pos -lt 0 and set $pos = 0 when
missing, then write ($pos + 1) with Set-Content "$dir\\langCode" so the file
contains 1 (first language) instead of 0.
---
Duplicate comments:
In `@bucket/attribute-changer.json`:
- Around line 138-143: post_install currently pipes the raw langCode file into
change-ac-language.ps1 which assumes a numeric value; add a validation step in
post_install that reads and trims "$dir\\langCode", checks it is a valid integer
(and within expected range), and if not replaces it with the fallback/default
before invoking "& \"$dir\\change-ac-language.ps1\" ..."; alternatively (or
additionally) harden change-ac-language.ps1 by wrapping the existing
[int]$args[0] parse in a try/catch or use a safe parse (e.g., TryParse) to fall
back to the default when the value is non-numeric or corrupted so the script
never throws on bad input.
---
Nitpick comments:
In `@bucket/attribute-changer.json`:
- Around line 25-27: The extraction sequence using
Expand-InnoArchive/Invoke-ExternalCommand then reading $installScript with
Get-Content can fail silently if innounp fails; modify the block around
Expand-InnoArchive/Invoke-ExternalCommand (the commands that call Get-HelperPath
-Helper Innounp and set $installScript) to verify extraction succeeded by
checking the external command's exit status or by testing Test-Path for
"$dir\install_script.iss" before calling Get-Content, and on failure log a
descriptive error (including stdout/stderr if available) and abort/return/throw
to avoid the confusing "file not found" later.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 847922f2-0921-4635-8793-96de3e1f12c2
📒 Files selected for processing (1)
bucket/attribute-changer.json
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bucket/attribute-changer.json`:
- Around line 90-114: After collecting matches from $matchesAc using the regex
against $installScript, check if the resulting $langs array is empty and
immediately abort with a clear error (throw or exit) so the installer fails fast
instead of producing broken state later; add this early-check right after the
foreach that builds $langs (the block using $patternAc, $matchesAc, $langs and
$match.Groups['Lang']) and similarly add the same emptiness check before the
later block that prepares language-specific files (the block that generates
change-ac-language.ps1 and copies messages/*.ini and ac,*.pdf), referencing the
same variables ($matchesAc, $langs) and bail out with a descriptive message if
no languages were detected.
- Around line 123-128: Persist the language identifier string instead of the
transient numeric slot: write the selected $ac_language (e.g., "english",
"german") into the langCode file rather than ($index + 1), so future reorders of
$langs don't break upgrades; update the logic around $index / $ac_language (the
Copy-Item lines that use $ac_language and the Set-Content that writes langCode)
to store the name, and move any conversion between numeric CLI IDs and
$ac_language to the CLI boundary where arguments are parsed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 64973308-c145-4956-b7bc-04e5868e3306
📒 Files selected for processing (1)
bucket/attribute-changer.json
| "`$index = [int]`$args[0] - 1", | ||
| "if (`$index -lt 0 -or `$index -ge `$langs.Count) { `$index = `$langs.IndexOf('english'); if (`$index -lt 0) { `$index = 0 } }", | ||
| "`$ac_language = `$langs[`$index]", | ||
| "Copy-Item \"$dir\\messages,`$ac_language.ini\" \"$dir\\messages.ini\" -Force", | ||
| "Copy-Item \"$dir\\ac,`$ac_language.pdf\" \"$dir\\ac.pdf\" -Force", | ||
| "Set-Content \"$dir\\langCode\" (`$index + 1) -Encoding utf8 -Force", |
There was a problem hiding this comment.
Persist the language key, not the transient numeric slot.
The new layout finally gives you stable identifiers (english, german, ...), but langCode still stores a 1-based position. If the vendor reorders languages in a future installer, Line 141 will silently reapply the wrong language on upgrade even though the extracted filenames are unchanged. Persist the language name and only translate numeric IDs at the CLI boundary.
Proposed fix
-`$index = [int]`$args[0] - 1
-if (`$index -lt 0 -or `$index -ge `$langs.Count) { `$index = `$langs.IndexOf('english'); if (`$index -lt 0) { `$index = 0 } }
-`$ac_language = `$langs[`$index]
+`$selection = `$args[0].Trim().ToLowerInvariant()
+if (`$selection -match '^\d+$') {
+ `$index = [int]`$selection - 1
+ if (`$index -lt 0 -or `$index -ge `$langs.Count) { `$index = `$langs.IndexOf('english'); if (`$index -lt 0) { `$index = 0 } }
+ `$ac_language = `$langs[`$index]
+} else {
+ `$ac_language = if (`$langs -contains `$selection) { `$selection } else { if (`$langs -contains 'english') { 'english' } else { `$langs[0] } }
+}
Copy-Item \"$dir\\messages,`$ac_language.ini\" \"$dir\\messages.ini\" -Force
Copy-Item \"$dir\\ac,`$ac_language.pdf\" \"$dir\\ac.pdf\" -Force
-Set-Content \"$dir\\langCode\" (`$index + 1) -Encoding utf8 -Force
+Set-Content \"$dir\\langCode\" `$ac_language -Encoding utf8 -Force
...
if (!(Test-Path \"$persist_dir\\langCode\")) {
$defaultIdx = $langs.IndexOf('english'); if ($defaultIdx -lt 0) { $defaultIdx = 0 }
- Set-Content \"$dir\\langCode\" ($defaultIdx + 1) -Encoding utf8
+ Set-Content \"$dir\\langCode\" $langs[$defaultIdx] -Encoding utf8
}Also applies to: 133-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bucket/attribute-changer.json` around lines 123 - 128, Persist the language
identifier string instead of the transient numeric slot: write the selected
$ac_language (e.g., "english", "german") into the langCode file rather than
($index + 1), so future reorders of $langs don't break upgrades; update the
logic around $index / $ac_language (the Copy-Item lines that use $ac_language
and the Set-Content that writes langCode) to store the name, and move any
conversion between numeric CLI IDs and $ac_language to the CLI boundary where
arguments are parsed.
There was a problem hiding this comment.
I'll wait for a maintainer's opinion on this
There was a problem hiding this comment.
@SiriosDev, that's completely fair — happy to wait for a maintainer's input. For context when they review: the concern is that storing a 1-based position in langCode ties persistence to the current ordering of $langs. If the vendor reorders languages in a future installer, post_install would silently apply the wrong language on upgrade even though the extracted filenames (messages,german.ini, etc.) would still be valid. Persisting the language name (e.g., "german") instead would make upgrades resilient to reordering. The tradeoff is a slightly more complex CLI boundary in change-ac-language.ps1, as shown in the proposed fix above.
There was a problem hiding this comment.
Sure, but if the developer were to change the language name tomorrow—for example, by switching to langcodes—the worst-case scenario would be a fallback, which would still require the user to change the language manually, but with the added inconvenience of having to type out an entire string instead of just a number.
There was a problem hiding this comment.
@SiriosDev, that's a fair point — if the developer renames language identifiers entirely, both approaches result in the same fallback outcome and require manual re-selection. One clarification on the UX concern though: the proposed fix actually preserves numeric input at the CLI boundary (the -match '^\d+$' branch handles it), so users would still type numbers like change-ac-language.ps1 2. The langCode file is only an internal detail that users never interact with directly.
In summary, the real tradeoff is between failure modes:
- Numeric index stored: vendor reorders → wrong language applied silently on upgrade (harder to detect)
- Name stored: vendor renames → fallback to English on upgrade (obvious, user is effectively notified by getting English)
Both are defensible. Since you're waiting for a maintainer's opinion anyway, I'll leave it there — it's ultimately a judgment call on which failure mode is preferable.
|
ops my bad |
|
/verify |
|
Your changes did not pass all checks. Please address the issues in the manifest and comment starting with attribute-changer
|
|
/verify |
|
All changes look good. Wait for review from human collaborators. attribute-changer
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
bucket/attribute-changer.json (1)
113-120:if (!($args.Count -eq 1))is awkward;if ($args.Count -ne 1)reads better.Stylistic, but it makes the usage gate easier to scan. No behavioral change.
♻️ Suggested refactor
- "if (!(`$args.Count -eq 1))", + "if (`$args.Count -ne 1)",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bucket/attribute-changer.json` around lines 113 - 120, The conditional using if (!($args.Count -eq 1)) is awkward; replace it with the clearer if ($args.Count -ne 1) in the script where the usage gate is enforced (the if block that checks $args.Count and prints INVALID USAGE), keeping the existing body unchanged; update the condition in that if statement (referencing $args.Count and the surrounding Write-Host/for loop block) to use -ne for readability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bucket/attribute-changer.json`:
- Around line 121-126: The script currently coerces invalid or out-of-range user
input (`$index`) to a fallback and unconditionally writes it to the persisted
lang file (`Set-Content "$dir\langCode"`), which causes accidental permanent
changes; change the flow in the function that uses `$index`, `$langs`, and
`$ac_language` so that you (1) validate the user-supplied `$args[0]` (numeric
and within 1..`$langs.Count`) before performing any Copy-Item or Set-Content,
(2) if invalid, print a clear warning and exit without copying or updating
`langCode`, and (3) only perform the Copy-Item and Set-Content when the index
was valid; preserve the existing fallback behavior for the separate post_install
caller by adding a flag/parameter or separate entry point so post_install can
still recover using the fallback without treating it as direct user input.
- Line 29: The current regex $patternArc requires a trailing '; Check: "..."' so
any {app}\ entries in install_script.iss that lack a Check: will not match and
thus won't be relocated; run innounp to extract install_script.iss and verify
all [Files] entries referencing '{app}\' (excluding those handled by
$patternTemplate, $patternMes, $patternAc) include Check:, and if not, update
$patternArc to make the '; Check: "..."' portion optional or add a separate
pattern to match common architecture-agnostic files (e.g., license/readme) so
entries with optional DestDir/DestName still get relocated.
- Around line 72-83: The extracted $lang string can contain multiple
space-separated languages; update both message and AC processing loops to split
the captured $lang on whitespace and iterate each language separately: in the
messages loop (pattern variable $patternMes, matcher $matchesMes, foreach
($match in $matchesMes) using $source, $destDir, $lang, $destName, $destPath,
Move-Item) replace single-use of $lang with a loop over $lang -split '\s+' and
create/move a file per language (e.g., build destName as
"messages,<singleLang>.ini"); do the same in the AC processing section (pattern
$patternAc / $matchesAc / its foreach) and ensure each individual language is
also added separately to the $langs array used by change-ac-language.ps1 so
entries aren’t lumped into one item.
---
Nitpick comments:
In `@bucket/attribute-changer.json`:
- Around line 113-120: The conditional using if (!($args.Count -eq 1)) is
awkward; replace it with the clearer if ($args.Count -ne 1) in the script where
the usage gate is enforced (the if block that checks $args.Count and prints
INVALID USAGE), keeping the existing body unchanged; update the condition in
that if statement (referencing $args.Count and the surrounding Write-Host/for
loop block) to use -ne for readability.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2c0aa93f-10a1-40f3-8f60-f68b141086d2
📒 Files selected for processing (1)
bucket/attribute-changer.json
1 participant
To resolve the language-switching issues in this application, the manifest has been reworked using one that is entirely inspired by GIMP.
Extras/bucket/gimp.json
Lines 16 to 49 in 4df2d39
Instead of using the automatic extractor from Scoop, followed by pre-install scripts to remove files based on architecture and post-install scripts to (attempt to) rename files according to the developer’s confused order, I switched to manual extraction and file manipulation based on the information contained within the installer itself, so that the files directly indicate their language in the filename and are independent of the above-mentioned disorder. Additionally, I directly handle the files based on the target architecture. Furthermore, to ensure (or at least attempt to ensure) language persistence, a support file has been created that is used with every update.
In summary:
notesno longer contains documentation forchange-ac-language.ps1innosetuphas been removedpre_installscripts are no longer divided by architecture and now serve solely to reset the persistent language (the language file is not set directly to avoid update issues)post_installno longer has the task of haphazardly renaming extracted files but rather of “activating” the DLL for File Explorerinstallerno longer has the sole task of creating the language-switch script but now handles the extraction and renaming of fileschange-ac-lang-uage.ps1now includes its documentation when run without arguments, and has obviously been rewritten to handle the new file structure**persistscript that persists a support file created to reset the language with every update**I opted to have files named after the languages and the script using IDs to try to be as dev-agnostic as possible—names because they are extracted from the setup, and IDs because the names could potentially change; this way, the only breaking change is the order of the languages
Closes #15332
<manifest-name[@version]|chore>: <general summary of the pull request>