src/ui/components: Added -Feature: Hitting back during "keypad" input goes back to previous group of characters #862- #1689
Conversation
|
Tested, but I think I found an edge case. See the recording. It seems to jump back "two navigations" when I go back and forward. edit: please use clang-format to format as well. I noticed CI failed on formatting. Screen.Recording.2025-11-17.at.15.30.22.mov |
|
Thank you for your feedback, I will definitely try to figure out what is wrong, will use clang-format as well 🙂 |
alessandro-opensource
force-pushed
the
feature/back-to-previous
branch
from
15eb347 to
fca0682
Compare
|
Hi, I rebased the fork on master branch, added a commit that seems to fix the problem with the previous one and a clang-format commit as well, happy to receive your feedback 🙂 |
benma
left a comment
benma
left a comment
There was a problem hiding this comment.
Another bug:
Navigation stack keeps stale entries when the alphabet is replaced externally (keyboard switch / number mode / wordlist reset). trinary_input_char_set_alphabet() sets in_progress to false but never
clears navigation_stack (src/ui/components/trinary_input_char.c:282-330). Calls such as _input_char_set_alphabet() (src/ui/components/trinary_input_string.c:340-350) can run while the user is mid-navigation,
leaving the old stack in place. Repro: drill into a sub‑alphabet (stack size 1), tap the keyboard switch to digits (stack still contains the old alphabet), navigate into a digits group (stack size 2),
then press back twice → the second back unexpectedly restores the old lowercase alphabet instead of staying in the digit keyboard.
You probably need to split trinary_input_char_set_alphabet() into the external/header function that calls an internal set_alphabet helper function, where the external one clears the stack and the internal one doesn't. This would also clean up the ugly hack around setting in_progress=true, right after it was set to false by trinary_input_char_set_alphabet.
| _navigation_stack_push(data, data->current_full_alphabet); | ||
|
|
||
| /* Store the new alphabet and navigate to it */ | ||
| snprintf(data->current_full_alphabet, sizeof(data->current_full_alphabet), "%s", alphabet); |
There was a problem hiding this comment.
This line is repeated inside the call below to trinary_input_char_set_alphabet, so it is not needed here, right?
Thinking further, maybe the current_full_alphabet var is not needed at all? Can't you just push the alphabet onto the stack directly in trinary_input_char_set_alphabet() instead of writing it to current_full_alphabet? (Not sure if possible, didn't think hard about it)
There was a problem hiding this comment.
yes, I think it is possible if we implement something like the _rebuild_full_alphabet() added in this last commit.
Not sure this is what you meant: the pop from the stack is now only done in the bool trinary_input_char_go_back(component_t* component) and the push in the static void _alphabet_selected(component_t* component, const char* alphabet) only. In this way the new internal _set_alphabet_internal is dedicated to manipulate strings etc. and not pushing or popping from the stack nor clearing it up.
There was a problem hiding this comment.
Overall, the bug you reported seems to be resolved with this approach.
|
|
||
| data->character_chosen_cb = character_chosen_cb; | ||
| // Initialize navigation stack | ||
| _navigation_stack_clear(data); |
There was a problem hiding this comment.
Technically not needed right? data is already initialized to be all zeroes.
There was a problem hiding this comment.
yes, removed in the new commit
| #define MAX_CHARS 33 | ||
|
|
||
| // Maximum depth for navigation stack | ||
| #define MAX_NAVIGATION_DEPTH 8 |
There was a problem hiding this comment.
4 should be enough, it's the maximum number of taps needed to get to the char.
input goes back to previous group of characters BitBoxSwiss#862-
alessandro-opensource
force-pushed
the
feature/back-to-previous
branch
from
fca0682 to
7b32709
Compare
|
I squashed the commits as requested, but if it’s more convenient to review the recent additions in a separate commit, I can force-push that separate commit since I still have a local copy from before the squash. Thanks for the feedback again! |
3 participants
Hi! I gave issue #862 a try.
I tested it by building in the container and using the graphical simulator, and it seems that now pressing the back button takes you to the previous group instead of all letters.
Unfortunately, I haven’t had a physical device yet, so I haven’t been able to try flashing it.
Would love your feedback! Thanks