Feauture/use index for widget list #297
Conversation
ShishkinDmitriy
added
the
enhancement
WalkthroughThe changes primarily involve updating parameter types across multiple files and the Changes
Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (3)
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: 4
🔭 Outside diff range comments (2)
examples/List/List.ino (2)
Line range hint
1-1: High-Level Feedback: Document Breaking ChangesThis PR introduces a significant architectural change in how widget callbacks receive their parameters. Instead of receiving the actual values, callbacks now receive indices into their respective arrays. While this change aligns with the PR objective of making it easier to work with indices, it introduces breaking changes that need to be addressed:
- All widget callbacks need to be updated to handle indices instead of values
- Documentation should be updated to reflect this change
- A migration guide should be provided for users
- Examples should demonstrate both index and value usage patterns
Consider:
- Adding clear documentation about the parameter type changes
- Providing helper methods to convert indices to values
- Including examples that show both index-based and value-based usage
- Adding a migration guide in the PR description
Breaking Change Confirmed: Num widget callback parameter is the array index
The review comment is correct. The
WIDGET_LISTimplementation shows that the callback receives the index (0-4) into thenumsarray, not the actual value. The current callback prints indices (0,1,2,3,4) instead of the intended values (5,7,9,12,32).The suggested fix is appropriate:
-[](const uint8_t num) { Serial.println(num); }, +// num parameter is the index (0-4) into the nums array +[](const uint8_t num) { Serial.println(nums[num]); },🔗 Analysis chain
Line range hint
27-29: Breaking Change: Num widget callback parameter changedThe
numparameter now receives the index (0-4) into thenumsarray instead of the actual number. This is a breaking change that needs documentation.Consider updating the callback to print the actual number:
-[](const uint8_t num) { Serial.println(num); }, +// num parameter is the index (0-4) into the nums array +[](const uint8_t num) { Serial.println(nums[num]); },🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find WIDGET_LIST implementation and related files ast-grep --pattern 'WIDGET_LIST' # Search for documentation or comments about widget list parameters rg -i "widget.*list.*param|widget.*list.*callback" -A 3 -B 3 # Find other examples using similar pattern rg "WIDGET_LIST\s*\(" -A 2 -B 2 # Look for any recent changes or documentation in markdown files fd -e md -x cat {}Length of output: 19495
Script:
#!/bin/bash # Find WidgetList class implementation and callback handling ast-grep --pattern 'class WidgetList' # Look for callback-related code in WidgetList rg "callback" src/widget/WidgetList.h -A 5 -B 5 # Find the definition of the nums array rg "nums\[\]|NUMS_COUNT" examples/List/List.ino -A 2 -B 2Length of output: 2112
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
docs/source/overview/items/item-widget.rst(3 hunks)examples/ButtonWithRotaryAdapter/ButtonWithRotaryAdapter.ino(1 hunks)examples/KeypadShield/KeypadShield.ino(1 hunks)examples/List/List.ino(1 hunks)examples/SimpleRotary/SimpleRotary.ino(1 hunks)examples/Widgets/Widgets.ino(3 hunks)src/widget/WidgetList.h(3 hunks)
🧰 Additional context used
📓 Learnings (1)
src/widget/WidgetList.h (1)
Learnt from: forntoh
PR: forntoh/LcdMenu#255
File: src/widget/WidgetList.h:61-65
Timestamp: 2024-11-12T11:59:37.512Z
Learning: In `src/widget/WidgetList.h`, the `updateValue` method of `WidgetList<T>` is always called after `activePosition` has been verified by the callers, so additional bounds checking within `updateValue` is not necessary.
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: run-ci (examples/Widgets/Widgets.ino)
- GitHub Check: run-ci (examples/ViewShifting/ViewShifting.ino)
- GitHub Check: run-ci (examples/SubMenu/SubMenu.ino)
- GitHub Check: run-ci (examples/MenuTimeout/MenuTimeout.ino)
- GitHub Check: run-ci (examples/List/List.ino)
- GitHub Check: run-ci (examples/ItemBack/ItemBack.ino)
- GitHub Check: run-ci (examples/IntFloatValues/IntFloatValues.ino)
- GitHub Check: run-ci (examples/CharsetInput/CharsetInput.ino)
- GitHub Check: run-ci (examples/Callbacks/Callbacks.ino)
- GitHub Check: run-ci (examples/Basic/Basic.ino)
- GitHub Check: compile (STMicroelectronics:stm32:GenF1, stm32, STM32-stm32-GenF1)
- GitHub Check: compile (arduino:samd:mkr1000, mkr1000, arduino-samd-mkr1000)
- GitHub Check: compile (esp8266:esp8266:huzzah, esp8266, esp8266-esp8266-huzzah)
- GitHub Check: compile (esp32:esp32:esp32, esp32, esp32-esp32-esp32)
🔇 Additional comments (10)
src/widget/WidgetList.h (4)
11-11: Verify the necessity of the template parameterT.The class
WidgetListstill uses the template parameterT, but it now inherits fromBaseWidgetValue<uint8_t>. SinceTis primarily used for thevaluesarray, consider whether the template is still required or if the class can be simplified by removing the template parameter.
63-72: Ensure the newdrawmethod handles formatting and buffer sizes correctly.The addition of the
drawmethod enhances functionality by allowing the widget to render into a specified buffer. Verify that the method correctly formats the output based on theformatstring and that it safely handles buffer sizes to prevent overflows.
74-90: Confirm boundary checks innextValueandpreviousValuemethods.The
nextValueandpreviousValuemethods manage the internalvaluecorrectly by incrementing or decrementing within the valid range. Ensure that these methods maintainvaluewithin0andsize - 1, and that cycling occurs as expected whencycleistrue.
110-118: Update return type consistency inWIDGET_LISTfunction.The
WIDGET_LISTfunction now returns a pointer toWidgetList<T>instead ofBaseWidgetValue<T>. Confirm that this change is consistent with the rest of the codebase and that it does not introduce compatibility issues with existing code that expects aBaseWidgetValue<T>*return type.docs/source/overview/items/item-widget.rst (1)
Line range hint
1-1:⚠️ Breaking Changes AlertThis PR introduces breaking changes to the callback parameter types:
- All callbacks now receive indices instead of actual values
- Existing code expecting string values will need to be updated
- Documentation should clearly explain the new index-based approach
Consider:
- Adding a migration guide
- Incrementing the major version number
- Adding deprecation warnings for the old approach
examples/ButtonWithRotaryAdapter/ButtonWithRotaryAdapter.ino (1)
32-32:⚠️ Potential issueBreaking Change: Color widget callback now receives index instead of value
The callback parameter type change from
const char*toconst uint8_tmeans the callback now receives the color's index (0-7) instead of the actual color name ("Red", "Green", etc.). This is a breaking change that needs documentation.Consider adding a comment to clarify the new behavior:
ITEM_WIDGET( "Color", - [](const uint8_t color) { Serial.println(color); }, + // color parameter is the index (0-7) into the colors array + [](const uint8_t color) { Serial.println(colors[color]); }, WIDGET_LIST(colors, COLORS_COUNT, 0, "%s")),examples/Widgets/Widgets.ino (3)
28-29:⚠️ Potential issueBreaking Change: Auto widget callback parameters changed
The
optionparameter now receives the index (0-1) into theoptionsarray instead of the actual option value ("Buy"/"Sell"). This is a breaking change that needs documentation.Consider updating the callback to print the actual option:
-[](const uint8_t option, bool isAuto) { Serial.println(option); Serial.println(isAuto); }, +// option parameter is the index (0-1) into the options array +[](const uint8_t option, bool isAuto) { + Serial.println(options[option]); + Serial.println(isAuto); +},
Line range hint
54-58: Breaking Change: Pin widget parameters changedThe
d1-d4parameters now receive indices (0-14) into thepinCharsarray instead of the actual pin characters. This is a breaking change that needs documentation.Consider updating the callback to print actual characters:
-[](const uint8_t d1, const uint8_t d2, const uint8_t d3, const uint8_t d4) { Serial.print(d1); Serial.print(d2); Serial.print(d3); Serial.println(d4); }, +// d1-d4 parameters are indices (0-14) into the pinChars array +[](const uint8_t d1, const uint8_t d2, const uint8_t d3, const uint8_t d4) { + Serial.print(pinChars[d1]); + Serial.print(pinChars[d2]); + Serial.print(pinChars[d3]); + Serial.println(pinChars[d4]); +},
42-43:⚠️ Potential issueBreaking Change: Freq widget day parameter changed
The
dayparameter now receives the index (0-6) into thedaysarray instead of the actual day value ("Mon"-"Sun"). This is a breaking change that needs documentation.Consider updating the callback to print the actual day:
-[](int hour, int minute, const uint8_t day) { Serial.println(hour); Serial.println(minute); Serial.println(day); }, +// day parameter is the index (0-6) into the days array +[](int hour, int minute, const uint8_t day) { + Serial.println(hour); + Serial.println(minute); + Serial.println(days[day]); +},examples/List/List.ino (1)
24-25:⚠️ Potential issueBreaking Change: Color widget callback parameter changed
The
colorparameter now receives the index (0-7) into thecolorsarray instead of the actual color name. This is a breaking change that needs documentation.Consider updating the callback to print the actual color:
-[](const uint8_t color) { Serial.println(color); }, +// color parameter is the index (0-7) into the colors array +[](const uint8_t color) { Serial.println(colors[color]); },
|
Memory usage change @ d07676d
Click for full report table
Click for full report CSV |
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
docs/source/overview/items/item-widget.rst(4 hunks)examples/ButtonWithRotaryAdapter/ButtonWithRotaryAdapter.ino(2 hunks)examples/KeypadShield/KeypadShield.ino(1 hunks)examples/List/List.ino(2 hunks)examples/SimpleRotary/SimpleRotary.ino(1 hunks)examples/Widgets/Widgets.ino(3 hunks)src/widget/WidgetList.h(3 hunks)
🧰 Additional context used
📓 Learnings (1)
src/widget/WidgetList.h (1)
Learnt from: forntoh
PR: forntoh/LcdMenu#255
File: src/widget/WidgetList.h:61-65
Timestamp: 2024-11-12T11:59:37.512Z
Learning: In `src/widget/WidgetList.h`, the `updateValue` method of `WidgetList<T>` is always called after `activePosition` has been verified by the callers, so additional bounds checking within `updateValue` is not necessary.
🪛 GitHub Check: clang_format
src/widget/WidgetList.h
[failure] 11-11: src/widget/WidgetList.h#L11
code should be clang-formatted [-Wclang-format-violations]
[failure] 12-12: src/widget/WidgetList.h#L12
code should be clang-formatted [-Wclang-format-violations]
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: compile (STMicroelectronics:stm32:GenF1, stm32, STM32-stm32-GenF1)
- GitHub Check: compile (arduino:samd:mkr1000, mkr1000, arduino-samd-mkr1000)
- GitHub Check: compile (esp8266:esp8266:huzzah, esp8266, esp8266-esp8266-huzzah)
- GitHub Check: compile (esp32:esp32:esp32, esp32, esp32-esp32-esp32)
🔇 Additional comments (26)
src/widget/WidgetList.h (8)
9-13: Good clarification on index-based value usage.
These doc comments clearly explain that the widget’s value is now an index of typeuint8_trather than a templatedT. This helps prevent confusion for callers and maintainers.🧰 Tools
🪛 GitHub Check: clang_format
[failure] 11-11: src/widget/WidgetList.h#L11
code should be clang-formatted [-Wclang-format-violations]
[failure] 12-12: src/widget/WidgetList.h#L12
code should be clang-formatted [-Wclang-format-violations]
16-16: Confirm necessity of template when the base is fixed to <uint8_t>.
WhileWidgetListis templated as<typename T>, the parent class isBaseWidgetValue<uint8_t>. This design is valid if the template parameterTis only used for storing the array of possible values. Just confirm whether tying the parent to<uint8_t>is intentional and if you anticipate further refactoring to unify the design.
30-31: Potential breaking change for callback signatures.
This changes the callback fromvoid (*callback)(const T&)tovoid (*callback)(const uint8_t&), affecting existing code that passes or expects<T>. Update usage references accordingly; documentation updates appear to be in place.
65-65: No additional bounds checking needed.
Per the retrieved learnings,updateValueis always preceded by verifiedvaluechanges. CallingBaseWidgetValue<uint8_t>::handleChange()here is appropriate.
68-77: Confirm safe usage ofsnprintfin draw method.
The call tosnprintfproperly limits output size toITEM_DRAW_BUFFER_SIZE - start, preventing buffer overruns. The usage ofvalues[value]is correct as long asvalue < size.
79-80: Check for nextValue overflow if size > 255.
if (value + 1 < size)works well as long assize <= 255matchesvalue’s limit. If a widget list ever has more than 255 entries, the nextValue logic could fail or overflow.Also applies to: 84-84
90-91: Maintain symmetry in previousValue.
Decrementingvalueand wrapping around works similarly well under the same constraints asnextValue. Great job mirroring the logic.Also applies to: 95-95
108-108: Accurate doc param name.
Renaming “active position” to “value” matches the code. This avoids confusion about the meaning of the parameter.examples/SimpleRotary/SimpleRotary.ino (1)
28-28: Ensure index-based access is in range.
Indexingcolors[color]is correct, but confirm thatcoloris always< COLORS_COUNTto avoid out-of-bounds. If your upstream logic guarantees this, you’re all set.examples/KeypadShield/KeypadShield.ino (1)
24-24: Use index-based lookup consistently.
Switching to(const uint8_t color){ Serial.println(colors[color]); }is a clean approach to print the color name by index. Just ensure thecolorparameter cannot exceedCOLORS_COUNT.examples/Widgets/Widgets.ino (2)
54-54: Confirm indexes for pin digits.Each digit parameter (
d1,d2,d3,d4) is used to index thepinCharsarray. Ensure that these values always remain within valid limits to prevent out-of-bounds access.Below is a script checking usage of
pinChars[d*]:✅ Verification successful
The index access of pinChars array is properly constrained
The implementation is safe because:
- The pinChars array has 15 characters ("123456789ABCDEF")
- WIDGET_LIST constrains selection using strlen(pinChars)
- Parameters are uint8_t ensuring positive indices
- The widget system is designed for controlled character selection
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash rg -A 3 'pinChars\[d'Length of output: 543
Script:
#!/bin/bash # Find pinChars definition and surrounding context rg -B 3 -A 3 'pinChars\s*\[\s*\]|pinChars\s*=|char\s+pinChars' # Get broader context of the widget setup rg -B 5 -A 5 'WIDGET_LIST.*pinChars'Length of output: 3510
28-28: Ensure array bounds for 'option' parameter.When using
options[option], verify that the providedoptionindex is always within the valid array range. Otherwise, an out-of-bounds access could occur.Below is a script that searches for all references to
options[option]to confirm the approach:✅ Verification successful
Array access is properly bounded by design
The array access is safe because:
- The
optionsarray size (2) is explicitly passed toWIDGET_LISTusingsizeofWidgetListclass internally tracks the array size- The
optionparameter isuint8_t, matching the size type used byWidgetList- The widget implementation manages value bounds as an index
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify all references to 'options[option]' for potential out-of-bounds usage rg -A 3 'options\[option\]'Length of output: 405
Script:
#!/bin/bash # Find options array definition and related context rg -B 5 -A 2 'options\[\s*\]' examples/Widgets/Widgets.ino # Look for WIDGET_LIST macro definition rg -B 2 -A 5 'WIDGET_LIST' --type cppLength of output: 1392
Script:
#!/bin/bash # Look for WidgetList implementation to verify bounds checking ast-grep --pattern 'template <typename T> class WidgetList { $$$ }' # Backup search with ripgrep rg -B 2 -A 10 'class WidgetList' --type cppLength of output: 1408
examples/List/List.ino (3)
24-24: Check correctness of color index usage.Accessing
colors[color]is valid as long ascolorremains within0..(COLORS_COUNT-1). Confirm the index is never out of range.
28-28: Verify numeric index for 'nums'.Ensure
num < NUMS_COUNTto avoid out-of-bounds issues.
47-47: No concerns with this line.examples/ButtonWithRotaryAdapter/ButtonWithRotaryAdapter.ino (2)
32-32: Validate index for 'colors'.When the callback uses
Serial.println(colors[color]);, confirm thatcoloris within0..(COLORS_COUNT-1).
67-67: No substantive change at this line.docs/source/overview/items/item-widget.rst (9)
24-24: Good overview of ItemWidget examples.The introduction clearly indicates how
ItemWidgetis used, matching the new index-based approach.
32-32: Parameter type update is consistent with PR objectives.Thank you for reflecting the transition to
const uint8_tin the docs so that users understand the new callback signature.
40-41: Clear explanation of index-based approach.The newly added note clarifies how the callback receives indices, not string values. This addresses potential confusion effectively.
43-43: Context about the 'auto' option usage looks correct.The documentation matches the callback signature, ensuring consistency.
69-69: Correct highlight for the 'day' parameter's new type.This line accurately reflects the shift to
const uint8_t. Matches the code updates.
78-79: Index-based note ensures clarity for 'day'.Thanks for reiterating that the callback receives day as an index. This helps users avoid confusion.
107-107: Accurate reflection of pin parameter changes.Shifting from
chartoconst uint8_tis properly documented and will help users adapt.
113-114: Reiterated index-based note for pin digits.Clear statement that the callback gets indices, not characters.
119-119: Documentation reference is complete.Linking to the example ensures readers can see the index-based usage in a practical context.
|
Memory usage change @ 43919fb
Click for full report table
Click for full report CSV |
There was a problem hiding this comment.
Functionality Tests are failing for WidgetList @ShishkinDmitriy, please adjust List.test.yml and Widgets.test.yml
The tests originally are expecting the actual value instead of the index, they have to be updated to expect index.
How to run it locally? |
It's quite a setup @ShishkinDmitriy, you first need docker... lot's of stuff, set up 🥲 |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
test/List.test.yml(1 hunks)test/Widgets.test.yml(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: compile (STMicroelectronics:stm32:GenF1, stm32, STM32-stm32-GenF1)
- GitHub Check: compile (arduino:samd:mkr1000, mkr1000, arduino-samd-mkr1000)
- GitHub Check: compile (esp8266:esp8266:huzzah, esp8266, esp8266-esp8266-huzzah)
- GitHub Check: compile (esp32:esp32:esp32, esp32, esp32-esp32-esp32)
🔇 Additional comments (9)
test/List.test.yml (3)
11-15: Switch to index-based output validation fornextValueThese changes correctly transition from referencing string values to using numeric indices. This aligns with the new
const uint8_tparameter usage in the widget callback.
23-25: Index-basedpreviousValuechecksSwitching the test expectations to numeric indices is consistent with the new logic for backward navigation in the widget list. Ensure all references to old string-based values have been removed across the codebase.
27-31: Confirm range wrap-around behaviorThe transitions from one index to another (e.g.,
4→0→2) suggest wrap-around or jump behavior. Verify that the underlying widget logic intentionally allows these leaps, and the expected output for each step is correct.Run the following commands to confirm consistency across all test files:
✅ Verification successful
Wrap-around behavior is verified and intentional
The test files consistently demonstrate wrap-around navigation and non-sequential transitions as expected behavior, with similar patterns found in both test/List.test.yml and test/Widgets.test.yml.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Examine references to WidgetList next/previous values to verify consistent indexing rg 'WidgetList::(nextValue|previousValue)=' -A 2 testLength of output: 4031
test/Widgets.test.yml (6)
9-15: Consistent numeric indices for toggling valuesConverting from string-based values to numeric indices is logical given the new
uint8_tapproach. These lines appear consistent and straightforward.
116-122: Sequential increments ofnextValueYou are correctly testing a series of
nextValueincrements from 1 through 4. This sequence demonstrates progression in the index-based approach. Confirm the final user-facing output matches your widget logic.
135-137: Confirm mid-sequence incrementsThe new lines verify that successive
nextValuecalls jump from 3 to 4. This indicates a valid numeric progression. Keep an eye on any edge cases for boundary conditions.
145-145: Index-basedpreviousValue=3test caseUsing
previousValue=3in this context is consistent with the rollback behavior. No issues found.
149-151: Extended range values fornextValueThe updates from 11 to 12 reflect a larger range of permissible indices. Ensure these larger indices are valid and supported by your new logic.
155-157: Check wrap-around fornextValue=0→1When wrapping from higher indices down to
0and then incrementing, confirm the list boundaries are properly handled.
Description
WidgetListhas callback that accept self typeT, but it's inconvinient to search it's index inside the callback. As solution this wiget will handleconst uint8_tas parameter of callback. If value is needed then it can be obtained by the index.How Has This Been Tested?
Checked on existing examples
Checklist
General Requirements
breaking-changeif it introduces a breaking change.Refactor/Enhancement
enhancement.Summary by CodeRabbit
Summary by CodeRabbit
Documentation
ItemWidgetwith revised parameter types.Refactor
char*andchartouint8_t.WidgetListclass to useuint8_tfor list management and value tracking.Bug Fixes