Skip to content

feat: expose graphical value selection renderer hook #412

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
forntoh merged 1 commit into from
May 18, 2026
Merged

feat: expose graphical value selection renderer hook #412

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions docs/source/overview/rendering/graphical-display.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ Renderer-specific enhancements remain optional through ``queryExtension()``.
For example, indicators use ``GraphicalIndicatorRenderer`` and value selection
highlighting can be added with ``GraphicalValueSelectionRenderer``.

``GraphicalDisplayRenderer`` now exposes ``GraphicalValueSelectionRenderer`` so
``ItemInput`` and ``ItemInputCharset`` can render active-character selection on
graphical displays.

Basic usage
-----------

Expand Down
91 changes: 90 additions & 1 deletion src/renderer/GraphicalDisplayRenderer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,30 @@

namespace {
const uint8_t listGlyph[] = {0x08, 0x1C, 0x3E, 0x00, 0x3E, 0x1C, 0x08, 0x00};
const uint8_t textBufferSize = ITEM_DRAW_BUFFER_SIZE;

uint8_t safeLength(const char* text) {
if (text == NULL) {
return 0;
}
size_t len = strlen(text);
return len > 255 ? 255 : static_cast<uint8_t>(len);
}

void copyTextRange(const char* text, uint8_t start, uint8_t count, char* out) {
if (text == NULL || count == 0) {
out[0] = '\0';
return;
}

uint8_t i = 0;
while (text[start] != '\0' && i < count && i < textBufferSize - 1) {
out[i] = text[start];
i++;
start++;
}
out[i] = '\0';
}

const GraphicalMenuItem* asGraphical(const MenuItem* item) {
if (item == NULL) {
Expand Down Expand Up @@ -100,20 +124,36 @@ void GraphicalDisplayRenderer::setValueAreaWidth(uint8_t width) {

void GraphicalDisplayRenderer::setActiveItem(const MenuItem* item) {
activeItem = item;
clearValueSelection();
applyItemFont(item);
}

GraphicalDisplayInterface* GraphicalDisplayRenderer::getGraphicalDisplay() {
return gDisplay;
}

void GraphicalDisplayRenderer::setValueSelection(uint8_t start, uint8_t length) {
valueSelectionStart = start;
valueSelectionLength = length;
hasValueSelection = length > 0;
}

void GraphicalDisplayRenderer::clearValueSelection() {
valueSelectionStart = 0;
valueSelectionLength = 0;
hasValueSelection = false;
}

void* GraphicalDisplayRenderer::queryExtension(uint8_t extensionId) {
if (extensionId == FrameLifecycleRenderer::extensionId()) {
return static_cast<FrameLifecycleRenderer*>(this);
}
if (extensionId == GraphicalIndicatorRenderer::extensionId()) {
return static_cast<GraphicalIndicatorRenderer*>(this);
}
if (extensionId == GraphicalValueSelectionRenderer::extensionId()) {
return static_cast<GraphicalValueSelectionRenderer*>(this);
}
if (extensionId == GraphicalRendererContext::extensionId()) {
return static_cast<GraphicalRendererContext*>(this);
}
Expand All @@ -127,6 +167,9 @@ const void* GraphicalDisplayRenderer::queryExtension(uint8_t extensionId) const
if (extensionId == GraphicalIndicatorRenderer::extensionId()) {
return static_cast<const GraphicalIndicatorRenderer*>(this);
}
if (extensionId == GraphicalValueSelectionRenderer::extensionId()) {
return static_cast<const GraphicalValueSelectionRenderer*>(this);
}
if (extensionId == GraphicalRendererContext::extensionId()) {
return static_cast<const GraphicalRendererContext*>(this);
}
Expand Down Expand Up @@ -174,6 +217,9 @@ void GraphicalDisplayRenderer::drawItem(const char* text, const char* value, boo
gDisplay->draw(label);
x += measureText(label) + 1;

const GraphicalMenuItem* graphicalItem = asGraphical(activeItem);
bool tightSelection = graphicalItem != NULL && graphicalItem->useTightGraphicalSelectionBox();
Comment on lines +220 to +221
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.


if (value != NULL && value[0] != '\0') {
uint8_t valueWidth = valueAreaWidth;
if (valueWidth == 0) {
Expand All @@ -183,8 +229,51 @@ void GraphicalDisplayRenderer::drawItem(const char* text, const char* value, boo
uint8_t valueX = valueRight > valueWidth ? valueRight - valueWidth : x;
gDisplay->setCursor(valueX, baseline);
gDisplay->draw(value);

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;
}
Comment on lines +233 to +241
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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 = valueSelectionStart and selectionEnd >= selectionStart + 1 (since length > 0), so selectionEnd > selectionStart always.
  • If valueSelectionStart >= valueLen: both clamp to valueLen, so selectionStart == valueLen and the second predicate selectionStart < valueLen is 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.


if (selectionEnd > selectionStart) {
char prefixBuf[textBufferSize];
char selectedBuf[textBufferSize];
copyTextRange(value, 0, selectionStart, prefixBuf);
copyTextRange(value, selectionStart, static_cast<uint8_t>(selectionEnd - selectionStart), selectedBuf);

uint8_t prefixWidth = measureText(prefixBuf);
uint8_t selectedWidth = measureText(selectedBuf);
if (selectedWidth == 0) {
selectedWidth = gDisplay->getFontWidth() == 0 ? 1 : gDisplay->getFontWidth();
}

uint8_t selectedX = valueX + prefixWidth;
uint8_t pad = tightSelection ? 0 : 1;
uint8_t highlightX = selectedX > pad ? static_cast<uint8_t>(selectedX - pad) : 0;
uint16_t highlightRight = static_cast<uint16_t>(selectedX) + selectedWidth + pad;
if (highlightRight > valueRight) {
highlightRight = valueRight;
}

uint8_t highlightWidth = highlightRight > highlightX
? static_cast<uint8_t>(highlightRight - highlightX)
: 0;
if (highlightWidth > 0) {
gDisplay->setDrawColor(1);
gDisplay->drawBox(highlightX, yTop, highlightWidth, h);
gDisplay->setDrawColor(0);
gDisplay->setCursor(selectedX, baseline);
gDisplay->draw(selectedBuf);
gDisplay->setDrawColor(1);
}
}
}
} else {
const GraphicalMenuItem* graphicalItem = asGraphical(activeItem);
if (graphicalItem != NULL && graphicalItem->hasGraphicalToggle()) {
uint8_t box = toggleIndicatorWidth();
uint8_t xBox = contentRight > rightPadding + box ? contentRight - rightPadding - box : x;
Expand Down
7 changes: 7 additions & 0 deletions src/renderer/GraphicalDisplayRenderer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include "GraphicalIndicatorRenderer.h"
#include "GraphicalItemFont.h"
#include "GraphicalRendererContext.h"
#include "GraphicalValueSelectionRenderer.h"
#include "MenuRenderer.h"
#include "display/GraphicalDisplayInterface.h"
#include "utils/std.h"
Expand All @@ -15,6 +16,7 @@
class GraphicalDisplayRenderer : public MenuRenderer,
public FrameLifecycleRenderer,
public GraphicalIndicatorRenderer,
public GraphicalValueSelectionRenderer,
public GraphicalRendererContext {
private:
GraphicalDisplayInterface* gDisplay;
Expand All @@ -29,6 +31,9 @@ class GraphicalDisplayRenderer : public MenuRenderer,
uint8_t cursorPixelY = 0;
uint8_t maxRowHeight = 8;
uint8_t maxFontWidth = 1;
bool hasValueSelection = false;
uint8_t valueSelectionStart = 0;
uint8_t valueSelectionLength = 0;

const char* cursorIcon;
const char* editCursorIcon;
Expand Down Expand Up @@ -74,6 +79,8 @@ class GraphicalDisplayRenderer : public MenuRenderer,
void setValueAreaWidth(uint8_t width) override;
void setActiveItem(const MenuItem* item) override;
GraphicalDisplayInterface* getGraphicalDisplay() override;
void setValueSelection(uint8_t start, uint8_t length) override;
void clearValueSelection() override;

void* queryExtension(uint8_t extensionId) override;
const void* queryExtension(uint8_t extensionId) const override;
Expand Down
52 changes: 52 additions & 0 deletions test/GraphicalDisplayRenderer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
#include <ArduinoUnitTests.h>
#include <display/GraphicalDisplayInterface.h>
#include <renderer/GraphicalDisplayRenderer.h>
#include <renderer/GraphicalValueSelectionRenderer.h>

class StubGraphicalDisplay : public GraphicalDisplayInterface {
public:
void begin() override {}
void clear() override {}
void show() override {}
void hide() override {}
void draw(uint8_t) override {}
void draw(const char*) override {}
void setCursor(uint8_t, uint8_t) override {}
void setBacklight(bool) override {}

void setFont(const uint8_t*) override {}
uint8_t getDisplayWidth() const override { return 128; }
uint8_t getDisplayHeight() const override { return 64; }
uint8_t getFontWidth() const override { return 6; }
uint8_t getFontHeight() const override { return 8; }
uint8_t getTextWidth(const char* text) override {
if (text == NULL) {
return 0;
}
uint8_t len = 0;
while (text[len] != '\0') {
len++;
}
return static_cast<uint8_t>(len * 6);
}
void setDrawColor(uint8_t) override {}
void clearBuffer() override {}
void sendBuffer() override {}
void drawBox(uint8_t, uint8_t, uint8_t, uint8_t) override {}
void drawFrame(uint8_t, uint8_t, uint8_t, uint8_t) override {}
void drawXbm(uint8_t, uint8_t, uint8_t, uint8_t, const uint8_t*) override {}
};

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();
}
Comment on lines +40 to +50
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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:

  1. Verify the const overload also returns non-null (it's a separate function in the diff).
  2. 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.


unittest_main()
Loading