events: Remove events unrelated to sliders #1607
Conversation
NickeZ
force-pushed
the
maint/reduce-events
branch
10 times, most recently
from
130affe to
0c8d37a
Compare
|
Fixed bug where keyboard was activated by wrong events. There was a logical bug in the event handler of trinary_input_char. |
| case UPPER_CASE: | ||
| trinary_input_char_set_alphabet(trinary_char, _alphabet_uppercase, 1); | ||
| break; | ||
| case DIGITS: | ||
| trinary_input_char_set_alphabet(trinary_char, _digits, 1); | ||
| break; | ||
| case SPECIAL_CHARS: | ||
| trinary_input_char_set_alphabet(trinary_char, _special_chars, 2); |
There was a problem hiding this comment.
This is a copy/paste of the last branch of _set_alphabet(), and before this PR, _set_alphabet() was called on keyboard switching. Why not just call _set_alphabet() here instead of repeating the code?
Or at least move this to a helper function and call it from both places.
There was a problem hiding this comment.
_set_alphabet() seems to be some kind of "god function" that behaves very differently based on input. I can see again if I can reduce the duplicity, but sometimes just writing it out is simpler than having an extra function.
There was a problem hiding this comment.
called the callback in _set_alphabet() to keep it DRY.
There was a problem hiding this comment.
Imho calling _on_keyboard_switch_cb() from _set_alphabet() even though there was no keyboard switch is less intuitive than calling _set_alphabet() from _on_keyboard_switch_cb() 😂 as you say, it's the "god" function that sets the keyboard correctly whenever called, so it's appropriate to call it.
But not that important, if you prefer it much more this way, then ok.
There was a problem hiding this comment.
i made a separate function to update the input_char from the keyboard_mode.
I really didn't like that the else clause could cause a nullptr-deref in case the keyboard switch component was NULL, so I added an extra check. I'm sure the other checks implicitly meant that that was impossible. But it just felt so unsafe.
NickeZ
force-pushed
the
maint/reduce-events
branch
from
0c8d37a to
c8e582f
Compare
Sorry, I started fixing the issues before reading this comment. |
NickeZ
force-pushed
the
maint/reduce-events
branch
3 times, most recently
from
eb04254 to
4d74e73
Compare
NickeZ
force-pushed
the
maint/reduce-events
branch
from
4d74e73 to
78578f6
Compare
* entry_screen isn't used * confirm_button was just a trivial wrapper
Using events for communication between components in the same screen seem to make things racy, simplify/sequentialize by using callbacks instead. Keep events only for external events like slider inputs.
NickeZ
force-pushed
the
maint/reduce-events
branch
from
78578f6 to
445a392
Compare
No description provided.