Just do it #12
Just do it #12
Conversation
WalkthroughThis update introduces several enhancements and refactorings to the UI framework. It adds a new Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Context
participant Widgets
participant Widget(TextField)
App->>Widgets: textfield(ctx, id, parentId, rect, alignment)
Widgets->>Context: Add new TextField widget
App->>Widgets: setFocus(ctx, id)
Widgets->>Context: Set mFocus to widget with id
App->>Widgets: onKey(ctx, key, isDown)
Widgets->>Widgets: eventDispatcher(ctx, KeyDownEvent, eventData)
Widgets->>Context: Check mFocus
Widgets->>Widget(TextField): Append key data to text (if focused widget is TextField)
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
![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 (7)
src/widgets.h (3)
39-40: Missing documentation for TextField widget type.The TextField widget type has been added, but its documentation comment is empty, unlike other widget types that have descriptive comments.
- TextField, ///< + TextField, ///< A text field widget for user input
197-198: Missing documentation for textfield creation method.The new textfield method lacks documentation comments, which is inconsistent with the documentation style of other widget creation methods.
- static ret_code textfield(Context &ctx, Id id, Id parentId, const Rect &rect, Alignment alignment); + /// @brief Creates a new widget from the type text field. + /// @param ctx The context to create the widget in. + /// @param id The unique id of the widget. + /// @param parentId The parent id of the widget. + /// @param rect The rect of the widget. + /// @param alignment The alignment of the widget. + /// @return ResultOk if the widget was created, ErrorCode if not. + static ret_code textfield(Context &ctx, Id id, Id parentId, const Rect &rect, Alignment alignment);
283-284: Missing documentation for setFocus method.The new setFocus method lacks documentation comments, which is inconsistent with the documentation style of other methods in this class.
- static ret_code setFocus(Context &ctx, Id id); + /// @brief Sets focus on the widget with the specified id. + /// @param ctx The context to set the focus in. + /// @param id The id of the widget to focus. + /// @return ResultOk if the focus was set, ErrorCode if not. + static ret_code setFocus(Context &ctx, Id id);src/widgets.cpp (4)
71-73: Use a wider type forpitchto avoid overflow
w * bytesPerPixelcan overflow anint32_tfor large textures (e.g. 16-bit pixels @ 16k width). Considerstd::size_tor at leastuint64_t.-int32_t pitch = w * bytesPerPixel; +std::size_t pitch = static_cast<std::size_t>(w) * bytesPerPixel;
206-219: TextField creation looks good but lacks initial text support
textfield()mirrorslabel()yet does not allow an initial text string. Consider adding aconst char *textparameter so callers can pre-populate the field, keeping the API symmetrical withlabel()andbutton().
453-458: Stack-allocatedEventDatais fine, but ensure null-termination
Only the first byte is filled; downstream code now correctlypush_backs one char (after previous fix). If you later decide to keep usingappend, remember to NULL-terminate here.eventData.data[0] = *key; eventData.data[1] = 0; // optional future-proofing
505-514: Restore previous focus & early-out when already focused
setFocus()overwritesctx.mFocuswithout clearing any previous visual focus indication and does extra work when the widget is already focused.- ctx.mFocus = widget; + if (ctx.mFocus == widget) { + return ResultOk; // nothing to change + } + ctx.mFocus = widget; + // TODO: optionally trigger a redraw / focus-changed callback here
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
samples/main.cpp(4 hunks)src/backends/sdl2_iodevice.cpp(1 hunks)src/backends/sdl2_iodevice.h(1 hunks)src/backends/sdl2_renderer.cpp(4 hunks)src/backends/sdl2_renderer.h(2 hunks)src/tinyui.cpp(1 hunks)src/tinyui.h(4 hunks)src/widgets.cpp(7 hunks)src/widgets.h(5 hunks)
🔇 Additional comments (19)
src/backends/sdl2_iodevice.h (2)
4-4: Copyright year corrected.The copyright year was updated from "2022-2025" to "2022-2024" to reflect the correct current year range.
34-35: Documentation comments removed.The Doxygen-style comments for the destructor and
updatemethod were removed while maintaining the functionality. This appears to be part of a broader documentation cleanup across the codebase.src/backends/sdl2_iodevice.cpp (1)
4-4: Copyright year corrected.The copyright year was updated from "2022-2025" to "2022-2024" to reflect the correct current year range.
src/tinyui.h (4)
4-4: Copyright year corrected.The copyright year was updated from "2022-2025" to "2022-2024" to reflect the correct current year range.
253-256: New keyboard event types added.Added key events to support keyboard input handling, which enables interaction with the new TextField widget mentioned in the summary.
259-262: Added EventData struct for event information.The new EventData struct provides a fixed-size buffer to store event-specific data, which will be necessary for passing keyboard input characters to widgets.
336-336: Added widget focus tracking to Context.Added a pointer to track the currently focused widget, which is essential for directing keyboard events to the appropriate widget (specifically the TextField mentioned in the summary).
Also applies to: 363-363
src/backends/sdl2_renderer.h (1)
4-4: Copyright year corrected.The copyright year was updated from "2022-2025" to "2022-2024" to reflect the correct current year range.
src/tinyui.cpp (1)
4-4: Copyright year updated correctly.The copyright year has been updated from 2022-2025 to 2022-2024, which correctly reflects the current year.
samples/main.cpp (4)
4-4: Copyright year updated correctly.The copyright year has been updated from 2022-2025 to 2022-2024, which correctly reflects the current year.
28-30: Good use of constants for panel IDs.Using named constants instead of magic numbers improves code readability and maintainability.
65-65: Panel height increased to accommodate new UI elements.The panel height has been increased from 400 to 500 pixels, which makes sense to accommodate the new textfield widget that is added below other elements.
78-79: Successfully implemented new TextField widget.The textfield widget is properly positioned below the progressBar and uses the newly implemented widget functionality with appropriate parameters.
src/backends/sdl2_renderer.cpp (4)
4-4: Copyright year updated correctly.The copyright year has been updated from 2022-2025 to 2022-2024, which correctly reflects the current year.
65-66: Improved function signature for queryDriver.The function now accepts a raw C-string pointer and maximum length parameter instead of std::string_view, which can be more efficient for C-style string handling in this context.
76-80: Added proper string length bounds checking.Good security practice - string length is now calculated and limited to the provided maxLen parameter before the comparison, preventing potential buffer overruns.
270-270: Updated function call to match new signature.The call to queryDriver has been correctly updated to pass a C-string literal and maximum length value.
src/widgets.h (2)
4-4: Copyright year updated correctly.The copyright year has been updated from 2022-2025 to 2022-2024, which correctly reflects the current year.
148-148: Modernized root item constant declaration.Changed RootItem from
static consttostatic constexpr, which is a best practice in modern C++ for compile-time constants.
|
|
||
| struct SurfaceImpl { | ||
| SDL_Surface *mSurface = nullptr; | ||
| SDL_Surface *mSurface; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Removed nullptr initialization for mSurface.
The explicit nullptr initialization for the mSurface member was removed. While the destructor and clear() method check for nullptr before freeing, there's a risk of undefined behavior if the member is accessed before being properly initialized.
- SDL_Surface *mSurface;
+ SDL_Surface *mSurface = nullptr;📝 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.
| SDL_Surface *mSurface; | |
| - SDL_Surface *mSurface; | |
| + SDL_Surface *mSurface = nullptr; |
| void eventDispatcher(Context &ctx, int32_t eventId, EventData *eventData) { | ||
| if (ctx.mFocus == nullptr) { | ||
| return; | ||
| } | ||
|
|
||
| if (eventId == Events::KeyDownEvent || eventId == Events::KeyUpEvent) { | ||
| if (eventData != nullptr) { | ||
| if (ctx.mFocus->mType == WidgetType::TextField) { | ||
| ctx.mFocus->mText.append((const char*)eventData->data[0]); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Unsafe cast when appending a single key to mText
eventData->data[0] is a single byte, yet it is re-interpreted as a C-string:
ctx.mFocus->mText.append((const char*)eventData->data[0]);This is undefined behaviour and usually crashes because the pointer is interpreted as an address. Append the character itself:
- ctx.mFocus->mText.append((const char*)eventData->data[0]);
+ ctx.mFocus->mText.push_back(static_cast<char>(eventData->data[0]));Additional thought: you probably want to filter non-printable characters and support UTF-8.
| image = new Image; | ||
| if (image == nullptr) { | ||
| return nullptr; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent memory-leak when stbi_load fails
image is allocated but never freed if stbi_load returns nullptr. This leaks every time an image cannot be loaded.
- unsigned char *data = stbi_load(filename, &w, &h, &bytesPerPixel, 0);
- if (data == nullptr) {
- return nullptr;
+ unsigned char *data = stbi_load(filename, &w, &h, &bytesPerPixel, 0);
+ if (data == nullptr) {
+ delete image; // reclaim the allocation
+ return nullptr;
}📝 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.
| image = new Image; | |
| if (image == nullptr) { | |
| return nullptr; | |
| } | |
| image = new Image; | |
| if (image == nullptr) { | |
| return nullptr; | |
| } | |
| unsigned char *data = stbi_load(filename, &w, &h, &bytesPerPixel, 0); | |
| if (data == nullptr) { | |
| delete image; // reclaim the allocation | |
| return nullptr; | |
| } |
| case WidgetType::TextField: | ||
| { | ||
| Renderer::drawRect(ctx, r.x1, r.y1, r.width, r.height, true, ctx.mStyle.mFg); | ||
| Renderer::drawRect(ctx, r.x1+2, r.y1+2, r.width-4, r.height-4, true, ctx.mStyle.mBorder); | ||
| } | ||
| break; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Render the text inside the TextField
The current rendering draws the box but never shows the user-typed text, making the widget appear empty.
Renderer::drawRect(ctx, r.x1+2, r.y1+2, r.width-4, r.height-4, true, ctx.mStyle.mBorder);
+ if (!currentWidget->mText.empty()) {
+ Color4 fg = ctx.mStyle.mTextColor, bg = ctx.mStyle.mBg;
+ Renderer::drawText(ctx, currentWidget->mText.c_str(),
+ ctx.mSDLContext.mDefaultFont,
+ currentWidget->mRect, fg, bg,
+ currentWidget->mAlignment);
+ }📝 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.
| case WidgetType::TextField: | |
| { | |
| Renderer::drawRect(ctx, r.x1, r.y1, r.width, r.height, true, ctx.mStyle.mFg); | |
| Renderer::drawRect(ctx, r.x1+2, r.y1+2, r.width-4, r.height-4, true, ctx.mStyle.mBorder); | |
| } | |
| break; | |
| case WidgetType::TextField: | |
| { | |
| Renderer::drawRect(ctx, r.x1, r.y1, r.width, r.height, true, ctx.mStyle.mFg); | |
| Renderer::drawRect(ctx, r.x1+2, r.y1+2, r.width-4, r.height-4, true, ctx.mStyle.mBorder); | |
| if (!currentWidget->mText.empty()) { | |
| Color4 fg = ctx.mStyle.mTextColor, bg = ctx.mStyle.mBg; | |
| Renderer::drawText(ctx, | |
| currentWidget->mText.c_str(), | |
| ctx.mSDLContext.mDefaultFont, | |
| currentWidget->mRect, | |
| fg, bg, | |
| currentWidget->mAlignment); | |
| } | |
| } | |
| break; |
Summary by CodeRabbit
New Features
Improvements
Removals
Documentation
Other