Repository navigation
Add checkbox + demo - #44
Conversation
WalkthroughThe PR adds checkbox state storage, rendering, mouse-button toggling, and demo placement. Existing factory signatures and callback wiring remain unchanged. ChangesCheckbox support
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Widgets
participant Widget
User->>Widgets: click checkbox
Widgets->>Widget: toggle checked state
Widgets-->>User: invoke existing callback
Widgets->>Widget: read label and checked state
Widgets-->>User: render checkbox and label
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: 4
🧹 Nitpick comments (1)
src/widgets.h (1)
81-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
///@brief`` documentation forCheckBoxContext.
CheckBoxContextis a new public type, but the declaration has no type-level brief. ThemCheckedfield uses a trailing///<comment instead of the requested///@brief`` form. Add brief comments for both declarations.Proposed documentation fix
+/// `@brief` Stores the checked state of a checkbox. struct CheckBoxContext { - bool mChecked{false}; ///< The checked state of the checkbox. + /// `@brief` The checked state of the checkbox. + bool mChecked{false}; };As per coding guidelines, use Doxygen-style
///@brief`` comments for documentation.🤖 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 `@src/widgets.h` around lines 81 - 84, Add Doxygen `/// `@brief`` comments for the public `CheckBoxContext` struct and its `mChecked` member, replacing the existing trailing `///<` field comment with the requested form while preserving the documented checked-state meaning.Source: Coding guidelines
🤖 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 `@src/widgets.cpp`:
- Around line 594-598: Update the unchecked branch in the checkbox rendering
logic around mCheckBoxContext->mChecked to preserve a visible boundary: retain
the background fill, then draw an outline around checkBoxRect using the
appropriate foreground or border style. Leave the checked-state rendering
unchanged.
- Around line 508-512: Update the checkbox factory implementation in
Widgets::checkBox to assign its callback parameter to child->mCallback and
increment the callback’s reference count consistently with other widget
factories, so Widgets::onMouseButton can dispatch it.
- Around line 509-510: Update Widget::~Widget() to release the CheckBoxContext
allocated by Widgets::checkBox before destruction, ensuring every
mCheckBoxContext allocation has exactly one owner and is freed safely; preserve
existing checkbox behavior.
- Line 508: Update Widgets::checkBox() to validate the text pointer before
calling child->mText.assign(text). Handle nullptr input safely while preserving
the existing checkbox creation behavior for valid labels.
---
Nitpick comments:
In `@src/widgets.h`:
- Around line 81-84: Add Doxygen `/// `@brief`` comments for the public
`CheckBoxContext` struct and its `mChecked` member, replacing the existing
trailing `///<` field comment with the requested form while preserving the
documented checked-state meaning.
🪄 Autofix
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 Plus
Run ID: 354de63b-300e-4006-8f91-900626d3c5f7
📒 Files selected for processing (3)
samples/demo/main.cppsrc/widgets.cppsrc/widgets.h
| if (child == nullptr) { | ||
| return WidgetHandle{WidgetHandle::InvalidId}; | ||
| } | ||
| child->mText.assign(text); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 'mText\.assign\(text\)|text != nullptr' src/widgets.cppRepository: kimkulling/tiny_ui
Length of output: 2099
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant functions and header declarations to determine whether the guard changes semantics or API expectations.
sed -n '350,520p' src/widgets.cpp | cat -n
printf '\n--- declarations/usages ---\n'
rg -n -C 3 'createCheckBox|CheckBox|void\s+draw.*(CheckBox|checkBox|checkbox)|WidgetHandle\s+create' srcRepository: kimkulling/tiny_ui
Length of output: 10062
🌐 Web query:
C++ std::string assign const char* nullptr behavior
💡 Result:
In modern C++ (C++23 and later), attempting to assign a nullptr to a std::string is ill-formed and will result in a compiler error [1]. This is because the C++23 standard added a deleted assignment operator: std::basic_string& operator=(std::nullptr_t) = delete; [2][1]. For older versions of C++ (C++11 through C++20), assigning nullptr to a std::string was syntactically valid but constituted undefined behavior [3][4][1]. Historically, this occurred because std::string provided constructors and assignment operators that accepted a const char* pointer [1]. Since nullptr implicitly converts to any pointer type, it matched these overloads, causing the string to attempt to read from a null memory address, which resulted in undefined behavior [3][1]. If you are working with an older C++ standard, you must ensure that any pointer passed to std::string::assign or used in an assignment is non-null [3]. You should explicitly check for null pointers before performing the assignment or construction [3].
Citations:
- 1: https://stackoverflow.com/questions/30268904/why-can-you-assign-nullptr-to-stdstring
- 2: https://cppreference.dev/w/cpp/string/basic_string/operator%3D
- 3: https://stackoverflow.com/questions/34901903/is-it-valid-to-pass-nullptr-to-stdstringassign
- 4: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2020/p2166r1.html
Guard text before assigning the checkbox label.
Widgets::checkBox() calls child->mText.assign(text) with a const char* that can be nullptr. Guard invalid text before assignment so other callers cannot crash this factory.
Proposed input guard
- child->mText.assign(text);
+ if (text != nullptr) {
+ child->mText.assign(text);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| child->mText.assign(text); | |
| if (text != nullptr) { | |
| child->mText.assign(text); | |
| } |
🤖 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 `@src/widgets.cpp` at line 508, Update Widgets::checkBox() to validate the text
pointer before calling child->mText.assign(text). Handle nullptr input safely
while preserving the existing checkbox creation behavior for valid labels.
Source: Coding guidelines
| child->mText.assign(text); | ||
| child->mCheckBoxContext = new CheckBoxContext; | ||
| child->mCheckBoxContext->mChecked = checked; | ||
|
|
||
|
|
||
| return child->mHandle; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Wire the callback parameter into the checkbox widget.
The public factory documents CallbackI *callback in src/widgets.h, Lines 271-279, but Widgets::checkBox never assigns it to child->mCallback or increments its reference count. Widgets::onMouseButton dispatches found->mCallback at Lines 678-680. A callback passed to checkBox therefore never runs.
Proposed callback wiring
child->mCheckBoxContext = new CheckBoxContext;
child->mCheckBoxContext->mChecked = checked;
+ child->mCallback = callback;
+ if (callback != nullptr) {
+ callback->incRef();
+ }
return child->mHandle;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| child->mText.assign(text); | |
| child->mCheckBoxContext = new CheckBoxContext; | |
| child->mCheckBoxContext->mChecked = checked; | |
| return child->mHandle; | |
| child->mText.assign(text); | |
| child->mCheckBoxContext = new CheckBoxContext; | |
| child->mCheckBoxContext->mChecked = checked; | |
| child->mCallback = callback; | |
| if (callback != nullptr) { | |
| callback->incRef(); | |
| } | |
| return child->mHandle; |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 509-509: Replace the use of "new" with an operation that automatically manages the memory.
🤖 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 `@src/widgets.cpp` around lines 508 - 512, Update the checkbox factory
implementation in Widgets::checkBox to assign its callback parameter to
child->mCallback and increment the callback’s reference count consistently with
other widget factories, so Widgets::onMouseButton can dispatch it.
| child->mCheckBoxContext = new CheckBoxContext; | ||
| child->mCheckBoxContext->mChecked = checked; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Release each allocated CheckBoxContext.
Widgets::checkBox allocates CheckBoxContext at Line 509. Widget::~Widget() in src/widgets.h, Lines 112-120, does not release mCheckBoxContext. Clearing a checkbox therefore leaks memory for every checkbox lifetime. Release the context in Widget::~Widget(), or replace the raw pointer with an owning value or RAII member.
Minimal lifetime fix
if (mContent != nullptr) {
delete [] mContent;
}
+ if (mCheckBoxContext != nullptr) {
+ delete mCheckBoxContext;
+ }🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 509-509: Replace the use of "new" with an operation that automatically manages the memory.
🤖 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 `@src/widgets.cpp` around lines 509 - 510, Update Widget::~Widget() to release
the CheckBoxContext allocated by Widgets::checkBox before destruction, ensuring
every mCheckBoxContext allocation has exactly one owner and is freed safely;
preserve existing checkbox behavior.
Source: Linters/SAST tools
| if (currentWidget->mCheckBoxContext->mChecked) { | ||
| Renderer::drawRect(ctx, checkBoxRect.top.x, checkBoxRect.top.y, checkBoxRect.width, checkBoxRect.height, true, ctx.mStyle.mFg); | ||
| } else { | ||
| Renderer::drawRect(ctx, checkBoxRect.top.x, checkBoxRect.top.y, checkBoxRect.width, checkBoxRect.height, true, ctx.mStyle.mBg); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep the unchecked box visible.
The unchecked branch fills the square with ctx.mStyle.mBg and draws no outline. If the surrounding background uses the same color, the checkbox has no visible boundary. Draw an outline for the unchecked state.
Proposed unchecked rendering
- Renderer::drawRect(ctx, checkBoxRect.top.x, checkBoxRect.top.y, checkBoxRect.width, checkBoxRect.height, true, ctx.mStyle.mBg);
+ Renderer::drawRect(ctx, checkBoxRect.top.x, checkBoxRect.top.y, checkBoxRect.width, checkBoxRect.height, false, ctx.mStyle.mFg);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (currentWidget->mCheckBoxContext->mChecked) { | |
| Renderer::drawRect(ctx, checkBoxRect.top.x, checkBoxRect.top.y, checkBoxRect.width, checkBoxRect.height, true, ctx.mStyle.mFg); | |
| } else { | |
| Renderer::drawRect(ctx, checkBoxRect.top.x, checkBoxRect.top.y, checkBoxRect.width, checkBoxRect.height, true, ctx.mStyle.mBg); | |
| } | |
| if (currentWidget->mCheckBoxContext->mChecked) { | |
| Renderer::drawRect(ctx, checkBoxRect.top.x, checkBoxRect.top.y, checkBoxRect.width, checkBoxRect.height, true, ctx.mStyle.mFg); | |
| } else { | |
| Renderer::drawRect(ctx, checkBoxRect.top.x, checkBoxRect.top.y, checkBoxRect.width, checkBoxRect.height, false, ctx.mStyle.mFg); | |
| } |
🤖 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 `@src/widgets.cpp` around lines 594 - 598, Update the unchecked branch in the
checkbox rendering logic around mCheckBoxContext->mChecked to preserve a visible
boundary: retain the background fill, then draw an outline around checkBoxRect
using the appropriate foreground or border style. Leave the checked-state
rendering unchanged.
2 participants
Summary by CodeRabbit