refactor animation cycling #50
Conversation
Move the next_/previous_animation logic from Engine to Keys (the "application")
| @impl GenServer | ||
| def handle_cast({:play_animation, %{loop: loop} = animation}, state) when loop >= 1 do | ||
| def handle_cast({:play_animation, %{loop: loop} = animation}, state) | ||
| when is_integer(loop) and loop >= 1 do |
There was a problem hiding this comment.
@nickdichev can you please make sure I didn't break any of your logic?
There was a problem hiding this comment.
✔️ Looks good to me! The comments I left are informational to document where we may want to go from here; not actionable, but can be used for discussion.
I'll give @doughsay and @nickdichev a chance to chime in on this PR as well before we merge it.
| animations: animations, | ||
| current_animation_index: 0 |
There was a problem hiding this comment.
ℹ️ (informational, no action required)
Keeping this data in Keys feels a little weird long-term, but you also noted that in the PR description. I'm fine with this in this PR. In a future PR we may want to try to work this data into xebow.ex and use that for the application logic that glues the other components together.
There was a problem hiding this comment.
See my possibly related documentation update calling out the fact that keys.ex has too many responsibilities: https://github.com/ElixirSeattle/xebow/pull/52/files#diff-adc9367d84d7e3a10f70650eece5eae2R7-R14
| @@ -102,20 +119,65 @@ defmodule Xebow.Keys do | |||
| poll_timer_ms = 15 | |||
| :timer.send_interval(poll_timer_ms, self(), :update_pin_values) | |||
|
|
|||
| animations = | |||
| Animation.types() | |||
There was a problem hiding this comment.
ℹ️ (informational, no action required)
It would be nice to eventually move the runtime animation types into a file that handles application logic (xebow.ex). Thanks for opening that issue ticket!
Related: #49 (comment)
| @impl GenServer | ||
| def handle_info({:hid_report, hid_report}, state) do | ||
| IO.binwrite(state.hid, hid_report) | ||
| {:noreply, state} | ||
| end | ||
|
|
||
| @impl GenServer |
There was a problem hiding this comment.
I thought you only needed the @impl on the first function body.
There was a problem hiding this comment.
I think you're right. I'm fine either way. With @vanvoljg's approach it's harder to forget to add it back in if one is removed during refactoring.
There was a problem hiding this comment.
I thought it mostly made it clearer that it's a callback. I can omit in the future them if they make things too cluttered.
|
This PR closed when the branch from #47 auto-deleted when it was merged. I've restored the branch and reopened it. |
|
I forgot to change the target branch before merging. 🙈 Fixed in #53. |
Depends on #47
Move the next_animation/previous_animation logic from Engine to Keys (the "application") because Keys feels like a better place than application.ex because that's more for application startup and management. Keys still needs to be refactored somehow, but this pulls non-engine-related logic out of the engine code.
Fixes #36