-
Notifications
You must be signed in to change notification settings - Fork 309
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: audioElements
をRecordから単体にする
#1525
Conversation
アプリとしての動作は問題なさそうなんですが、ユニットテストが落ちるようになってしまったみたいなので、 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!!
現状の使用感が変化することなく単純化できていて、今後のコーディングのしやすさが向上しそうなのでこの方針で良さそうに感じました。
PLAY_AUDIO_BLOB
がglobalのaudioElementを利用するというコードの変更も入っていそうでした。
影響を受けるのは辞書周りだけで、まあそこまで不自然でない共通化ができるのでこの形で良さそう。
(辞書機能を作ってくださった @y-chan さんにも共有です!)
1つだけちょっとした提案を書いてみました。
そのままでもそこまで問題ではないと思うので、変更しない方が良さそうであればコメントいただければと思います。
レビューありがとうございます!
確かにです!今まで全然意識してこなかったのもあり、まだ分割が甘いですね、すいません… |
これくらいであれば言及さえあれば特に問題ないのかなと! 問題ないと思うのでマージします!実装ありがとうございました!! |
内容
store/audio.ts
のを
とします。
以前の実装は複数の同時再生をしたい場合は便利ですが、少なくとも現在は使われておらず、また将来的にも使われる可能性が低そうなため、変更しても大丈夫そうです。
ユーザー視点での変更はありません。
(かなり厳密に言うと、辞書登録で長い文章を読ませている最中に[キャンセル]>[閉じるボタン]後に詳細調整欄の再生ボタンを押すと辞書の読みが停止されるようになりますが、実質影響なしだと思います)
関連 Issue
store/audio.ts
周りのリファクタリング #1475上記の一環です。
audioPlayer.ts
を新規作成しaudio.ts
の一部機能を移転 #1492上記で話題に出ました。(具体的な該当コメントは「その他」を参照)
その他
該当コメント
(前略)将来をひとまず考えないなら、現状同時に複数再生する必要はないはずなので、
を
としてしまうことで何か所か単純化できる気がしました。再生中の音声が高々一つになるので、そこら辺の実装を省けそうかなと。結構レビューしていただいた範囲も変更が必要そうなので、そこはアレなんですが…。
今のところは音声を複数再生できた方が良いissueは無いはず…?
Originally posted by @thiramisu in #1492 (comment)
こちらに関しても賛成です。
仮に同時再生する機能があっても聞き比べ程度しか需要がなさそうで、もしその機能を実装する場合も任意の個数でなく2つあれば良さそうですし。
あとはCeVIOみたいにタイムライン編集できると嬉しいかもですが、これも別の実装になりそうなので、1つだけにするので良いのかなと思いました!
Originally posted by @Hiroshiba in #1492 (comment)