UI: Rework the options window#7097
Conversation
📝 WalkthroughWalkthroughReworked the options dialog: replaced legacy Button-based layout with Group-based controls, added a live search Edit field that filters options and hides tabs during search, revamped tab visuals, rebuilt option rows with hover/highlight and separators, and adjusted sizing, scrollbar behavior, and cancel handling. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 |
![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
🧹 Nitpick comments (2)
lua/ui/dialogs/options.lua (2)
216-219: Dead code:SetSolidColorstub onbgis never invoked.The hover visuals call
SetSolidColoronself._bg(a bitmap created byUIUtil.CreateBitmapColorat line 225, which already supports it), never onbgitself. The no-op stub can be removed without any behavioral change.♻️ Proposed cleanup
- local bg = Group(parent) - - if not bg.SetSolidColor then - bg.SetSolidColor = function(self, color) - end - end - - LayoutHelpers.SetDimensions(bg, OptionRowWidth, OptionRowHeight) + local bg = Group(parent) + LayoutHelpers.SetDimensions(bg, OptionRowWidth, OptionRowHeight)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/ui/dialogs/options.lua` around lines 216 - 219, Remove the dead no-op stub that assigns bg.SetSolidColor because SetSolidColor is never called on the local bg variable; hover visuals call SetSolidColor on self._bg (created via UIUtil.CreateBitmapColor) so the empty function wrapper is unnecessary—delete the entire conditional block that defines bg.SetSolidColor to clean up the code.
521-527: Search results lack tab/section context.While searching, tab buttons are hidden and matching items from every tab are merged into a single scrollable list. Because results are interleaved and any header rows that happen to match the query are included inline (while headers whose children match are not), users lose visual grouping by tab. Consider either:
- always rendering each tab's matching header as a sectioning separator when that tab has any match, or
- prefixing matched rows with a small tab-name label on the left.
Purely a UX polish — not blocking.
Also applies to: 585-593
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/ui/dialogs/options.lua` around lines 521 - 527, When isSearching is true the code currently hides tabButtons and flattens all matches into one list, losing tab/section context; update the search-list population logic (the routine that builds the scrollable results and the code that hides/shows tabButtons around isSearching) so that when a given tab has any matches you insert that tab's header as a section separator (or alternatively prefix each matched row with the tab name) before adding that tab's matched rows to the unified list; look for references to isSearching, tabButtons, the scroll/list population routine that creates header rows and rows (and any header creation helper) and ensure headers are created and inserted when building search results.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lua/ui/dialogs/options.lua`:
- Around line 362-366: The rebuild on every keystroke can leave tooltips
orphaned because SetNewPage calls optionGrid:DeleteAndDestroyAll(true) which can
destroy hovered controls before MouseExit fires; to fix, ensure
Tooltip.DestroyMouseoverDisplay() is called before tearing down the grid—either
add an unconditional Tooltip.DestroyMouseoverDisplay() at the start of
SetNewPage or call it in searchEdit.OnTextChanged right before SetNewPage is
invoked so any existing mouseover tooltip is destroyed prior to
optionGrid:DeleteAndDestroyAll(true).
- Around line 530-537: The search uses byte-wise string.lower and string.find in
AddOptionsFromTabData (variables: isSearching, titleStr, searchString), which
fails for non-ASCII case differences; either document this limitation clearly
where AddOptionsFromTabData is defined and in the options UI docs, or implement
Unicode-aware normalization before comparing (e.g., add a helper to
case-fold/normalize both titleStr and searchString using an explicit mapping or
normalization table for accented/Unicode characters and use that normalized form
in the string.find call) so searches become case-insensitive for localized
titles.
- Around line 681-682: Remove the dead backwards-compatibility comment and the
unused local import of Text (the line importing
import("/lua/maui/text.lua").Text) from options.lua; since Text is not
referenced in this file and declaring it local blocks external access, simply
delete that comment + the local Text import to eliminate misleading dead code
(alternatively, if external access is actually needed, remove only the local
keyword to export Text, but prefer deletion here).
---
Nitpick comments:
In `@lua/ui/dialogs/options.lua`:
- Around line 216-219: Remove the dead no-op stub that assigns bg.SetSolidColor
because SetSolidColor is never called on the local bg variable; hover visuals
call SetSolidColor on self._bg (created via UIUtil.CreateBitmapColor) so the
empty function wrapper is unnecessary—delete the entire conditional block that
defines bg.SetSolidColor to clean up the code.
- Around line 521-527: When isSearching is true the code currently hides
tabButtons and flattens all matches into one list, losing tab/section context;
update the search-list population logic (the routine that builds the scrollable
results and the code that hides/shows tabButtons around isSearching) so that
when a given tab has any matches you insert that tab's header as a section
separator (or alternatively prefix each matched row with the tab name) before
adding that tab's matched rows to the unified list; look for references to
isSearching, tabButtons, the scroll/list population routine that creates header
rows and rows (and any header creation helper) and ensure headers are created
and inserted when building search results.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9d57afd4-27c8-4208-b7e8-97fd2500044f
📒 Files selected for processing (2)
changelog/snippets/feature.7097.mdlua/ui/dialogs/options.lua
| -- kept for mod backwards compatibility | ||
| local Text = import("/lua/maui/text.lua").Text No newline at end of file | ||
| local Text = import("/lua/maui/text.lua").Text |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how `Text` was declared in options.lua prior to this PR
# and whether any mod-side code references it from this module.
fd -t f 'options.lua' lua/ui/dialogs | head
git log --oneline -- lua/ui/dialogs/options.lua | head -20
git show HEAD~1:lua/ui/dialogs/options.lua 2>/dev/null | rg -n 'Text\s*=\s*import' || \
git log -p -S'Text = import("/lua/maui/text.lua")' -- lua/ui/dialogs/options.lua | head -80
# Look for mod-style external references to the options module's Text symbol
rg -nP --type=lua -C2 'dialogs/options\.lua.*\)\.Text\b'
rg -nP --type=lua -C2 "require.*dialogs/options.*Text\b"Repository: FAForever/fa
Length of output: 138
🏁 Script executed:
#!/bin/bash
# Get the exact git history and show previous versions
git log --oneline --all -- lua/ui/dialogs/options.lua | head -10
# Try to show the file from the parent commit(s)
git show HEAD:lua/ui/dialogs/options.lua 2>/dev/null | grep -n -A2 -B2 "Text" | grep -E "^\d+[-:].*Text" | head -20Repository: FAForever/fa
Length of output: 488
🏁 Script executed:
#!/bin/bash
# Get git diff to see what changed in this commit
git diff HEAD~1 HEAD -- lua/ui/dialogs/options.lua 2>/dev/null | grep -A10 -B10 "Text.*import" | head -40Repository: FAForever/fa
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Search for any code that accesses the options module's Text export
rg -n "options\s*\)" lua --type=lua -A3 | grep -i "text" | head -20
rg -n "import.*options\.lua" lua --type=lua -A2 | head -30Repository: FAForever/fa
Length of output: 2257
🏁 Script executed:
#!/bin/bash
# Get the parent commit and show the file at that point
git log --oneline -5 -- lua/ui/dialogs/options.lua
echo "---"
# Show the specific line in the previous commit
git show HEAD~1:lua/ui/dialogs/options.lua 2>/dev/null | sed -n '675,690p'
echo "---"
# Show current version
git show HEAD:lua/ui/dialogs/options.lua 2>/dev/null | sed -n '675,690p'Repository: FAForever/fa
Length of output: 295
🏁 Script executed:
#!/bin/bash
# Show the exact diff for this region
git diff HEAD~1 HEAD -- lua/ui/dialogs/options.lua 2>/dev/null | grep -A15 -B5 "Text"Repository: FAForever/fa
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Search across the entire repo for any external access to options module's Text
rg -n "\.Text" lua --type=lua | grep -i "options" | head -20
rg -n "import.*options" lua --type=lua -A5 | grep "\.Text" | head -20Repository: FAForever/fa
Length of output: 133
🏁 Script executed:
#!/bin/bash
# Compare file sizes
echo "HEAD~1 line count:"
git show HEAD~1:lua/ui/dialogs/options.lua 2>/dev/null | wc -l
echo "HEAD line count:"
git show HEAD:lua/ui/dialogs/options.lua 2>/dev/null | wc -l
echo ""
# Search for Text declaration in HEAD~1
echo "Text in HEAD~1:"
git show HEAD~1:lua/ui/dialogs/options.lua 2>/dev/null | grep -n "Text" | head -20
echo ""
echo "Text in HEAD:"
git show HEAD:lua/ui/dialogs/options.lua 2>/dev/null | grep -n "Text" | head -20Repository: FAForever/fa
Length of output: 1735
🏁 Script executed:
#!/bin/bash
# Look at the git log with actual patches to see what changed
git log -p --all -- lua/ui/dialogs/options.lua 2>/dev/null | grep -A10 -B10 "Text.*import" | head -60Repository: FAForever/fa
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Check git log with follow to track file history
git log --follow --oneline -- lua/ui/dialogs/options.lua | head -20
echo "---"
# Check if file exists in any recent commits before HEAD~1
git log --all --oneline -- lua/ui/dialogs/options.lua | head -20Repository: FAForever/fa
Length of output: 548
🏁 Script executed:
#!/bin/bash
# Search for when Text import was added to this file
git log -p --all -- lua/ui/dialogs/options.lua 2>/dev/null | grep -B20 "local Text = import" | head -40Repository: FAForever/fa
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Check the merge base and relevant branch info
git log --oneline -20
echo "---"
git show 4aba5cd3 --stat 2>/dev/null | head -30Repository: FAForever/fa
Length of output: 1804
🏁 Script executed:
#!/bin/bash
# List available branches
git branch -a | grep options
echo "---"
# Check if file exists in new_options_window branch
git show new_options_window:lua/ui/dialogs/options.lua 2>/dev/null | wc -l
echo "---"
# Get the Text line from new_options_window
git show new_options_window:lua/ui/dialogs/options.lua 2>/dev/null | tail -20Repository: FAForever/fa
Length of output: 65
🏁 Script executed:
#!/bin/bash
# Search in older commits for where this pattern came from
git log --all -S "kept for mod backwards compatibility" --oneline -- luaRepository: FAForever/fa
Length of output: 142
🏁 Script executed:
#!/bin/bash
# Check if there's an older options.lua elsewhere that might have had this
find lua -name "*options*" -type f | head -20Repository: FAForever/fa
Length of output: 135
🏁 Script executed:
#!/bin/bash
# Check the old options.lua file
cat lua/options/options.lua | head -50
echo "---"
cat lua/options/options.lua | grep -n "Text" | head -10
echo "---"
cat lua/options/options.lua | grep -n "backwards" | head -10Repository: FAForever/fa
Length of output: 3437
🏁 Script executed:
#!/bin/bash
# Search for Text in old options files
grep -n "local Text" lua/options/*.lua
grep -n "^Text = " lua/options/*.luaRepository: FAForever/fa
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Check if the new options.lua is a replacement/refactor of old one
wc -l lua/options/options.lua lua/ui/dialogs/options.luaRepository: FAForever/fa
Length of output: 130
🏁 Script executed:
#!/bin/bash
# Check if Text (as imported variable, not as method) is used
cat lua/ui/dialogs/options.lua | grep -n "Text" | grep -v "SetText\|CreateText\|_tipText\|labelText" | head -20Repository: FAForever/fa
Length of output: 301
🏁 Script executed:
#!/bin/bash
# Show the exact context around line 682
tail -10 lua/ui/dialogs/options.luaRepository: FAForever/fa
Length of output: 236
Remove the unused Text import and comment; the line is dead code that will not achieve the stated backwards compatibility goal.
Line 682 imports Text but it is never used anywhere in the file. More critically, declaring it local prevents external mods from accessing it via import("/lua/ui/dialogs/options.lua").Text, making the "mod backwards compatibility" comment misleading or incorrect. Since this is a new file with no prior history to preserve, either drop local (if external access is genuinely required), or remove the line and comment entirely (as dead code).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lua/ui/dialogs/options.lua` around lines 681 - 682, Remove the dead
backwards-compatibility comment and the unused local import of Text (the line
importing import("/lua/maui/text.lua").Text) from options.lua; since Text is not
referenced in this file and declaring it local blocks external access, simply
delete that comment + the local Text import to eliminate misleading dead code
(alternatively, if external access is actually needed, remove only the local
keyword to export Text, but prefer deletion here).
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)
lua/ui/dialogs/options.lua (1)
294-308:⚠️ Potential issue | 🟡 MinorClear active mouseover tooltips before destroying the dialog.
SetNewPagenow handles rebuilds, butKillDialogcan still destroy a hovered option row via Escape/NIS/cancel paths beforeMouseExitruns.🛡️ Proposed fix
local function KillDialog() + Tooltip.DestroyMouseoverDisplay() currentTabButton = false OptionsLogic.SetCustomDataChangedCallback(nil)Also applies to: 550-562
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/ui/dialogs/options.lua` around lines 294 - 308, KillDialog may destroy a dialog while an option row is still hovered, so before destroying the dialog (before dialog:Destroy() / parent:Destroy()) call the routine that clears active mouseover tooltips; add a call such as UIUtil.HideMouseover() or OptionsLogic.ClearMouseoverTooltip() (or invoke the hovered row's MouseExit) in KillDialog (near the dialog/parent Destroy calls) so any active tooltip is hidden before dialog is destroyed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lua/ui/dialogs/options.lua`:
- Around line 533-538: The loop uses the non-standard `continue` keyword
(causing Luacheck E011); replace both occurrences by inverting the condition and
wrapping the remaining loop body in an if-block so standard Lua control flow is
used: for the title filter (where you compute titleStr = LOC(option.title) and
call string.find with searchString) remove `continue` and change the test from
`if not string.find(...) then continue` to `if string.find(...) then ... end`
(placing the original remaining loop body inside that `then`), and apply the
same pattern for the other occurrence that filters by description/search—this
removes `continue` and preserves behavior using only standard Lua constructs
(use the existing symbols option, titleStr, searchString, LOC, string.find).
---
Outside diff comments:
In `@lua/ui/dialogs/options.lua`:
- Around line 294-308: KillDialog may destroy a dialog while an option row is
still hovered, so before destroying the dialog (before dialog:Destroy() /
parent:Destroy()) call the routine that clears active mouseover tooltips; add a
call such as UIUtil.HideMouseover() or OptionsLogic.ClearMouseoverTooltip() (or
invoke the hovered row's MouseExit) in KillDialog (near the dialog/parent
Destroy calls) so any active tooltip is hidden before dialog is destroyed.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6623d1cb-6611-4b3d-8849-458f160f1bc5
📒 Files selected for processing (1)
lua/ui/dialogs/options.lua
| if isSearching then | ||
| local titleStr = LOC(option.title) | ||
| -- Note: string.lower does not fully handle Unicode case matching | ||
| if not string.find(string.lower(titleStr), string.lower(searchString), 1, true) then | ||
| continue | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm whether the repository's Lua checker can parse the new control flow.
# Expected: no parser error near line 538.
if command -v luacheck >/dev/null 2>&1; then
luacheck --codes lua/ui/dialogs/options.lua
else
printf 'luacheck is not installed; inspect the changed continue statements instead:\n'
rg -n -C2 '^\s*continue\s*$' lua/ui/dialogs/options.lua
fiRepository: FAForever/fa
Length of output: 223
🏁 Script executed:
# Read the file to see the actual code and verify the line numbers
wc -l lua/ui/dialogs/options.luaRepository: FAForever/fa
Length of output: 86
🏁 Script executed:
# Check the specific lines mentioned in the review (533-538)
sed -n '530,545p' lua/ui/dialogs/options.luaRepository: FAForever/fa
Length of output: 800
🏁 Script executed:
# Check the second location (611-614)
sed -n '608,620p' lua/ui/dialogs/options.luaRepository: FAForever/fa
Length of output: 408
🏁 Script executed:
# Find all continue statements in the file to see the full picture
rg -n '^\s*continue\s*$' lua/ui/dialogs/options.luaRepository: FAForever/fa
Length of output: 117
Refactor to remove non-standard continue statements that break Lua tooling.
Luacheck reports an E011 parse error at line 538, confirming that continue is not recognized by standard Lua parsers. While the Moho runtime may support this extension, the code fails static analysis and is incompatible with standard Lua tooling.
Refactor both occurrences (lines 537 and 613) to use conditional logic instead:
Line 537 fix
- if isSearching then
+ local includeOption = true
+ if isSearching then
local titleStr = LOC(option.title)
-- Note: string.lower does not fully handle Unicode case matching
- if not string.find(string.lower(titleStr), string.lower(searchString), 1, true) then
- continue
- end
+ includeOption = string.find(string.lower(titleStr), string.lower(searchString), 1, true) ~= nil
end
+
+ if includeOption then
-- add an empty row before headers for visual spacing (except the very first row)
if option.type == 'header' and row > 1 then
optionGrid:AppendRows(1, true)
row = row + 1
endLine 613 fix
for _, key in optionsOrder do
- if not options[key] then
- continue
- end
-
- local tabData = options[key]
- local curButton = CreateModernTab(contentParent, tabData.title)
+ if options[key] then
+ local tabData = options[key]
+ local curButton = CreateModernTab(contentParent, tabData.title)
...
- curButton.tabData = tabData
+ curButton.tabData = tabData
+ end
end🧰 Tools
🪛 Luacheck (1.2.0)
[error] 538-538: expected '=' near 'end'
(E011)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lua/ui/dialogs/options.lua` around lines 533 - 538, The loop uses the
non-standard `continue` keyword (causing Luacheck E011); replace both
occurrences by inverting the condition and wrapping the remaining loop body in
an if-block so standard Lua control flow is used: for the title filter (where
you compute titleStr = LOC(option.title) and call string.find with searchString)
remove `continue` and change the test from `if not string.find(...) then
continue` to `if string.find(...) then ... end` (placing the original remaining
loop body inside that `then`), and apply the same pattern for the other
occurrence that filters by description/search—this removes `continue` and
preserves behavior using only standard Lua constructs (use the existing symbols
option, titleStr, searchString, LOC, string.find).
1 participant
Description of the proposed changes
FAF added a lot of option entries, which made the original options window (very similar to the one on Steam) feel cramped and harder to navigate. This rework gives the dialog more room, improves readability, and adds live search so the growing option set remains manageable.
Features:
Testing done on the proposed changes
Tested extensively in real games.
Screenshots
Checklist
Request 2-3 reviewers from the list of reviewers and their areas of knowledge.Summary by CodeRabbit
New Features
Bug Fixes