Skip to content

Annotation: OnSelectionChanged and GetSelectedUnits - #6967

Merged
Hdt80bro merged 2 commits into
FAForever:developfrom
4z0t:annotation/OnSelectionChanged
Dec 9, 2025
Merged

Annotation: OnSelectionChanged and GetSelectedUnits#6967
Hdt80bro merged 2 commits into
FAForever:developfrom
4z0t:annotation/OnSelectionChanged

Conversation

@4z0t

@4z0t 4z0t commented Nov 14, 2025

Copy link
Copy Markdown
Member

Description of the proposed changes

Add annotation about GetSelectedUnits being used within OnSelectionChanged.

Testing done on the proposed changes

No testing needed.

Additional context

Useful tip for modders, so they don't stumble as I did.

Checklist

Summary by CodeRabbit

  • Documentation
    • Added clarification notes to internal code comments regarding engine behavior.

Note: This release contains no user-visible changes.

@4z0t
4z0t requested a review from lL1l1 November 14, 2025 08:33
@coderabbitai

coderabbitai Bot commented Nov 14, 2025

Copy link
Copy Markdown
Contributor

Walkthrough

A comment was added to the OnSelectionChanged function in gamemain.lua documenting that GetSelectedUnits returns nil when the function is invoked by the engine. No functional code changes were made.

Changes

Cohort / File(s) Change Summary
Documentation & Comments
lua/ui/game/gamemain.lua
Added informational comment in OnSelectionChanged documenting nil return behavior of GetSelectedUnits when engine-invoked

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

Poem

🐰 A note in code, so clear and bright,
To guide the reader through the night—
When nil appears from engine's call,
This comment helps us understand it all! ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Annotation: OnSelectionChanged and GetSelectedUnits' clearly identifies the main change as adding an annotation about these functions, which matches the actual changeset.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description check ✅ Passed The PR description covers the main sections but omits the changelog snippet documentation, which is a required checklist item per the template guidelines.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
lua/ui/game/gamemain.lua (1)

608-608: Helpful tip for modders, consider minor wording clarification.

The annotation is valuable documentation that warns modders about a gotcha behavior. The current phrasing is slightly ambiguous—it could be clearer that it's the result of calling GetSelectedUnits within this function, not that the engine calls it.

Consider rephrasing to: "Within this function (called by the engine), GetSelectedUnits() will always return nil." This makes it explicit that calling GetSelectedUnits from within this callback won't work as expected.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5eddd6f and fd6668f.

📒 Files selected for processing (1)
  • lua/ui/game/gamemain.lua (1 hunks)

@4z0t
4z0t requested a review from clyfordv November 14, 2025 08:40
@Hdt80bro
Hdt80bro merged commit 33ef66d into FAForever:develop Dec 9, 2025
4 checks passed
relent0r pushed a commit to relent0r/fa that referenced this pull request Feb 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants