Fix selection with Hidden wrapper not being ignored in commandmode - #7186
Fix selection with Hidden wrapper not being ignored in commandmode#7186lL1l1 wants to merge 8 commits into
Hidden wrapper not being ignored in commandmode#7186Conversation
Test with
```
import('/lua/ui/game/selection.lua').Hidden(function()
UISelectionByCategory('ALLUNITS', false, false, false, false)
LOG('Selected units', table.getn(GetSelectedUnits()))
end)
```
gamemain's SetIgnoreSelection calls commandmode's function too This also allows us to skip the table accesses with import in gamemain every time we select stuff (+UI responiveness)
This reverts commit e14a779.
📝 WalkthroughWalkthroughHidden selection callbacks now set gamemain’s ignore-selection state while restoring selections and command mode. ChangesSelection handling
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant SelectionUI
participant GameMain
participant Callback
SelectionUI->>GameMain: Enable ignore selection
SelectionUI->>Callback: Run Hidden callback
Callback->>GameMain: Restore selection and command mode
SelectionUI->>GameMain: Disable ignore selection
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lua/ui/game/selection.lua`:
- Around line 54-59: Protect the callback and selection-restoration sequence
around callback, SelectUnits, and CM.StartCommandMode using protected execution,
ensuring hidden_select is reset and gamemainSetIgnoreSelection(false) runs even
when any step raises. After cleanup, rethrow the original error while preserving
normal restoration behavior on success.
🪄 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: f19be17e-bd8d-4b63-903e-63dbbcdb14ff
📒 Files selected for processing (3)
changelog/snippets/fix.7186.mdlua/ui/game/gamemain.lualua/ui/game/selection.lua
💤 Files with no reviewable changes (1)
- lua/ui/game/gamemain.lua
| gamemainSetIgnoreSelection(true) | ||
| callback() | ||
| SelectUnits(old_selection) | ||
| CM.StartCommandMode(current_command[1], current_command[2]) | ||
| hidden_select = false | ||
| gamemainSetIgnoreSelection(false) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Restore selection state on failure.
If callback(), SelectUnits(), or StartCommandMode() raises, execution skips lines 58-59 and leaves hidden_select and gamemain/commandmode’s ignoreSelection enabled permanently, suppressing later selection handling. Wrap the callback/restoration sequence in protected execution and clear both flags before rethrowing the original error.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lua/ui/game/selection.lua` around lines 54 - 59, Protect the callback and
selection-restoration sequence around callback, SelectUnits, and
CM.StartCommandMode using protected execution, ensuring hidden_select is reset
and gamemainSetIgnoreSelection(false) runs even when any step raises. After
cleanup, rethrow the original error while preserving normal restoration behavior
on success.
4z0t
left a comment
4z0t
left a comment
There was a problem hiding this comment.
Use solution from IssueBlueprintCommandToUnits.
|
No, just fix the one you're touching in this PR |
|
I refactored it as requested and also noticed that the hidden state would be broken by errors so I wrapped it in pcall. If pcall fails it fixes the hidden state and then errors so the script that called it doesn't continue assuming something in the callback worked. |
3 participants
Description of the proposed changes
Fixes selection done within the
Hiddenwrapper not being ignored incommandmode.lua.gamemain.lua'sSetIgnoreSelectionis used since it also sets the ignore mode incommandmode.luaand it lets us avoid an import call every time selection changes, which is especially frequent for hidden selections.Testing done on the proposed changes
Un-revert the debug logging commit.
Select a unit.
Select the move order, shift click once, then run this command while holding shift:
letting go of shift should cancel command mode and not log anything. Before these changes it didn't.
Checklist
Summary by CodeRabbit