Memory Observer improvements:#1927
Open
notdodgeball wants to merge 4 commits into
Open
Conversation
• proper TextFlags in input box • remove button in address tables • new position for "fixed-point values" checkbox • new buttons for (un)freeze/all • smaller buttons for read and write breakpoints • validate sequence size to prevent error
Contributor
📝 WalkthroughWalkthroughUI changes to the memory observer: Pattern input character filtering by Hex state; per-row Remove buttons in Plain and Delta-over-time results; Delta-over-time adds New scan, conditional fixed-point display, bulk Freeze/Unfreeze/Remove frozen controls, expanded result columns with Freeze and Read/Write breakpoints; Pattern search truncates sequences for fixed sizes. ChangesMemory observer UI
Sequence Diagram(s)sequenceDiagram
participant User
participant GUI
participant MemoryObserver
User->>GUI: Enter value in "Plain search" input
GUI->>GUI: Filter input characters (hex or decimal)
User->>GUI: Click "Remove" on a found address
GUI->>MemoryObserver: Remove address and update results
User->>GUI: Use "Delta-over-time" controls (New scan, Freeze all, etc.)
GUI->>MemoryObserver: Initialize/clear results, apply bulk freeze/unfreeze/remove
User->>GUI: Select sequence size in "Pattern search"
GUI->>GUI: Truncate sequence string if it exceeds selected size
Estimated code review effort: Suggested reviewers
Poem
🚥 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 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 |
There was a problem hiding this comment.
Pull Request Overview
This PR refines the Memory Observer widget with improved input handling, new removal buttons, updated layout for checkboxes and breakpoints, and sequence size validation to prevent errors.
- Enhanced the TextFlags for the pattern input to support hexadecimal/decimal validation
- Added new buttons for removing addresses and for freezing/unfreezing memory addresses
- Implemented sequence length truncation when selecting preset sequence sizes
Comments suppressed due to low confidence (1)
src/gui/widgets/memory_observer.cc:443
- Truncating m_sequence in the radio button selection logic may inadvertently remove valid data if the sequence exceeds the expected length; please verify that this behavior is intended and consider adding a clarifying comment or test to document it.
if (ImGui::RadioButton(_("8 bytes (fast)"), &m_sequenceSize, 8) && strlen(m_sequence) > 8) {
Add column for freeze action Change button order, one less button Use std::erase_if instead of std::remove_if Disable input when not needed Finally, change the Next Scan button to underneath it all to stop hitting it accidentally
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/gui/widgets/memory_observer.cc`:
- Around line 211-252: The final stride-aligned block never gets evaluated
because the switch handling accumulated memValue only runs when i % stride == 0
inside the loop; after the loop ends you must run the same switch on the last
accumulated memValue (the block that covers memBase + memSize - stride .. end)
and push matches into m_addressValuePairs; implement this by either extracting
the switch logic into a helper (e.g., ProcessScannedValue(int64_t value,
uint32_t addr) and call it inside the loop and once after the loop) or by
duplicating the switch after the loop using the same conditions (use m_scanType,
m_value, memValue, memBase, stride, getValueAsSelectedType, memData to compute
the correct address).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d234f543-ec13-425e-b029-e3402d3f6c31
📒 Files selected for processing (1)
src/gui/widgets/memory_observer.cc
Comment on lines
+211
to
+252
| if (ImGui::Button(_("New scan"))) { | ||
| if (m_addressValuePairs.empty()) { | ||
| int64_t memValue = 0; | ||
|
|
||
| for (uint32_t i = 0; i < memSize; ++i) { | ||
| if (i != 0 && i % stride == 0) { | ||
| switch (m_scanType) { | ||
| case ScanType::ExactValue: | ||
| if (memValue == m_value) { | ||
| m_addressValuePairs.push_back({memBase + i - stride, memValue}); | ||
| } | ||
| break; | ||
| case ScanType::GreaterThan: | ||
| if (memValue > m_value) { | ||
| m_addressValuePairs.push_back({memBase + i - stride, memValue}); | ||
| } | ||
| break; | ||
| case ScanType::LessThan: | ||
| if (memValue < m_value) { | ||
| m_addressValuePairs.push_back({memBase + i - stride, memValue}); | ||
| } | ||
| break; | ||
| case ScanType::Changed: | ||
| case ScanType::Unchanged: | ||
| case ScanType::Increased: | ||
| case ScanType::Decreased: | ||
| break; | ||
| case ScanType::UnknownInitialValue: | ||
| m_addressValuePairs.push_back({memBase + i - stride, memValue}); | ||
| } | ||
| break; | ||
| case ScanType::Changed: | ||
| case ScanType::Unchanged: | ||
| case ScanType::Increased: | ||
| case ScanType::Decreased: | ||
| break; | ||
| case ScanType::UnknownInitialValue: | ||
| m_addressValuePairs.push_back({memBase + i - stride, memValue}); | ||
| break; | ||
| break; | ||
| } | ||
|
|
||
| memValue = 0; | ||
| } | ||
|
|
||
| memValue = 0; | ||
| const uint8_t currentByte = memData[i]; | ||
| const uint8_t leftShift = 8 * (i % stride); | ||
| const uint32_t mask = 0xffffffff ^ (0xff << leftShift); | ||
| const int byteToWrite = currentByte << leftShift; | ||
| memValue = (memValue & mask) | byteToWrite; | ||
| memValue = getValueAsSelectedType(memValue); | ||
| } |
Contributor
There was a problem hiding this comment.
Last stride-aligned memory block is never scanned.
The loop processes accumulated memValue at stride boundaries (when i != 0 && i % stride == 0), but the final block accumulated during the last stride iterations is never evaluated after the loop exits. This causes the addresses from memBase + memSize - stride onward to be missing from scan results.
Proposed fix: process the final block after the loop
memValue = getValueAsSelectedType(memValue);
}
+ // Process the final accumulated value
+ switch (m_scanType) {
+ case ScanType::ExactValue:
+ if (memValue == m_value) {
+ m_addressValuePairs.push_back({memBase + memSize - stride, memValue});
+ }
+ break;
+ case ScanType::GreaterThan:
+ if (memValue > m_value) {
+ m_addressValuePairs.push_back({memBase + memSize - stride, memValue});
+ }
+ break;
+ case ScanType::LessThan:
+ if (memValue < m_value) {
+ m_addressValuePairs.push_back({memBase + memSize - stride, memValue});
+ }
+ break;
+ case ScanType::Changed:
+ case ScanType::Unchanged:
+ case ScanType::Increased:
+ case ScanType::Decreased:
+ break;
+ case ScanType::UnknownInitialValue:
+ m_addressValuePairs.push_back({memBase + memSize - stride, memValue});
+ break;
+ }
} else {
m_addressValuePairs.clear();Alternatively, refactor the loop to process after accumulation or extract the switch logic into a helper function to avoid duplication.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/gui/widgets/memory_observer.cc` around lines 211 - 252, The final
stride-aligned block never gets evaluated because the switch handling
accumulated memValue only runs when i % stride == 0 inside the loop; after the
loop ends you must run the same switch on the last accumulated memValue (the
block that covers memBase + memSize - stride .. end) and push matches into
m_addressValuePairs; implement this by either extracting the switch logic into a
helper (e.g., ProcessScannedValue(int64_t value, uint32_t addr) and call it
inside the loop and once after the loop) or by duplicating the switch after the
loop using the same conditions (use m_scanType, m_value, memValue, memBase,
stride, getValueAsSelectedType, memData to compute the correct address).
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
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.
• proper TextFlags in input box
• new remove button in address tables
• changed position for "fixed-point values" checkbox
• new buttons for (un)freeze/all
• smaller buttons for read and write breakpoints, unified in a single column
• validate sequence size to prevent error