Stalker: keep LED bright while key is held #1388
Conversation
|
Forgot to mention: this is how it used to be before 2073c4f#diff-8ad5917f582836d9d7f56d572e4f08b9d7c15728ac75ec22bb1fab809f3d2ce9 |
| @@ -71,6 +69,11 @@ void StalkerEffect::TransientLEDMode::update() { | |||
|
|
|||
| for (auto key_addr : KeyAddr::all()) { | |||
| uint8_t step = map_[key_addr.toInt()]; | |||
|
|
|||
There was a problem hiding this comment.
@tremby would you mind adding a comment here explaining what's happening?
There was a problem hiding this comment.
I added a comment. Honestly I am not 100% certain that live_keys and Key_Inactive mean exactly what I think they do, so please do review! I am not well versed in the Kaleidoscope API.
| @@ -51,9 +52,6 @@ EventHandlerResult StalkerEffect::onKeyEvent(KeyEvent &event) { | |||
| if (::LEDControl.get_mode_index() != led_mode_id_) | |||
| return EventHandlerResult::OK; | |||
|
|
|||
| // The simplest thing to do is trigger on both press and release. The color | |||
| // will fade while the key is held, and get restored to full brightness when | |||
| // it's released. | |||
| ::LEDControl.get_mode<TransientLEDMode>()->map_[event.addr.toInt()] = 0xff; | |||
There was a problem hiding this comment.
I do feel like, for clarity, if this is setting the key to full brightness on release, even when we don't need it to, it's worth a comment explaining what's going on, so that future hackers don't need to go spelunking too deeply for intent.
There was a problem hiding this comment.
This comment was added when the API changed such that onKeyEvent only fires on press and release, whereas before that change it used to fire continuously while held too. So that comment was added to describe the degraded behaviour it had since that change.
I could certainly add a comment here saying this lights it up on press and release, but isn't that just what onKeyEvent means anyway, so wouldn't that be obvious to someone who knows the API?
But what if instead we remove the entire onKeyEvent handler? I don't think it adds anything. I just tried with it entirely commented out in the cpp and h file and it's working just fine.
There was a problem hiding this comment.
I added a commit removing the handler. Let me know what you think.
There was a problem hiding this comment.
If it still works right without the handler, then I approve :)
This closes keyboardio#1342. Signed-off-by: Bart Nagel <[email protected]>
|
Oops, I'm not used to signing commits. Squashed and signed. |
This closes #1342.
Thanks to @algernon for guidance. But any bugs are mine not his.
Questions:
updatethe right place for this? I've read also ofbeforeSyncingLedsandafterEachCycle, and I don't know which is most appropriate.