Skip to content

UX improvements: AppList, Settings, non-touch devices - #671

Merged
KenVanHoeylandt merged 6 commits into
mainfrom
develop
Oct 3, 2026
Merged

KenVanHoeylandt merged 6 commits into
mainfrom
develop

Conversation

@KenVanHoeylandt

@KenVanHoeylandt KenVanHoeylandt commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor
  • Implement double size icon assets in lvgl-module
  • AppList and Settings app now show a paginated grid of icons
  • T-Lora Pager gets a larger font to improve readability (14 -> 16 pixels)
  • Improve default widget selection in several apps (PowerOff, AlertDialog, AppList, Settings, Boot)
  • USB HID LVGL device is now only created when necessary. And the code is called in a safer way.
  • Renamed “Region & Language” to “Locale.”
  • The terminal toolbar is shown only when pointer input is available.

@coderabbitai

coderabbitai Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: a555dce8-ec29-49a4-a7fb-c83e94975d91
📥 Commits

Reviewing files that changed from the base of the PR and between 0559e7e and 1117524.

📒 Files selected for processing (1)
  • Tactility/Source/lvgl/UsbHidInput.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
  • Tactility/Source/lvgl/UsbHidInput.cpp

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review.


📝 Walkthrough

Walkthrough

The LVGL module adds configurable 2× shared icon fonts and a public input-device query. A new AppGrid provides paginated app tiles and replaces list-based displays in the app list and settings screens. USB HID handling now manages pointer-device creation and removal based on mouse connections. Several screens set initial keyboard focus or omit pointer controls when no pointer device exists. Font-generation paths and USB HID subscription behavior also change.

Priority: ➖ Normal

Merge Risk: 🔵 Low · up to 11175

A third-party caller with a very small or already-full queue could miss the mouse-connected notification. The built-in LVGL path is unaffected. This is a minor follow-up and does not block merge.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 0559e

Mouse availability now depends on connection notifications that can be dropped. Losing one can leave mouse input unavailable until reconnection or restart. The supported exposure is local to the device; no expansion of privileges or remote access was established.

Retained concerns

  • Low · security · inferred: Pointer-device existence now depends on best-effort lifecycle events. If traffic or scheduling pressure causes a mouse-connected event to be dropped, subsequent movement and button reports do not recreate the input device, allowing a transient delivery failure to strand local mouse input until a later connection event or restart. Subscription can also report success despite failed replay delivery. A physical HID-driven denial-of-input scenario is plausible, but its practical triggering rate is unverified.
Security review details

Security Blast Radius

  • inferred — The supported attackable surface is the local USB-host input path and its subscribers. Triggering the conditional availability failure would require control of attached HID traffic or access to an in-process subscription queue; the inspected path does not establish remote, cross-device, or cross-tenant exposure.

Security Findings and Attack Paths

  • inferred — If HID traffic fills the subscriber queue when a connection notification is sent, ignored delivery failure can leave the consumer without a pointer input device even after subsequent reports are processed. Unlike base behavior, pointer dispatch itself then remains absent. This is a source-supported conditional failure path, not a runtime-demonstrated exploit.

Trust Boundaries and Controls

  • observed — Replay and live publication use the same subscription mutex, and the consumer subscribes before launching its task. Its newly created 64-entry queue substantially limits initial replay-capacity risk, but successful registration does not guarantee successful replay delivery.

Resilience and Maintainability Implications

  • observed — Failed subscription attempts are retried after receive timeouts. Once subscribed, the consumer has no independent connection-state reconciliation in its event loop, so this retry mechanism does not repair a lost lifecycle notification.

Hardening Proposals

  • proposed — Make connection state recoverable independently of ordinary report delivery, for example through durable state reconciliation or a reliable lifecycle channel. Define how subscription handles incomplete replay rather than treating registration success as successful state synchronization.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 7.69% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 65 functions across 36 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the UX improvements to AppList, Settings, and non-touch devices, which cover key changes in the pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 3


ℹ️ Review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 64ae055f-babc-45ed-ad4d-febc32654d77
📥 Commits

Reviewing files that changed from the base of the PR and between 34ca63e and d5fc0c6.

