Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 24 minutes and 30 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds KeyInputType and default-text support to input text widgets and updates input handling and rendering; introduces IODevice::delay(uint32_t) in the SDL2 backend; adds an error log when a requested font cannot be loaded in the SDL2 renderer. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/widgets.cpp (1)
260-283:⚠️ Potential issue | 🔴 Critical
defaultis a reserved keyword – rename the parameter to match the header fix.Same issue as in the header:
defaultis a C++ reserved keyword. Rename todefaultTextor similar.🐛 Proposed fix
-ret_code Widgets::inputText(Id id, Id parentId, const Rect &rect, Alignment alignment, KeyInputType type, const char *default) { +ret_code Widgets::inputText(Id id, Id parentId, const Rect &rect, Alignment alignment, KeyInputType type, const char *defaultText) { auto &ctx = TinyUi::getContext(); if (ctx.mBackendCtx == nullptr) { return InvalidRenderHandle; } if (ctx.mRoot == nullptr) { return InvalidRenderHandle; } Widget *widget = createWidget(ctx, id, parentId, rect, WidgetType::InputField); if (widget == nullptr) { return ErrorCode; } widget->mAlignment = alignment; widget->mKeyInputType = type; - if (default != nullptr) { - widget->mText.assign(default); + if (defaultText != nullptr) { + widget->mText.assign(defaultText); } widget->mCallback = new CallbackI(inputHandler, (void *)&ctx, Events::MouseButtonDownEvent); return ResultOk; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/widgets.cpp` around lines 260 - 283, The function Widgets::inputText uses a parameter named "default", which is a reserved C++ keyword; rename that parameter to match the header change (e.g., "defaultText") in the Widgets::inputText signature and all uses inside the implementation (including the null check and widget->mText.assign call) so the definition matches the header and compiles cleanly.src/backends/sdl2_renderer.cpp (1)
204-218:⚠️ Potential issue | 🟠 MajorThe
fontparameter is always overwritten, making the conditional block dead code.Line 215 unconditionally assigns
font = ctx.mDefaultFont;, which means:
- The
fontparameter passed todrawTextis always ignored- The error message at lines 208-209 fires when the passed
fontis null, but then immediately gets overwritten anyway- The conditional assignment at line 210 (
font = ctx.mDefaultFont;) is redundantThis appears to be unintended. If the intent is to always use the default font, remove the parameter. Otherwise, preserve the passed font when it's valid.
🔧 Proposed fix to respect the font parameter
if (ctx.mDefaultFont == nullptr) { if (ctx.mStyle.mFont.mName != nullptr) { loadFont(ctx); - if (font == nullptr) { - const std::string msg = "Cannot load font: " + std::string(ctx.mStyle.mFont.mName) + ", using the default font."; - ctx.mLogger(LogSeverity::Error, msg.c_str()); - font = ctx.mDefaultFont; - } } } - - font = ctx.mDefaultFont; + + if (font == nullptr) { + font = ctx.mDefaultFont; + } if (font == nullptr) { return InvalidHandle; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backends/sdl2_renderer.cpp` around lines 204 - 218, The code currently overwrites the incoming font parameter by unconditionally setting font = ctx.mDefaultFont;—remove that unconditional assignment and instead only fall back to ctx.mDefaultFont when the passed-in font is null: i.e., if (font == nullptr) { if (ctx.mDefaultFont == nullptr && ctx.mStyle.mFont.mName != nullptr) { loadFont(ctx); if (ctx.mDefaultFont == nullptr) { ctx.mLogger(LogSeverity::Error, ("Cannot load font: " + std::string(ctx.mStyle.mFont.mName) + ", using the default font.").c_str()); return InvalidHandle; } } font = ctx.mDefaultFont; } This preserves a valid passed font, attempts to load the default only when needed (using loadFont(ctx) and ctx.mDefaultFont), and returns InvalidHandle if no font is available.
🧹 Nitpick comments (2)
src/widgets.h (1)
56-62: Consider specifyingint32_tas the underlying type for the enum.As per coding guidelines, enums should use
int32_tas the underlying type:enum class KeyInputType : int32_t.♻️ Suggested change
-enum class KeyInputType { +enum class KeyInputType : int32_t { Invalid = -1, ///< Not initialized Character, ///< A character input Password, ///< A special key input Numeric, ///< A numeric input Count ///< The number of key input types };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/widgets.h` around lines 56 - 62, The enum class KeyInputType currently has no explicit underlying type; update its declaration to use int32_t as the underlying type to follow coding guidelines (declare as enum class KeyInputType : int32_t). Modify the KeyInputType declaration so all enumerators (Invalid, Character, Password, Numeric, Count) remain the same but the enum is explicitly typed as int32_t; include the necessary <cstdint> include if not already present.src/widgets.cpp (1)
249-258: Consider usingautofor the type deduction.Static analysis suggests replacing
Context *ctxwithauto *ctxat line 254 since the type is already clear from thestatic_cast.♻️ Minor cleanup
static int inputHandler(Id id, void *instance) { if (instance == nullptr) { return ErrorCode; } - Context *ctx = static_cast<Context *>(instance); + auto *ctx = static_cast<Context *>(instance); ctx->mFocus = Widgets::findWidget(id, ctx->mRoot); return ResultOk; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/widgets.cpp` around lines 249 - 258, Replace the explicit pointer type with automatic type deduction in inputHandler to match the static_cast; change the declaration "Context *ctx = static_cast<Context *>(instance);" to use "auto *ctx = static_cast<Context *>(instance);" so the pointer type is inferred while preserving the cast and subsequent use of ctx and Widgets::findWidget(id, ctx->mRoot) in the function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/backends/sdl2_iodevice.h`:
- Around line 46-48: The delay(uint32_t ms) declaration conflicts with project
conventions; change its signature to return ret_code (int32_t) and accept an
explicit-width signed int (int32_t) so it becomes ret_code delay(int32_t ms),
update the corresponding implementation (method named delay) to validate the
input at the start (e.g., check ms >= 0), return an appropriate ret_code on
invalid input, and ensure callers and any header/implementation pairs are
updated to match the new signature.
In `@src/widgets.cpp`:
- Line 281: The assignment to widget->mCallback uses new CallbackI(inputHandler,
(void *)&ctx, Events::MouseButtonDownEvent) but omits the corresponding
reference increment; update the widget creation logic so that after assigning
widget->mCallback (the CallbackI instance created with inputHandler and ctx for
Events::MouseButtonDownEvent) you call the callback's incRef() method (mirroring
the pattern used in button and treeView creation) to ensure proper reference
counting and avoid premature deletion or double-free.
- Around line 153-164: The KeyDown handling can crash because
ctx.mFocus->mText.erase(ctx.mFocus->mText.size() - 1) is called without checking
for empty; modify the Events::KeyDownEvent branch inside the handler to first
early-return or continue if eventPayload is null or ctx.mFocus is null or
ctx.mFocus->mType != WidgetType::InputField, then when handling SDLK_BACKSPACE
check if ctx.mFocus->mText.empty() before calling mText.erase (or use
pop_back()), otherwise append the character from eventPayload->payload[0]; this
both prevents underflow/UB and reduces nesting.
In `@src/widgets.h`:
- Around line 204-207: The parameter name `default` in the function declaration
inputText(Id id, Id parentId, const Rect &rect, Alignment alignment,
KeyInputType type, const char *default) is a C++ keyword and causes compile
errors; rename that parameter to a non-reserved identifier like defaultText or
initialText in the declaration and in every implementation/usage (e.g., the
inputText definition, any calls, and related header/CPP references) so the
function signature becomes inputText(..., const char *defaultText) and all
references are updated accordingly.
---
Outside diff comments:
In `@src/backends/sdl2_renderer.cpp`:
- Around line 204-218: The code currently overwrites the incoming font parameter
by unconditionally setting font = ctx.mDefaultFont;—remove that unconditional
assignment and instead only fall back to ctx.mDefaultFont when the passed-in
font is null: i.e., if (font == nullptr) { if (ctx.mDefaultFont == nullptr &&
ctx.mStyle.mFont.mName != nullptr) { loadFont(ctx); if (ctx.mDefaultFont ==
nullptr) { ctx.mLogger(LogSeverity::Error, ("Cannot load font: " +
std::string(ctx.mStyle.mFont.mName) + ", using the default font.").c_str());
return InvalidHandle; } } font = ctx.mDefaultFont; } This preserves a valid
passed font, attempts to load the default only when needed (using loadFont(ctx)
and ctx.mDefaultFont), and returns InvalidHandle if no font is available.
In `@src/widgets.cpp`:
- Around line 260-283: The function Widgets::inputText uses a parameter named
"default", which is a reserved C++ keyword; rename that parameter to match the
header change (e.g., "defaultText") in the Widgets::inputText signature and all
uses inside the implementation (including the null check and
widget->mText.assign call) so the definition matches the header and compiles
cleanly.
---
Nitpick comments:
In `@src/widgets.cpp`:
- Around line 249-258: Replace the explicit pointer type with automatic type
deduction in inputHandler to match the static_cast; change the declaration
"Context *ctx = static_cast<Context *>(instance);" to use "auto *ctx =
static_cast<Context *>(instance);" so the pointer type is inferred while
preserving the cast and subsequent use of ctx and Widgets::findWidget(id,
ctx->mRoot) in the function.
In `@src/widgets.h`:
- Around line 56-62: The enum class KeyInputType currently has no explicit
underlying type; update its declaration to use int32_t as the underlying type to
follow coding guidelines (declare as enum class KeyInputType : int32_t). Modify
the KeyInputType declaration so all enumerators (Invalid, Character, Password,
Numeric, Count) remain the same but the enum is explicitly typed as int32_t;
include the necessary <cstdint> include if not already present.
🪄 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: b552a7df-7487-4f01-99a0-9ba73f58e3e5
📒 Files selected for processing (6)
samples/demo/main.cppsrc/backends/sdl2_iodevice.cppsrc/backends/sdl2_iodevice.hsrc/backends/sdl2_renderer.cppsrc/widgets.cppsrc/widgets.h
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Features:
What is missing
Summary by CodeRabbit
New Features
Bug Fixes