feat: expose graphical value selection renderer hook#412
Merged
forntoh merged 1 commit intoMay 18, 2026
Merged
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
coderabbitai
Bot
added
documentation
![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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/GraphicalDisplayRenderer.cpp`:
- Around line 233-241: The conditional block that forces selectionEnd =
selectionStart + 1 is unreachable given the invariants around hasValueSelection,
valueSelectionLength > 0 and valueLen >= 1; remove that recovery branch and
replace it with a short comment or an assert documenting the invariant
(referencing hasValueSelection, valueSelectionStart, valueSelectionLength,
safeLength(value), selectionStart, selectionEnd) so future readers know the
guard was intentionally omitted; alternatively, if you want to keep a defensive
check, convert it to an explicit assertion (e.g., assert(selectionEnd >
selectionStart)) with a comment explaining it exists solely to catch future API
changes that might allow zero-length selections.
- Around line 220-221: The code currently computes graphicalItem =
asGraphical(activeItem) and tightSelection = graphicalItem != NULL &&
graphicalItem->useTightGraphicalSelectionBox() on every drawItem call even
though tightSelection is only needed inside the selection-highlight branch; move
the lookup of useTightGraphicalSelectionBox() into the block guarded by hasFocus
&& MenuItem::isEditing() && hasValueSelection so you only call
graphicalItem->useTightGraphicalSelectionBox() when that branch runs, while
retaining a separate asGraphical(activeItem) lookup before the toggle branch
(the code at the toggle branch uses graphicalItem) or split into two lookups to
avoid changing behavior for the toggle path.
In `@test/GraphicalDisplayRenderer.cpp`:
- Around line 40-50: The test only covers the non-const queryExtension path; add
two assertions: call the const overload of
GraphicalDisplayRenderer::queryExtension (by using a const
GraphicalDisplayRenderer or const reference) with
GraphicalValueSelectionRenderer::extensionId() and assert the returned pointer
is non-null, and call queryExtension with a made-up/unknown extension id (e.g.,
a different integer or zero) and assert it returns NULL to ensure unknown ids
are rejected; update the
unittest(graphical_renderer_exposes_value_selection_extension) to perform these
two checks alongside the existing non-const code so both overloads and the
negative case are exercised.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 49d43dcf-db49-4baa-9a50-79e319560e23
📒 Files selected for processing (4)
docs/source/overview/rendering/graphical-display.rstsrc/renderer/GraphicalDisplayRenderer.cppsrc/renderer/GraphicalDisplayRenderer.htest/GraphicalDisplayRenderer.cpp
Comment on lines
+220
to
+221
| const GraphicalMenuItem* graphicalItem = asGraphical(activeItem); | ||
| bool tightSelection = graphicalItem != NULL && graphicalItem->useTightGraphicalSelectionBox(); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Optional: scope tightSelection to where it's used.
graphicalItem and tightSelection are computed on every drawItem call but only consumed inside the selection-highlight block at Line 256. Moving the lookup inside if (hasFocus && MenuItem::isEditing() && hasValueSelection) avoids querying useTightGraphicalSelectionBox() for every rendered row.
♻️ Suggested change
- const GraphicalMenuItem* graphicalItem = asGraphical(activeItem);
- bool tightSelection = graphicalItem != NULL && graphicalItem->useTightGraphicalSelectionBox();
-
if (value != NULL && value[0] != '\0') {…and inside the selection block:
if (hasFocus && MenuItem::isEditing() && hasValueSelection) {
+ const GraphicalMenuItem* graphicalItem = asGraphical(activeItem);
+ bool tightSelection = graphicalItem != NULL && graphicalItem->useTightGraphicalSelectionBox();
uint8_t valueLen = safeLength(value);Note: graphicalItem is also used for the toggle branch at Line 277, so you'd need to keep the lookup for that path (or split the variable).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/GraphicalDisplayRenderer.cpp` around lines 220 - 221, The code
currently computes graphicalItem = asGraphical(activeItem) and tightSelection =
graphicalItem != NULL && graphicalItem->useTightGraphicalSelectionBox() on every
drawItem call even though tightSelection is only needed inside the
selection-highlight branch; move the lookup of useTightGraphicalSelectionBox()
into the block guarded by hasFocus && MenuItem::isEditing() && hasValueSelection
so you only call graphicalItem->useTightGraphicalSelectionBox() when that branch
runs, while retaining a separate asGraphical(activeItem) lookup before the
toggle branch (the code at the toggle branch uses graphicalItem) or split into
two lookups to avoid changing behavior for the toggle path.
Comment on lines
+233
to
+241
| if (hasFocus && MenuItem::isEditing() && hasValueSelection) { | ||
| uint8_t valueLen = safeLength(value); | ||
| uint8_t selectionStart = valueSelectionStart > valueLen ? valueLen : valueSelectionStart; | ||
| uint16_t rawEnd = static_cast<uint16_t>(valueSelectionStart) + valueSelectionLength; | ||
| uint8_t selectionEnd = rawEnd > valueLen ? valueLen : static_cast<uint8_t>(rawEnd); | ||
|
|
||
| if (selectionEnd <= selectionStart && selectionStart < valueLen) { | ||
| selectionEnd = selectionStart + 1; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Defensive selectionEnd <= selectionStart expansion is unreachable.
Given the call site invariants (hasValueSelection is only true when valueSelectionLength > 0, and value[0] != '\0' so valueLen >= 1), tracing the clamp logic:
- If
valueSelectionStart < valueLen:selectionStart = valueSelectionStartandselectionEnd >= selectionStart + 1(since length > 0), soselectionEnd > selectionStartalways. - If
valueSelectionStart >= valueLen: both clamp tovalueLen, soselectionStart == valueLenand the second predicateselectionStart < valueLenis false.
Either the expansion block can be dropped, or the intent (e.g., guarding against future API changes that allow length = 0) deserves a brief comment so readers don't try to reason about which case it covers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/GraphicalDisplayRenderer.cpp` around lines 233 - 241, The
conditional block that forces selectionEnd = selectionStart + 1 is unreachable
given the invariants around hasValueSelection, valueSelectionLength > 0 and
valueLen >= 1; remove that recovery branch and replace it with a short comment
or an assert documenting the invariant (referencing hasValueSelection,
valueSelectionStart, valueSelectionLength, safeLength(value), selectionStart,
selectionEnd) so future readers know the guard was intentionally omitted;
alternatively, if you want to keep a defensive check, convert it to an explicit
assertion (e.g., assert(selectionEnd > selectionStart)) with a comment
explaining it exists solely to catch future API changes that might allow
zero-length selections.
Comment on lines
+40
to
+50
| unittest(graphical_renderer_exposes_value_selection_extension) { | ||
| StubGraphicalDisplay display; | ||
| GraphicalDisplayRenderer renderer(&display); | ||
|
|
||
| void* extension = renderer.queryExtension(GraphicalValueSelectionRenderer::extensionId()); | ||
| assertTrue(extension != NULL); | ||
|
|
||
| GraphicalValueSelectionRenderer* selection = static_cast<GraphicalValueSelectionRenderer*>(extension); | ||
| selection->setValueSelection(1, 2); | ||
| selection->clearValueSelection(); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Optional: broaden coverage to const overload and negative case.
The test confirms the non-const queryExtension returns a usable pointer. Two small additions would lock down the contract more fully:
- Verify the
constoverload also returns non-null (it's a separate function in the diff). - Verify an unknown extension id returns
NULL, so a future regression that aliases ids is caught.
♻️ Suggested addition
GraphicalValueSelectionRenderer* selection = static_cast<GraphicalValueSelectionRenderer*>(extension);
selection->setValueSelection(1, 2);
selection->clearValueSelection();
+
+ const GraphicalDisplayRenderer& constRenderer = renderer;
+ const void* constExtension = constRenderer.queryExtension(GraphicalValueSelectionRenderer::extensionId());
+ assertTrue(constExtension != NULL);
+
+ assertTrue(renderer.queryExtension(0xFF) == NULL);
}📝 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.
Suggested change
| unittest(graphical_renderer_exposes_value_selection_extension) { | |
| StubGraphicalDisplay display; | |
| GraphicalDisplayRenderer renderer(&display); | |
| void* extension = renderer.queryExtension(GraphicalValueSelectionRenderer::extensionId()); | |
| assertTrue(extension != NULL); | |
| GraphicalValueSelectionRenderer* selection = static_cast<GraphicalValueSelectionRenderer*>(extension); | |
| selection->setValueSelection(1, 2); | |
| selection->clearValueSelection(); | |
| } | |
| unittest(graphical_renderer_exposes_value_selection_extension) { | |
| StubGraphicalDisplay display; | |
| GraphicalDisplayRenderer renderer(&display); | |
| void* extension = renderer.queryExtension(GraphicalValueSelectionRenderer::extensionId()); | |
| assertTrue(extension != NULL); | |
| GraphicalValueSelectionRenderer* selection = static_cast<GraphicalValueSelectionRenderer*>(extension); | |
| selection->setValueSelection(1, 2); | |
| selection->clearValueSelection(); | |
| const GraphicalDisplayRenderer& constRenderer = renderer; | |
| const void* constExtension = constRenderer.queryExtension(GraphicalValueSelectionRenderer::extensionId()); | |
| assertTrue(constExtension != NULL); | |
| assertTrue(renderer.queryExtension(0xFF) == NULL); | |
| } |
🧰 Tools
🪛 Cppcheck (2.20.0)
[error] 40-40: syntax error
(syntaxError)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/GraphicalDisplayRenderer.cpp` around lines 40 - 50, The test only covers
the non-const queryExtension path; add two assertions: call the const overload
of GraphicalDisplayRenderer::queryExtension (by using a const
GraphicalDisplayRenderer or const reference) with
GraphicalValueSelectionRenderer::extensionId() and assert the returned pointer
is non-null, and call queryExtension with a made-up/unknown extension id (e.g.,
a different integer or zero) and assert it returns NULL to ensure unknown ids
are rejected; update the
unittest(graphical_renderer_exposes_value_selection_extension) to perform these
two checks alongside the existing non-const code so both overloads and the
negative case are exercised.
Contributor
|
Memory usage change @ ceb4634
Click for full report table
Click for full report CSV |
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
1 participant
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.
Summary
GraphicalDisplayRendererto implement and exposeGraphicalValueSelectionRendererviaqueryExtension().graphical-displaydocs and add unit coverage intest/GraphicalDisplayRenderer.cppto verify extension exposure.Summary by CodeRabbit