Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughA new public constructor for the CCLabel class was introduced to simplify label creation with text and font size parameters using Arial as the default font. Additionally, the SystemFont getter now returns "Arial" as a fallback when no font name is set. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cocos2d/label_nodes/CCLabel.cs (2)
43-55:SystemFontgetter fallback doesn't protect internal usages ofm_FontName.The
?? "Arial"fallback only applies when external code readsSystemFont. Internally,m_FontNameis used directly inDraw()(Line 365, 371),Textsetter (Line 89),SystemFontSizesetter (Line 63), andSystemFontSpacingsetter (Line 76) — all of which will still seenullif no font was explicitly set (e.g., via the parameterless constructor).Consider using the property
SystemFontin those internal call sites, or assigning the default in the field initializer:Option A: Default at field level
- protected string m_FontName; + protected string m_FontName = "Arial";This would make the getter fallback unnecessary and ensure consistent behavior everywhere.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cocos2d/label_nodes/CCLabel.cs` around lines 43 - 55, The SystemFont getter's "?? 'Arial'" fallback doesn't protect internal code that accesses m_FontName directly (used in Draw(), the Text setter, SystemFontSize setter, and SystemFontSpacing setter), so either initialize m_FontName with the default ("Arial") at declaration or change all internal usages to read the SystemFont property instead of m_FontName; update calls that pass m_FontName into InitializeFont (and any places that set m_pConfiguration/m_bFontDirty) to use SystemFont so the default is consistently applied across Draw(), Text, SystemFontSize and SystemFontSpacing.
126-129: Consider extracting the default font name to a constant.
"Arial"appears as a magic string here and in theSystemFontgetter (Line 45). A shared constant would keep them in sync and make the default easy to change later.Suggested refactor
+ private const string DefaultFont = "Arial"; + - public CCLabel(string text, float fontSize) : - this(text, "Arial", fontSize, CCSize.Zero, CCTextAlignment.Left, CCVerticalTextAlignment.Top) + public CCLabel(string text, float fontSize) : + this(text, DefaultFont, fontSize, CCSize.Zero, CCTextAlignment.Left, CCVerticalTextAlignment.Top) { }And update the getter:
- get { return m_FontName ?? "Arial"; } + get { return m_FontName ?? DefaultFont; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cocos2d/label_nodes/CCLabel.cs` around lines 126 - 129, Extract the magic font name into a shared constant: add a new constant (e.g., DEFAULT_FONT_NAME) and replace the literal "Arial" in the CCLabel constructor (public CCLabel(string text, float fontSize)) and in the SystemFont getter with that constant so both use the same default and are easy to change later; ensure the constant is declared close to CCLabel (static readonly or const) and referenced where the default font name is currently hard-coded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cocos2d/label_nodes/CCLabel.cs`:
- Around line 43-55: The SystemFont getter's "?? 'Arial'" fallback doesn't
protect internal code that accesses m_FontName directly (used in Draw(), the
Text setter, SystemFontSize setter, and SystemFontSpacing setter), so either
initialize m_FontName with the default ("Arial") at declaration or change all
internal usages to read the SystemFont property instead of m_FontName; update
calls that pass m_FontName into InitializeFont (and any places that set
m_pConfiguration/m_bFontDirty) to use SystemFont so the default is consistently
applied across Draw(), Text, SystemFontSize and SystemFontSpacing.
- Around line 126-129: Extract the magic font name into a shared constant: add a
new constant (e.g., DEFAULT_FONT_NAME) and replace the literal "Arial" in the
CCLabel constructor (public CCLabel(string text, float fontSize)) and in the
SystemFont getter with that constant so both use the same default and are easy
to change later; ensure the constant is declared close to CCLabel (static
readonly or const) and referenced where the default font name is currently
hard-coded.
There was a problem hiding this comment.
Pull request overview
This PR adds a convenience constructor to the CCLabel class that accepts only text and font size parameters, defaulting the font name to "Arial". It also updates the SystemFont property getter to provide a default value of "Arial" when the internal font name field is null.
Changes:
- Added a new constructor
CCLabel(string text, float fontSize)that defaults to Arial font - Modified the
SystemFontproperty getter to return "Arial" whenm_FontNameis null
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
* license update * Update LICENSE Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update LICENSE Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * add directory build props for centralizing MG version * Update/ttf font improvements (#512) * add new cclabel test * update autosize test * add custom font * adjust spacing for line height for vertical alignment space * add more custom fonts for testing * adjust label texture scaling to ignore adjusting fontsize * clean up and add height padding prop * Update/support runactions (#514) * add support for RunActions * better support for CCRepeat for CCSprite * better check for sequence actions and return * Fix tag propagation inconsistency in RepeatForever overloads (#515) * Initial plan * Fix tag handling inconsistency in RepeatForever overloads Co-authored-by: brandmooffin <1774581+brandmooffin@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: brandmooffin <1774581+brandmooffin@users.noreply.github.com> --------- Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: brandmooffin <1774581+brandmooffin@users.noreply.github.com> * add size prop for ccnode (#521) * add size prop for ccnode * update prop summary * add constructor for default font (#529) * add constructor for default font * move font default * Add CCGameView embeddable view support (#522) * Initial plan * Add CCGameView embeddable view support similar to CocosSharp Co-authored-by: brandmooffin <1774581+brandmooffin@users.noreply.github.com> * Address code review feedback for CCGameView implementation Co-authored-by: brandmooffin <1774581+brandmooffin@users.noreply.github.com> * Fix Android and iOS build errors by removing deprecated OpenTK types Co-authored-by: brandmooffin <1774581+brandmooffin@users.noreply.github.com> * Fix Matrix type ambiguity for Android build by adding explicit using alias Co-authored-by: brandmooffin <1774581+brandmooffin@users.noreply.github.com> * Fix Matrix type conflict by using fully qualified Microsoft.Xna.Framework.Matrix Co-authored-by: brandmooffin <1774581+brandmooffin@users.noreply.github.com> * improvements for support CCGameView * add support for multiple views (#528) * add support for multiple views * changes from review comments * adjustments from latest review * Address review comments: add thread safety for secondary views, call RemoveExistingView, document CCDrawManagerState limitations Co-authored-by: brandmooffin <1774581+brandmooffin@users.noreply.github.com> * add embedded view tests --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: brandmooffin <1774581+brandmooffin@users.noreply.github.com> Co-authored-by: brandmooffin <brandmooffin@outlook.com> * bump version for release * update release notes * fixes from code review --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: brandmooffin <1774581+brandmooffin@users.noreply.github.com>
Summary by CodeRabbit