37 id concept the id as a unique identifier does not scale#39
Merged
kimkulling merged 7 commits intomainfrom Apr 29, 2026
Merged
Conversation
WalkthroughThe pull request migrates the widget identification system from raw integer IDs to a strongly-typed Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/widgets.h (1)
162-272: 🛠️ Refactor suggestion | 🟠 MajorKeep error reporting separate from the returned handle.
These factory APIs now return only
WidgetHandle, so callers lose the distinction between invalid context, invalid parent, and generic creation failure. Consider keepingret_codeas the return type and writing the generated handle to an out-parameter, then update the Doxygen blocks to match the new contract.As per coding guidelines, "Use ret_code (int32_t) for return values".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/widgets.h` around lines 162 - 272, The factory methods (e.g., container, box, imageBox, label, inputText, textButton, imageButton, panel, treeView, treeItem, progressBar) currently return WidgetHandle which hides error codes; change their signature to return ret_code (int32_t) and add an out-parameter like WidgetHandle *outHandle to receive the created handle, update each implementation to return distinct error codes (invalid context/parent/generic failure) while writing the handle to outHandle on success, and update the Doxygen comments for each affected function to document the new return type and the out-parameter; keep findWidget/findSelectedWidget unchanged.src/widgets.cpp (4)
467-473:⚠️ Potential issue | 🟠 MajorPersist the requested initial fill state.
fillRateis clamped intostate.filledState, but that value is never copied intopayload.payload. New progress bars therefore start at0until the update callback mutates them.💡 Copy the initial state into the payload
FilledState state; state.filledState = fillRate; child->mContent = new uint8_t[sizeof(EventPayload)]; child->mCallback = callback; EventPayload payload; payload.type = EventDataType::FillState; + std::memcpy(payload.payload, &state, sizeof(state)); memcpy(child->mContent, &payload, sizeof(EventPayload));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/widgets.cpp` around lines 467 - 473, The initial clamped fill value in FilledState (state.filledState) is never copied into the EventPayload, so new progress bars start at 0; before copying payload into child->mContent assign the initial value (e.g., payload.payload = state.filledState or memcpy into payload.payload) so EventPayload contains the requested fill rate, then memcpy(child->mContent, &payload, sizeof(EventPayload)); ensure you reference FilledState, fillRate, EventPayload, and child->mContent when making the change.
129-136:⚠️ Potential issue | 🔴 CriticalDelete orphan widgets when parent resolution fails.
If
setParent()returnsnullptr, this function still returns a freshWidget*with a valid-looking handle. Every factory built on top of this can then leak the allocation and hand back a handle for a widget that is not attached to the tree.💡 Minimal fix in this helper
static Widget *createWidget(Context &ctx, WidgetHandle parentId, const Rect &rect, WidgetType type) { auto *widget = new Widget; widget->mHandle = WidgetHandle{createHandle()}; widget->mType = type; widget->mRect = rect; widget->mParent = setParent(ctx, widget, parentId); + if (widget->mParent == nullptr) { + delete widget; + return nullptr; + } return widget; }Each caller should then translate
nullptrintoWidgetHandle{WidgetHandle::InvalidId}.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/widgets.cpp` around lines 129 - 136, createWidget currently allocates a Widget and always returns it even if setParent(ctx, widget, parentId) fails, producing orphan widgets and leaking memory; change createWidget so that after calling setParent(...) you check if it returned nullptr, and if so delete the newly allocated Widget and return nullptr instead of the orphaned pointer (keep creating the handle via createHandle and assigning mType/mRect only after parent success or if you prefer create early, still delete on failure); callers of createWidget (which expect a Widget* to be turned into a WidgetHandle) should then treat a nullptr return as WidgetHandle{WidgetHandle::InvalidId}.
714-716:⚠️ Potential issue | 🟠 MajorHonor the
recursiveflag inclearItem().The function always deletes every child subtree, even when
recursiveisfalse. That breaks the documented contract and can remove more UI than the caller asked for.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/widgets.cpp` around lines 714 - 716, clearItem() currently ignores the recursive flag and always calls recursiveClear on every child (the loop calling recursiveClear(widget->mChildren[i])), removing entire subtrees; change the logic in clearItem() so that when the recursive parameter is false it only removes direct child items (e.g., delete or clear widget->mChildren[i] without descending) and when recursive is true it calls recursiveClear for each child as before; locate the clearItem function and the loop that invokes recursiveClear and branch on the recursive parameter to perform non-recursive versus recursive deletion accordingly.
474-476:⚠️ Potential issue | 🟠 MajorUpdate callbacks outlive deleted widgets.
This registers the callback in
Context::mUpdateCallbackList, but widget teardown does not remove it. After a progress bar is cleared,TinyUi::run()can still invoke the stale callback whilemInstancepoints at freed widget memory.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/widgets.cpp` around lines 474 - 476, The update callback registered into Context::mUpdateCallbackList from the snippet (callback->mInstance = child) can outlive the widget and later be invoked by TinyUi::run() with mInstance pointing at freed memory; fix this by removing or nulling callbacks during widget teardown: add logic in the widget destructor/clear routine (or a Widget::destroy/clear method) to call a Context method (e.g., Context::removeUpdateCallback or similar) to erase any entries whose mInstance equals this, or alternatively set callback->mInstance = nullptr for all callbacks owned by the widget so TinyUi::run() can skip them, and ensure Context::mUpdateCallbackList iteration checks for nullptr mInstance before invoking.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@samples/hello_world/main.cpp`:
- Around line 51-56: Check the WidgetHandle returned by Widgets::panel (the
local variable panel) for validity before creating child widgets; if the handle
is invalid, log or return early instead of calling Widgets::label or
Widgets::textButton. Move creation of dynamicQuitCallback (CallbackI) and any
subsequent UI additions (Widgets::label, Widgets::textButton) to after the
validity check so you don't allocate callbacks or build UI under an invalid
parent, and ensure you handle/cleanup the callback on early exit.
In `@src/tinyui.cpp`:
- Around line 148-149: The code is passing a bogus WidgetHandle{1} into every
update callback; replace the synthesized constant with the widget's real handle
(e.g. use the widget object's handle member or accessor rather than
WidgetHandle{1}) so callbacks get the actual id. Specifically, in the call site
that constructs WidgetHandle handle{1} and invokes
(*it)->mfuncCallback[Events::UpdateEvent](handle, (*it)->mInstance), use the
widget's real handle (for example (*it)->mHandle or (*it)->getHandle()) and
ensure the type matches WidgetHandle (note WidgetHandle::getRootHandle() is 0,
so using 1 is incorrect).
In `@src/tinyui.h`:
- Around line 101-108: InvalidId is currently set to 999999 which can collide
with generated Ids; change the sentinel to an out-of-band value (e.g. use the
maximum representable Id via std::numeric_limits<Id>::max()) or alternatively
add a separate validity flag (e.g. bool mValid) to track handle validity, then
update the isValid() method to check the new sentinel or the new flag; update
references to InvalidId, mId, and isValid() accordingly (or add mValid and
set/reset it where handles are created/destroyed).
In `@src/widgets.cpp`:
- Around line 173-185: The code currently overwrites ctx.mRoot with the newly
created container (Widgets::container -> ctx.mRoot = widget), which can replace
a synthetic root created by TinyUi::getContext()/getValidRoot() and leave
widget->mParent dangling; change the logic so you only set ctx.mRoot when it was
previously null (i.e., if (ctx.mRoot == nullptr) ctx.mRoot = widget;) or simply
remove the unconditional assignment, leaving createWidget/getValidRoot to attach
the new container under the existing root and preserving widget->mParent.
---
Outside diff comments:
In `@src/widgets.cpp`:
- Around line 467-473: The initial clamped fill value in FilledState
(state.filledState) is never copied into the EventPayload, so new progress bars
start at 0; before copying payload into child->mContent assign the initial value
(e.g., payload.payload = state.filledState or memcpy into payload.payload) so
EventPayload contains the requested fill rate, then memcpy(child->mContent,
&payload, sizeof(EventPayload)); ensure you reference FilledState, fillRate,
EventPayload, and child->mContent when making the change.
- Around line 129-136: createWidget currently allocates a Widget and always
returns it even if setParent(ctx, widget, parentId) fails, producing orphan
widgets and leaking memory; change createWidget so that after calling
setParent(...) you check if it returned nullptr, and if so delete the newly
allocated Widget and return nullptr instead of the orphaned pointer (keep
creating the handle via createHandle and assigning mType/mRect only after parent
success or if you prefer create early, still delete on failure); callers of
createWidget (which expect a Widget* to be turned into a WidgetHandle) should
then treat a nullptr return as WidgetHandle{WidgetHandle::InvalidId}.
- Around line 714-716: clearItem() currently ignores the recursive flag and
always calls recursiveClear on every child (the loop calling
recursiveClear(widget->mChildren[i])), removing entire subtrees; change the
logic in clearItem() so that when the recursive parameter is false it only
removes direct child items (e.g., delete or clear widget->mChildren[i] without
descending) and when recursive is true it calls recursiveClear for each child as
before; locate the clearItem function and the loop that invokes recursiveClear
and branch on the recursive parameter to perform non-recursive versus recursive
deletion accordingly.
- Around line 474-476: The update callback registered into
Context::mUpdateCallbackList from the snippet (callback->mInstance = child) can
outlive the widget and later be invoked by TinyUi::run() with mInstance pointing
at freed memory; fix this by removing or nulling callbacks during widget
teardown: add logic in the widget destructor/clear routine (or a
Widget::destroy/clear method) to call a Context method (e.g.,
Context::removeUpdateCallback or similar) to erase any entries whose mInstance
equals this, or alternatively set callback->mInstance = nullptr for all
callbacks owned by the widget so TinyUi::run() can skip them, and ensure
Context::mUpdateCallbackList iteration checks for nullptr mInstance before
invoking.
In `@src/widgets.h`:
- Around line 162-272: The factory methods (e.g., container, box, imageBox,
label, inputText, textButton, imageButton, panel, treeView, treeItem,
progressBar) currently return WidgetHandle which hides error codes; change their
signature to return ret_code (int32_t) and add an out-parameter like
WidgetHandle *outHandle to receive the created handle, update each
implementation to return distinct error codes (invalid context/parent/generic
failure) while writing the handle to outHandle on success, and update the
Doxygen comments for each affected function to document the new return type and
the out-parameter; keep findWidget/findSelectedWidget unchanged.
🪄 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: eb895a24-9752-49fd-9555-5f25df37d364
📒 Files selected for processing (6)
samples/demo/main.cppsamples/hello_world/main.cppsrc/tinyui.cppsrc/tinyui.hsrc/widgets.cppsrc/widgets.h
|
kimkulling
deleted the
37-id-concept-the-id-as-a-unique-identifier-does-not-scale
branch
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
2 participants
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.
Summary by CodeRabbit