Skip to content

feat: show units and ranges for numeric settings - #911

Open
Naman015 wants to merge 3 commits into
selfcustody:developfrom
Naman015:feat/numeric-setting-context
Open

feat: show units and ranges for numeric settings#911
Naman015 wants to merge 3 commits into
selfcustody:developfrom
Naman015:feat/numeric-setting-context

Conversation

@Naman015

@Naman015 Naman015 commented Jul 28, 2026

Copy link
Copy Markdown

What is this PR for?

This adds units and allowed ranges to Screensaver Time, Shutdown Time, and Buttons Debounce in the existing numeric editor.

The editor keeps the full text on one line when it fits. If it does not, it shows the value and unit, then the raw value. This keeps long input from wrapping into the M5StickV keypad. Stored values, validation, and input behavior are unchanged.

Design reference

Numeric setting context - Krux

Changes made to:

  • Code
  • Tests
  • Docs
  • CHANGELOG

Did you build the code and tested on device?

  • Yes, built and tested on TZT

What is the purpose of this pull request?

  • Bug fix
  • New feature
  • Docs update
  • Other

Verification

  • Full test suite: 1,141 passed with 97% coverage
  • Pylint 10/10; formatting, Vulture, translation checks, and the strict docs build passed
  • Simulator visual checks passed on M5StickV, TZT, and Amigo, including the M5StickV fallback states
  • TZT smoke test passed, and the original settings were restored

Hardware demo

Krux.numeric.mp4

@codecov

codecov Bot commented Jul 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.44%. Comparing base (8e0ac0c) to head (16316c0).
⚠️ Report is 21 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #911      +/-   ##
===========================================
+ Coverage    97.41%   97.44%   +0.02%     
===========================================
  Files           83       83              
  Lines        10856    10901      +45     
===========================================
+ Hits         10575    10622      +47     
+ Misses         281      279       -2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Naman015
Naman015 marked this pull request as ready for review July 28, 2026 12:14

@qlrd qlrd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

First round :)

I liked the change. One thing that, IMO, you could change is [0-N] to non-zero value, like [1-N] or [5-N] (or maybe when use choose zero show a warning that feature will be disabled, just as "backup warning"). Other that don't have 0, just ignore the idea. About Touchscreen is worth to have it too?

@Naman015
Naman015 marked this pull request as draft July 29, 2026 12:51
@Naman015

Copy link
Copy Markdown
Author

Thanks for taking a look and for the suggestions!

I kept 0 in the ranges because it is a valid value for both Screensaver Time and Shutdown Time, and it is how those features are disabled. Showing [1-N] or [5-N] would make the range on screen different from what the setting actually accepts. I left the warning out of this PR because it would need another prompt and new translations. If we want that warning, I think it would be better as a separate change.

Touchscreen was worth adding. I pushed the [2-200] range for Touch Threshold and updated the screenshot too.

@odudex

odudex commented Jul 29, 2026

Copy link
Copy Markdown
Member

Looks good to me, both UI and implementation.

@Naman015
Naman015 marked this pull request as ready for review July 29, 2026 13:49
Comment thread tests/pages/test_page.py Outdated

@qlrd qlrd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Two more nits on tests (optional). Tested those applying same idea on previous comment. WDYT?

Comment thread tests/pages/test_settings_page.py Outdated
Comment thread tests/pages/test_settings_page.py Outdated
@Naman015
Naman015 force-pushed the feat/numeric-setting-context branch from ecf1285 to 97fd814 Compare July 30, 2026 12:47
@Naman015

Copy link
Copy Markdown
Author

Thanks, all nits make sense to me, applied all three. The assertions are cleaner and more readable now.

Comment thread src/krux/pages/__init__.py
@Naman015
Naman015 force-pushed the feat/numeric-setting-context branch 2 times, most recently from b46b0f7 to 2ed88ec Compare July 31, 2026 07:17
@Naman015
Naman015 force-pushed the feat/numeric-setting-context branch from 2ed88ec to 16316c0 Compare August 2, 2026 05:08
@Naman015

Naman015 commented Aug 2, 2026

Copy link
Copy Markdown
Author

My bad, earlier I squashed everything into one commit. I’ve fixed that now and split it into separate feat, test and docs commits.

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.

3 participants