Feature: Add progress bar #7
Conversation
|
Warning Rate limit exceeded@kimkulling has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 37 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (4)
WalkthroughThis pull request modifies the widget creation process in the application by introducing a new progress bar widget. It updates the method signatures in Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Widgets as Widgets Class
participant Renderer as SDL2 Renderer
App->>Widgets: progressBar(ctx, id, parentId, rect, fillRate, callback)
Widgets-->>Widgets: Create FilledState
Widgets->>Renderer: Render progress bar
Renderer-->>App: Display progress bar
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 1
🧹 Nitpick comments (3)
src/widgets.cpp (1)
274-288: Memory allocation for progress bar content.Allocating memory for a simple struct works, but be mindful to free it if widgets are destroyed. Also, consider stronger typing (e.g., using a dedicated data pointer / smart pointer) to prevent memory leaks and to clarify ownership.
src/widgets.h (2)
79-79: New mContent field.Adding a generic pointer like
mContentboosts flexibility. However, be sure to document usage, lifetime, and deallocation strategy to avoid confusion or errors.
250-253: New progressBar signature.Renaming and repurposing from status bar to progress bar is clear. If the codebase grows to include multiple bar types (e.g., progress, status, health, etc.), consider an abstraction instead of repurposing one method.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
samples/main.cpp(2 hunks)src/backends/sdl2_renderer.h(0 hunks)src/widgets.cpp(3 hunks)src/widgets.h(5 hunks)
💤 Files with no reviewable changes (1)
- src/backends/sdl2_renderer.h
🔇 Additional comments (7)
samples/main.cpp (2)
61-61: Confirm the new panel height.
Changing the panel height from 300 to 400 may alter the layout significantly; ensure that this larger height is intended and that other elements align or appear as desired in the UI.
71-71: New progress bar addition.
Great addition! This provides a clear visual indicator of progress at 50% fill rate. Consider making this fill rate dynamic if you want real-time updates.
src/widgets.cpp (2)
255-255: TreeView signature updated for callbacks.
Including CallbackI *callback is beneficial for extensibility, but ensure it’s properly tested to avoid regressions if the callback remains uninitialized in certain usage scenarios.
270-273: Introduction of FilledState struct.
This struct clarifies how the progress fill state is stored. If you foresee additional fields (e.g., color gradients, animation settings), keep it flexible.
src/widgets.h (3)
42-42: Renamed StatusBar to ProgressBar.
This is a good move if you no longer need the status bar. If you anticipate reintroducing distinct status bar functionality later, consider keeping it separate. Otherwise, this consolidation avoids confusion.
94-95: Safe callback and content initialization.
Great job keeping them set to nullptr by default. This prevents accidental usage of uninitialized pointers.
237-239: TreeView callback parameter.
Adding this callback helps unify the creation pattern with other widget types. Test thoroughly to ensure the callback is triggered as intended in typical scenarios (expanding, collapsing, selection).
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
samples/main.cpp (1)
62-67: LGTM: Consistent widget creation patternThe widget creation calls correctly use the new Rect-based API. Consider extracting the layout values into named constants for better maintainability.
+// Layout constants +constexpr int PANEL_X = 90; +constexpr int PANEL_Y = 5; +constexpr int PANEL_WIDTH = 120; +constexpr int PANEL_HEIGHT = 400; + Widgets::panel(ctx, RootPanelId, 0, "Sample-Dialog", Rect(90, 5, 120, 400), nullptr);src/widgets.h (1)
225-229: Add parameter validation for fillRateWhile the implementation includes clamping, the header should document the valid range for
fillRate(0-100).-/// @param fillRate The fill rate of the widget. +/// @param fillRate The fill rate of the widget (0-100).src/widgets.cpp (1)
261-273: Enhance type safety and efficiencyConsider these improvements:
- Make FilledState more descriptive with proper type
- Make clamp constexpr for compile-time evaluation
struct FilledState { - uint32_t filledState; + uint32_t fillRate; // Percentage 0-100 }; -template<class T> -inline void clamp(T min, T max, T &value) { +template<typename T> +constexpr void clamp(T min, T max, T &value) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
samples/main.cpp(2 hunks)src/widgets.cpp(6 hunks)src/widgets.h(5 hunks)
🔇 Additional comments (3)
samples/main.cpp (2)
38-39: LGTM: Clean refactor to use Rect parameter
The change correctly implements the new Rect-based widget creation pattern.
71-72: LGTM: Progress bar implementation
The progress bar is correctly initialized with a 50% fill rate. The implementation demonstrates proper usage of the new widget type.
src/widgets.cpp (1)
341-348: Validate fill rate within 0–100
The fill rate calculation could overflow if not properly bounded.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/widgets.h (1)
83-83:⚠️ Potential issueReview memory management for mContent
The new
mContentmember is used to store widget-specific data (like progress bar state), but there's no cleanup in the destructor. This could lead to memory leaks.src/widgets.cpp (1)
345-352:⚠️ Potential issueValidate fill rate within 0–100.
Currently, if
fillRateexceeds 100, the progress bar can overflow. Enforce range checks to avoid drawing out-of-bounds.
🧹 Nitpick comments (2)
src/widgets.h (1)
54-57: Enhance FilledState with proper initialization and validation.The struct could benefit from constructor initialization and value validation.
Consider this implementation:
struct FilledState { + static constexpr uint32_t MIN_STATE = 0; + static constexpr uint32_t MAX_STATE = 100; uint32_t filledState; + + FilledState(uint32_t initial = MIN_STATE) : filledState(std::clamp(initial, MIN_STATE, MAX_STATE)) {} + + void update(uint32_t newState) { + filledState = std::clamp(newState, MIN_STATE, MAX_STATE); + } };src/tinyui.h (1)
306-306: Use ofUpdateCallbackList = std::list<CallbackI*>
Storing callback pointers in a list is acceptable. Ensure that the lifecycle ofCallbackIobjects is properly managed to avoid dangling pointers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
samples/main.cpp(3 hunks)src/tinyui.cpp(1 hunks)src/tinyui.h(5 hunks)src/widgets.cpp(6 hunks)src/widgets.h(6 hunks)
🔇 Additional comments (8)
samples/main.cpp (1)
85-86:⚠️ Potential issueFix callback instance initialization.
The progress bar callback is initialized with a null instance, but the callback handler requires a valid instance to update the progress bar state.
Update the callback initialization:
- CallbackI updateProgressBarCallback(updateProgressbar, nullptr, Events::UpdateEvent); + CallbackI updateProgressBarCallback(updateProgressbar, nullptr); // Instance will be set by progressBar Widgets::progressBar(ctx, 8, RootPanelId, Rect(100, 300, 100, 40), 50, &updateProgressBarCallback);Likely invalid or redundant comment.
src/widgets.cpp (1)
272-293:⚠️ Potential issueAdd proper error handling and memory management.
The progress bar implementation has several memory management issues:
- No error handling for memory allocations
- No cleanup on error paths
- Potential memory leak if callback setup fails
Apply these improvements:
ret_code Widgets::progressBar(Context &ctx, Id id, Id parentId, const Rect &rect, int fillRate, CallbackI *callback) { if (ctx.mSDLContext.mRenderer == nullptr) { ctx.mLogger(LogSeverity::Error, "TUI-Renderer is nullptr."); return ErrorCode; } Widget *child = createWidget(ctx, id, parentId, rect, WidgetType::ProgressBar); if (child == nullptr) { return ErrorCode; } FilledState *state = new FilledState; + if (state == nullptr) { + ctx.mLogger(LogSeverity::Error, "Failed to allocate FilledState"); + return ErrorCode; + } clamp(0, 100, fillRate); state->filledState = fillRate; child->mContent = new uint8_t[sizeof(FilledState)]; + if (child->mContent == nullptr) { + ctx.mLogger(LogSeverity::Error, "Failed to allocate content buffer"); + delete state; + return ErrorCode; + } child->mCallback = callback; memcpy(child->mContent, state, sizeof(FilledState)); + delete state; // Free temporary state after memcpy if (callback != nullptr) { callback->mInstance = child; - ctx.mUpdateCallbackList.push_back(callback); + try { + ctx.mUpdateCallbackList.push_back(callback); + } catch (...) { + delete[] child->mContent; + ctx.mLogger(LogSeverity::Error, "Failed to register callback"); + return ErrorCode; + } } return ResultOk; }Likely invalid or redundant comment.
src/tinyui.cpp (1)
94-98: Validate callback pointers before invoking them
If a callback forEvents::UpdateEventhappens not to be set (e.g., left asnullptrafterclear()), dereferencing it can cause a crash. Consider adding a null check or fallback to avoid undefined behavior:for (auto it = ctx.mUpdateCallbackList.begin(); it != ctx.mUpdateCallbackList.end(); ++it) { - (*it)->mfuncCallback[Events::UpdateEvent](1, (*it)->mInstance); + if ((*it)->mfuncCallback[Events::UpdateEvent] != nullptr) { + (*it)->mfuncCallback[Events::UpdateEvent](1, (*it)->mInstance); + } }✅ Verification successful
Let me gather more information about the callback handling in the codebase to verify this concern.
This will help us understand:
- How callbacks are handled throughout the code
- If there are any clear() implementations that might set callbacks to nullptr
- How callbacks are initialized and assigned
Validate callback pointers before invoking them
The verification confirms that callbacks can indeed be null:
- The
clear()method explicitly sets all callbacks tonullptr- Callbacks are initialized to
nullptrin the constructor- While callbacks are assigned in event registration, there's no guarantee that
Events::UpdateEventcallback is setGiven this evidence, the original review comment's concern about potential null pointer dereference is valid. The suggested null check is necessary to prevent undefined behavior.
Locations needing attention:
src/tinyui.cpp:94-98: Add null check before invoking the update callback🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for callback-related code and initialization patterns rg -U "mfuncCallback.*Events::UpdateEvent" -A 5 -B 5 # Look for any clear() implementations that might affect callbacks ast-grep --pattern 'clear() { $$$ }' # Search for callback initialization or assignment patterns rg "mfuncCallback\[.*\]\s*=\s*" -A 2 -B 2Length of output: 1010
src/tinyui.h (5)
28-28: Include of<list>is appropriate
No issues identified. Using a standard library list to maintain callback pointers is straightforward.
254-255: Introduction of newUpdateEvent
Defining a dedicated update event type and extendingNumEventsis consistent. Make sure that other event-related subsystems gracefully handle this new event.
273-276: Constructor now acceptseventType
This is a flexible approach but be mindful that only one event callback is assigned perCallbackIinstance. If multiple events must be handled by the same instance, consider extending or reusingmfuncCallbackfor each event or creating multipleCallbackIobjects.
315-320: New member fields inContext
The addition ofmUpdateCallbackListis logically consistent with other context-owned resources. Verify that destruction behavior properly cleans up or invalidates any callback pointers.
335-336: Default initialization in the private constructor
InitializingmLoggertonullptrandmUpdateCallbackListto an empty list is clear and safe.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/tinyui.h (4)
254-255: Add documentation for the new UpdateEvent type.While the event type is properly added, it would be helpful to add documentation explaining its purpose and usage in the context of progress bars, similar to other event types in the
Eventsstruct.+ /// @brief Event triggered when progress needs to be updated static constexpr int32_t UpdateEvent = 5;
273-276: Document the new eventType parameter and improve implementation.
- Add documentation for the new parameter.
- Consider using member initializer list for better efficiency.
- CallbackI(funcCallback callbackFunc, void *instance, size_t eventType = Events::MouseButtonDownEvent) : - mfuncCallback{ nullptr }, mInstance(instance) { - clear(); - mfuncCallback[eventType] = callbackFunc; - } + /// @brief Constructor with callback function and event type + /// @param callbackFunc The callback function to register + /// @param instance The instance data for the callback + /// @param eventType The event type to register the callback for + CallbackI(funcCallback callbackFunc, void *instance, size_t eventType = Events::MouseButtonDownEvent) : + mInstance(instance) { + std::fill_n(mfuncCallback, Events::NumEvents, nullptr); + mfuncCallback[eventType] = callbackFunc; + }
306-307: Document the UpdateCallbackList type.Add documentation explaining the purpose of this callback list type in the context of progress bars.
+/// @brief List of callbacks for progress updates using UpdateCallbackList = std::list<CallbackI*>;
306-320: Consider thread safety for progress updates.Since progress bars often involve updates from background operations, consider:
- Adding thread safety mechanisms for the
mUpdateCallbackList.- Documenting thread safety requirements for callback implementations.
- Implementing a thread-safe queue for progress updates if updates come from worker threads.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/tinyui.h(5 hunks)
🔇 Additional comments (1)
src/tinyui.h (1)
28-28: LGTM: Include statement is appropriately placed.The addition of
<list>header is necessary for the newUpdateCallbackListtype.
|
Summary by CodeRabbit
New Features
UI Improvements
Code Cleanup