Fix ItemToggle Duplicate Item When Refreshing Menu in Callback #286
Conversation
WalkthroughThe changes in the Changes
Assessment against linked issues
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 (
|
mriveralee
changed the title
bugfix Fix ItemToggle Duplicate Item When Refreshing Menu in Callbackbugfix Fix ItemToggle Duplicate Item When Refreshing Menu in Callback; fixes #264
mriveralee
changed the title
bugfix Fix ItemToggle Duplicate Item When Refreshing Menu in Callback; fixes #264bugfix Fix ItemToggle Duplicate Item When Refreshing Menu in Callback
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/ItemToggle.h(1 hunks)src/LcdMenu.cpp(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/ItemToggle.h (1)
Learnt from: forntoh
PR: forntoh/LcdMenu#250
File: src/ItemToggle.h:105-105
Timestamp: 2024-11-12T11:59:37.512Z
Learning: In the file `src/ItemToggle.h`, the logging format `"ItemToggle::toggle"` used in the `toggle` method matches the convention used in other files.
🔇 Additional comments (1)
src/ItemToggle.h (1)
102-103: Ordering looks correct.Toggling the
enabledstate before logging and callingdraw()ensures the displayed state matches the internal state. Invoking the callback last prevents any race conditions during UI updates. This is coherent with the fix described in the PR. Good job!
mriveralee
force-pushed
the
fixDisplayRefreshDupe
branch
from
4291de0 to
1835741
Compare
forntoh
changed the title
bugfix Fix ItemToggle Duplicate Item When Refreshing Menu in Callback
- Disabled auto-fix and commit options in lint check. - Added dynamic repository reference in CI setup.
- Disabled auto-fix and commit options in lint check. - Added dynamic repository reference in CI setup.
forntoh
dismissed
coderabbitai[bot]’s stale review
The merge-base changed after approval.
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 (1)
.github/workflows/lint_check.yml(1 hunks)
🧰 Additional context used
📓 Learnings (1)
📓 Common learnings
Learnt from: forntoh
PR: forntoh/LcdMenu#250
File: src/ItemToggle.h:105-105
Timestamp: 2024-11-12T11:59:37.512Z
Learning: In the file `src/ItemToggle.h`, the logging format `"ItemToggle::toggle"` used in the `toggle` method matches the convention used in other files.
Description
This PR fixes #264. I reproduced this bug and have found the issue:
ItemToggle::toggle()executes itscallbackfunction before it redraws. Whenmenu.refresh()is in the callback function, it changes the cursor row position to the max row. Then ItemToggle calls its draw to duplicate the item on the last visible row.The simple solution here is to move the callback after the redraw. A similar approach is used in ItemInput,
ItemInput::back()(https://github.com/forntoh/LcdMenu/blob/master/src/ItemInput.h#L189) , which callsdraw(...)first and then execute the callback.How Has This Been Tested?
I used the provided code in #264 with an Arduino Nano, LCD Display 4x20 I2C , and Rotary Encoder. I tested by running the code, toggle the first item to trigger the
menu.refresh()call, and visually verified the problem is resolved. I also tested the individual toggle examples to make sure the behavior works as expected.Additional Notes
Checklist
General Requirements
breaking-changeif it introduces a breaking change.Bug Fix
bugfix.Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
Chores