Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor animation cycling #50

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 4 additions & 44 deletions lib/xebow/engine.ex
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@ defmodule Xebow.Engine do

use GenServer

import Xebow.Utils, only: [mod: 2]

alias Xebow.Animation

defmodule State do
Expand Down Expand Up @@ -35,22 +33,6 @@ defmodule Xebow.Engine do
GenServer.start_link(__MODULE__, {initial_animation, paintables}, name: __MODULE__)
end

@doc """
Cycle to the next animation and play it.
"""
@spec next_animation :: :ok
def next_animation do
GenServer.cast(__MODULE__, :next_animation)
end

@doc """
Cycle to the previous animation and play it.
"""
@spec previous_animation :: :ok
def previous_animation do
GenServer.cast(__MODULE__, :previous_animation)
end

@doc """
Play the given animation.

Expand Down Expand Up @@ -145,31 +127,8 @@ defmodule Xebow.Engine do
end

@impl GenServer
def handle_cast(:next_animation, state) do
animation_types = Animation.types()
num = Enum.count(animation_types)
current = Enum.find_index(animation_types, &(&1 == state.animation.type))
next = mod(current + 1, num)
animation_type = Enum.at(animation_types, next)
animation = Animation.new(type: animation_type)

{:noreply, set_animation(state, animation)}
end

@impl GenServer
def handle_cast(:previous_animation, state) do
animation_types = Animation.types()
num = Enum.count(animation_types)
current = Enum.find_index(animation_types, &(&1 == state.animation.type))
previous = mod(current - 1, num)
animation_type = Enum.at(animation_types, previous)
animation = Animation.new(type: animation_type)

{:noreply, set_animation(state, animation)}
end

@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
Copy link
Member Author

Choose a reason for hiding this comment

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

@nickdichev can you please make sure I didn't break any of your logic?

current_animation = state.animation
expected_duration = Animation.duration(animation)
Process.send_after(self(), {:reset_animation, current_animation}, expected_duration)
Expand All @@ -188,7 +147,8 @@ defmodule Xebow.Engine do
end

@impl GenServer
def handle_call({:play_animation, %{loop: loop} = animation}, from, state) when loop >= 1 do
def handle_call({:play_animation, %{loop: loop} = animation}, from, state)
when is_integer(loop) and loop >= 1 do
current_animation = state.animation
duration = Animation.duration(animation)
Process.send_after(self(), {:reply, from, current_animation}, duration)
Expand Down
76 changes: 65 additions & 11 deletions lib/xebow/keys.ex
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,25 @@ defmodule Xebow.Keys do

# Client

def start_link([], opts \\ []) do
GenServer.start_link(__MODULE__, [], opts)
@spec start_link(any()) :: GenServer.on_start()
def start_link(_) do
GenServer.start_link(__MODULE__, [], name: __MODULE__)
end

@doc """
Cycle to the next animation
"""
@spec next_animation() :: :ok
def next_animation do
GenServer.cast(__MODULE__, :next_animation)
end

@doc """
Cycle to the previous animation
"""
@spec previous_animation() :: :ok
def previous_animation do
GenServer.cast(__MODULE__, :previous_animation)
end

# Server
Expand Down Expand Up @@ -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()
Copy link
Member

@amclain amclain Jul 4, 2020

Choose a reason for hiding this comment

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

ℹ️ (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)

|> Enum.map(&Animation.new(type: &1))

{:ok,
%{
pins: pins,
keyboard_state: keyboard_state,
hid: hid
hid: hid,
animations: animations,
current_animation_index: 0
Comment on lines +131 to +132
Copy link
Member

Choose a reason for hiding this comment

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

ℹ️ (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.

Copy link
Member

Choose a reason for hiding this comment

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

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

}}
end

@impl GenServer
def handle_cast(:next_animation, state) do
next_index = state.current_animation_index + 1

next_index =
case next_index < Enum.count(state.animations) do
true -> next_index
_ -> 0
end

animation = Enum.at(state.animations, next_index)

Xebow.Engine.play_animation(animation)

state = %{state | current_animation_index: next_index}

{:noreply, state}
end

@impl GenServer
def handle_cast(:previous_animation, state) do
previous_index = state.current_animation_index - 1

previous_index =
case previous_index < 0 do
true -> Enum.count(state.animations) - 1
_ -> previous_index
end

animation = Enum.at(state.animations, previous_index)

Xebow.Engine.play_animation(animation)

state = %{state | current_animation_index: previous_index}

{:noreply, state}
end

@impl GenServer
def handle_info({:hid_report, hid_report}, state) do
IO.binwrite(state.hid, hid_report)
{:noreply, state}
end

@impl GenServer
Copy link
Member

Choose a reason for hiding this comment

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

I thought you only needed the @impl on the first function body.

Copy link
Member

@amclain amclain Jul 4, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

def handle_info(:update_pin_values, state) do
new_pins =
Enum.map(state.pins, fn {pin_number, pin_ref, old_value} ->
Expand Down Expand Up @@ -158,14 +220,6 @@ defmodule Xebow.Keys do
Xebow.Engine.play_animation(animation, async: false)
end

def next_animation do
Xebow.Engine.next_animation()
end

def previous_animation do
Xebow.Engine.previous_animation()
end

def start_wifi_wizard do
case VintageNetWizard.run_wizard() do
:ok -> flash("green")
Expand Down