refactor: refactor HTMLAudioPlayer #316
Conversation
| asset: HTMLAudioAsset; | ||
| } | ||
|
|
||
| export class HTMLAudioPlayerContext { |
There was a problem hiding this comment.
HTMLAudioElement を直接操作する抽象化的なクラスを新規で作成します。
There was a problem hiding this comment.
切り出す方針同意なのですが、名前づけが難しいですね。
- AudioPlayer と g.AudiPlayContext
- HTMLAudioPlayer と HTMLAudioPlayerContext
で "player" と "context" の関係が逆になっているように思います (前者は context が player で実装されている一方、後者は context で player が実装されている) 。 "play context" と "player context" で別の語だと解釈することもできそうですが、名前からはあまり違いが読み取れなさそうです。
コメントされているとおり主眼は HTMLAudioElement の制御にあると思いますから、いっそのこと PDI 切り離して、純粋に 「HTMLAudioElement を制御する役」にしてしまうのはどうでしょうか？ HTMLAudioElement (cloneElement() された) と、duration や offset を生成時の引数として受け取る形にすれば、PDI 依存もなくなると思います。("たまたま" asset と同じ形の interface で duration, offset, loopOffset などを受け取ることもできると思います)
この時、名前は AudioElement を軸に命名できそうです。それでも名付けにくいですが、例えば LoopableAudioElement とか、 AudioElementController とかですかね。
There was a problem hiding this comment.
おっしゃるとおり 「HTMLAudioElement を制御するプレイヤー実装」という切り出し方にした方がシンプルになりそうなので、88025ed にて修正しました。
名前については変に長くなっても分かりづらいだけな気がしたため AudioElementPlayer としました。
| this._previousCurrentTime = element.currentTime * 1000; | ||
|
|
||
| if (this.offsetEnd < currentOffset + deltaSinceLastCall || this.offsetEnd <= currentOffset) { | ||
| this._setRewindTimer(deltaUntilEnd); |
There was a problem hiding this comment.
HTMLAudioPlayer が複雑になっている要因は「複数箇所で同じような処理をしている」「それらの内どちらかの処理が終わっていればもう一方の処理を中断する」というような状態管理によるものだと仮定しました。
その仮定のもと
- 音声の再生・停止・巻き戻しを機能単位で切り分け
HTMLAudioPlayerはそれらの機能を呼ぶだけ
- 音声が終端に達したときの処理 (非ループの場合は停止、ループの場合は先頭に戻るなど) を一元化
するように修正しました。
|
|
||
| setupChromeMEIWorkaround(audio); | ||
| audio.volume = this._calculateVolume(); |
There was a problem hiding this comment.
この箇所に対応する、 play() 時の HTMLAudioElement#volume の設定箇所が消えたように見えますが、外部影響ないでしょうか？
There was a problem hiding this comment.
見落としてました。5c4045f にて修正済みです。
| asset: HTMLAudioAsset; | ||
| } | ||
|
|
||
| export class HTMLAudioPlayerContext { |
There was a problem hiding this comment.
切り出す方針同意なのですが、名前づけが難しいですね。
- AudioPlayer と g.AudiPlayContext
- HTMLAudioPlayer と HTMLAudioPlayerContext
で "player" と "context" の関係が逆になっているように思います (前者は context が player で実装されている一方、後者は context で player が実装されている) 。 "play context" と "player context" で別の語だと解釈することもできそうですが、名前からはあまり違いが読み取れなさそうです。
コメントされているとおり主眼は HTMLAudioElement の制御にあると思いますから、いっそのこと PDI 切り離して、純粋に 「HTMLAudioElement を制御する役」にしてしまうのはどうでしょうか？ HTMLAudioElement (cloneElement() された) と、duration や offset を生成時の引数として受け取る形にすれば、PDI 依存もなくなると思います。("たまたま" asset と同じ形の interface で duration, offset, loopOffset などを受け取ることもできると思います)
この時、名前は AudioElement を軸に命名できそうです。それでも名付けにくいですが、例えば LoopableAudioElement とか、 AudioElementController とかですかね。
| export interface HTMLAudioPlayerContextParameterObject { | ||
| asset: HTMLAudioAsset; | ||
| export interface AudioElementPlayerParameterObject { | ||
| id: string; |
There was a problem hiding this comment.
id 使ってなさそうですし定義的にもアセットのIDであって HTMLAudioElement の再生には関係なさそうなので削りませんか。
There was a problem hiding this comment.
かなり歪な存在ではありますが、同一アセットの判定に使っているので一旦このままとさせてください。
このPullRequestが解決する内容
HTMLAudioPlayerをリファクタリングします。HTMLAudioPlayerとHTMLAudioElementの間にHTMLAudioPlayerContextを追加します。仕様
durationが未定義の場合は全体を再生しますoffsetが未定義の場合は最初から再生しますloopOffsetが未定義の場合はループ時に最初から再生します