📒 Files selected for processing (38)
  • Devices/lilygo-tlora-pager/device.properties
  • Modules/lvgl-module/.gitignore
  • Modules/lvgl-module/CMakeLists.txt
  • Modules/lvgl-module/generate-icons.py
  • Modules/lvgl-module/include/lvgl/devices/indev.h
  • Modules/lvgl-module/include/lvgl/fonts.h
  • Modules/lvgl-module/private/lvgl/devices/indev_private.h
  • Modules/lvgl-module/source-fonts/material_symbols_launcher_30.c
  • Modules/lvgl-module/source-fonts/material_symbols_launcher_36.c
  • Modules/lvgl-module/source-fonts/material_symbols_launcher_42.c
  • Modules/lvgl-module/source-fonts/material_symbols_launcher_48.c
  • Modules/lvgl-module/source-fonts/material_symbols_launcher_64.c
  • Modules/lvgl-module/source-fonts/material_symbols_launcher_72.c
  • Modules/lvgl-module/source-fonts/material_symbols_shared_12.c
  • Modules/lvgl-module/source-fonts/material_symbols_shared_16.c
  • Modules/lvgl-module/source-fonts/material_symbols_shared_20.c
  • Modules/lvgl-module/source-fonts/material_symbols_shared_24.c
  • Modules/lvgl-module/source-fonts/material_symbols_shared_32.c
  • Modules/lvgl-module/source-fonts/material_symbols_shared_40.c
  • Modules/lvgl-module/source-fonts/material_symbols_shared_48.c
  • Modules/lvgl-module/source-fonts/material_symbols_shared_64.c
  • Modules/lvgl-module/source-fonts/material_symbols_statusbar_12.c
  • Modules/lvgl-module/source-fonts/material_symbols_statusbar_16.c
  • Modules/lvgl-module/source-fonts/material_symbols_statusbar_20.c
  • Modules/lvgl-module/source-fonts/material_symbols_statusbar_30.c
  • Modules/lvgl-module/source/devices/indev.cpp
  • Modules/lvgl-module/source/fonts.c
  • Modules/lvgl-module/source/symbols.c
  • Tactility/Private/Tactility/app/AppGrid.h
  • Tactility/Source/app/AppGrid.cpp
  • Tactility/Source/app/alertdialog/AlertDialog.cpp
  • Tactility/Source/app/applist/AppList.cpp
  • Tactility/Source/app/boot/Boot.cpp
  • Tactility/Source/app/localesettings/LocaleSettings.cpp
  • Tactility/Source/app/poweroff/PowerOff.cpp
  • Tactility/Source/app/settings/Settings.cpp
  • Tactility/Source/app/terminal/main.cpp
  • Tactility/Source/lvgl/UsbHidInput.cpp
💤 Files with no reviewable changes (1)
  • Modules/lvgl-module/private/lvgl/devices/indev_private.h

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

Comment thread Tactility/Source/app/localesettings/LocaleSettings.cpp
Comment thread Tactility/Source/lvgl/UsbHidInput.cpp Outdated
Comment thread Tactility/Source/lvgl/UsbHidInput.cpp

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to GitHub limitations.

⚠️ Outside diff range comments (2)

🟡 Minor · Keep a 2× font source available for every allowed size. · CMakeLists.txt:77

Modules/lvgl-module/CMakeLists.txt:77
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Keep a 2× font source available for every allowed size.

TT_LVGL_SHARED_ICON_SIZE allows 40. With that setting, CMake adds material_symbols_shared_80.c, which is absent, so the ESP-IDF build can fail. Add 2× sources for allowed sizes or limit the setting to sizes with matching font sources. Size 32 maps to the existing material_symbols_shared_64.c.

🟡 Minor · Handle displays bound after USB HID startup. · UsbHidInput.cpp:384-386

Tactility/Source/lvgl/UsbHidInput.cpp:384-386
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Handle displays bound after USB HID startup.

Tactility attaches existing displays before its on_start hook, but lvgl_display_add() also supports later calls while holding the LVGL lock. If no display is bound when this code checks lv_layer_sys() and a display is added later, ctx->mouse_cursor remains null. A subsequent mouse connection can create an indev without a cursor. Create the cursor after the late display bind from a stack safe for loading its flash-backed asset, and attach it whether the mouse indev already exists or is created later.


ℹ️ Review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 755d0bd9-0674-4306-8b1d-6a02689909f1
📥 Commits

Reviewing files that changed from the base of the PR and between d5fc0c6 and 0559e7e.

📒 Files selected for processing (5)
  • Platforms/platform-esp32/source/drivers/usb/esp32_usbhost_hid.cpp
  • Tactility/Include/Tactility/lvgl/UsbHidInput.h
  • Tactility/Source/app/localesettings/LocaleSettings.cpp
  • Tactility/Source/lvgl/UsbHidInput.cpp
  • TactilityKernel/include/tactility/drivers/usb_host_hid.h
🚧 Files skipped from review as they are similar to previous changes (1)
  • Tactility/Source/app/localesettings/LocaleSettings.cpp

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.

Comment thread Platforms/platform-esp32/source/drivers/usb/esp32_usbhost_hid.cpp
Comment thread Tactility/Source/lvgl/UsbHidInput.cpp
@KenVanHoeylandt
KenVanHoeylandt merged commit 2b6b4b5 into main Oct 3, 2026
66 checks passed
@KenVanHoeylandt
KenVanHoeylandt deleted the develop branch October 3, 2026 23:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant