Refactor: `audioPlayer.ts`を新規作成し`audio.ts`の一部機能を移転 #1492

thiramisu · 2023-08-08T13:58:45Z

内容

HTMLAudioElementに関連する処理をaudioPlayer.tsに移動します。
ユーザー視点での変更はありません。

関連 Issue

ref #1475
上記の一環です。

src/store/audioPlayer.ts

Hiroshiba · 2023-08-13T15:23:46Z

@y-chan さんに当たっていますがお忙しそうなので僕がレビューしたいと思います！

src/components/AudioDetail.vue

src/store/audioPlayer.ts

src/store/audio.ts

src/store/audioPlayer.ts

Co-authored-by: Hiroshiba <[email protected]>

`PLAYLIST`をAUDIOS`に `STOP_AUDIO`の引数名を変更

Co-authored-by: Hiroshiba <[email protected]>

src/store/audio.ts

This reverts commit 5fa4201.

thiramisu · 2023-08-20T04:45:25Z

今更ですが、レビューありがとうございます！
コメントの方はひとまずは全て解決したと思われるので、確認お願いします🙇

Hiroshiba

申し訳ありません、あと少しかなと思って自分で微調整してマージしようかなと思って色々細かくコメントしていたのですが、ちょっと改修しておいた方が良さそうな点を発見しました。
（nowPlayingAudioKeysがnowPlayingContinuouslyAudioKey含まれなくなった点です。）

ちょっと提案なのですが、AudioPlayerに移動するだけのプルリクエストにしていただくことは可能でしょうか･･･？
というのもレビューする際に移動、移動だけであれば簡単なのですが、一緒に引数の追加や機能の分解や実装の変更があると同じ動作をしているのかをチェックするのが結構大変だったりしてしまうためです。
（ということをドキュメントに書いておくべきでした、大変申し訳ないです。。）

ここの考え方に関して何か参考資料がないかと思ってちょっと探してみました。
https://google-engineering-practices.translation.shuuji3.xyz/review/developer/small-cls.html#refactoring

多分このプルリクエストは↓3つが行われていて、可能であればそれぞれ分けてのプルリクエストだと非常に嬉しいなと･･････！

audioPlayerの移転
名称変更・一部getter化のリファクタリング
連続再生ロジックの変更

本来であればリファクタリングはサクサク行えるものだと思うのですが、エディタはテストがほとんど進んでおらず、かつ連続再生回りはe2eテストも行いづらいため、どうしても丁寧にレビューを行う必要があるという感じです･･･。
最初に気づくべきでした、本当に申し訳ないです 🙇

Hiroshiba · 2023-08-20T17:47:57Z

src/store/audioPlayer.ts

+        beforePlayFn,
+      }: {
+        audioKeys: AudioKey[];
+        beforePlayFn: (audioKey: AudioKey) => Promise<void>;


単語省略はできる限り避けた方がよかったりします。
参考：https://qiita.com/alt_yamamoto/items/25eda376e6b947208996#%E5%8D%98%E8%AA%9E%E3%82%92%E7%9C%81%E7%95%A5%E3%81%99%E3%82%8B
（個人的には略し方が人それぞれで振動するので、だったら略さない方がいいかなという思いです）

Hiroshiba · 2023-08-20T18:10:28Z

src/store/audioPlayer.ts

+        );
+      }
+    },
+    action(_, { nowPlayingAudioKey }: { nowPlayingAudioKey: AudioKey }) {


（これは全く問題ないかもなのですが）
現在再生中以外のものでストップさせたいものがないので、nowPlayingは冗長かもとちょっと思いました。

･･･という説明は結構言い訳みたいな感じになっています。
というのも、多分audioKeyでも問題なくて、PLAY_AUDIOはそうなっているから揃えるためにaudioKeyにした方が良さそうだけど、「良さそう」以上の理由を探してこうなりました･･･。

Hiroshiba · 2023-08-20T18:28:33Z

src/store/audioPlayer.ts

+            dispatch("SHOW_ALERT_DIALOG", {
+              title: "再生エラー",
+              message:
+                "再生デバイスが見つかりませんでした。設定 / オプション から再生デバイスを設定しなおしてみてください。",


エラーが出た時の案内は、確かに解決策が具体的な方が良いのですが、設定の場所って変わりうるんですよね･･･。
そうなった時むしろこれを書いていた方が混乱するのですが、良い解決策は思いつきませんでした･･･。
とりあえずオプションの場所はあまり変わらなさそうなので、一旦大丈夫そう。（どうすればいいんですかね。。。）

Hiroshiba · 2023-08-20T18:33:56Z

src/store/audio.ts

+        const bufStartPoint = state.audioPlayStartPoint;
+        const currentAudioKey =


bufとcurrentで、やってることが同じだけど表記が揺れてそうです。
正直currentは何かおかしいんですが、まあちょっとそっちに寄せるのが良さそう？
（ChatGPTに聞いたところ、backupやoriginalの提案がありました https://chat.openai.com/share/701a5fb7-dc5d-46df-b1b0-fed9442fd62b ）

Hiroshiba · 2023-08-20T18:42:32Z

src/store/audio.ts

-      }
+  PREPARE_AND_PLAY_AUDIO_CONTINUOUSLY: {
+    action: createUILockAction(
+      async ({ commit, dispatch, state }, { audioKey }) => {


多分audioKey指定は使われない気がします。
複数選択範囲を連続再生するなどの場合はaudioKeysが渡されるので。
別のとこでも書いたかもなのですが、後々使うかもしれない機能は結局使わないことが大半（この場合だと消していいのかもパッとわからない）ので実装しない方が多分良いかなと！

Hiroshiba · 2023-08-20T18:51:19Z

src/store/audioPlayer.ts

+  PLAY_AUDIOS: {
+    mutation(state, { audioKey }: { audioKey: AudioKey }) {
+      state.nowPlayingContinuouslyAudioKey = audioKey;
+    },


あ、こちら解決してないかもです！
https://github.com/VOICEVOX/voicevox/pull/1492/files#r1293824232

正直なところ、やっていることがPLAY_AUDIOSと関係なく、nowPlayingContinuouslyAudioKeyの設定をやっているので、以前（SET_NOW_PLAYING_CONTINUOUSLY）と同様に別の関数にしてあげるのが良いのかなと思いました。

Hiroshiba · 2023-08-20T19:10:20Z

src/store/type.ts

+  nowPlayingAudioKeys: AudioKey[];
+  nowPlayingContinuouslyAudioKey?: AudioKey;


今気づいたのですが、通常の再生と連続再生で保存されるAudioKeyの場所が異なることになってそうです。
つまり、今までは連続再生している時にもnowPlayingAudioKeysに該当する制御変数が変化していたのですが、この設計だと別々になっているので、連続再生中はnowPlayingAudioKeysが空っぽという予測しづらい状況になっていそうです。

nowPlayingContinuouslyAudioKeyをやめ、以前と同様にnowPlayingAudioKeysと連続再生中かどうかの制御変数に戻すのはどうでしょう。
あるいは両方とも_をつけてプライベートっぽい変数名にし、必要な情報はゲッター関数にしてしまうとかでしょうか。

（多分将来の連続再生などの設計も見越した上での変更だったと思うのですが、その実装がないのでこの変更で行けるかわからないこともあり、とりあえず戻す方向に倒したいかもです。申し訳ない。。。）

Hiroshiba · 2023-08-20T19:25:04Z

src/store/audioPlayer.ts

+  STOP_AUDIOS: {
+    mutation(state) {
+      state.nowPlayingContinuouslyAudioKey = undefined;
+    },
+    action({ state, commit, dispatch }) {
+      const nowPlayingAudioKey = state.nowPlayingContinuouslyAudioKey;
+      if (nowPlayingAudioKey !== undefined) {
+        dispatch("STOP_AUDIO", { nowPlayingAudioKey });
+      }
+      commit("STOP_AUDIOS");
+    },
+  },


関数の名称とやっている内容が乖離していそうです。
多分PLAY_AUDIOSとの対象だからこの名称だと思うのですが、複数のオーディオの停止などと混同されそうです。

対象的にさせるのであれば、元の名称の「連続再生を開始する」「連続再生を停止する」の方が分かりやすいかもです。

thiramisu · 2023-08-21T17:19:12Z

PR周りに関してのガイドはあまり読めていなかった感じがあるので、ちょっとこの機会に色々記事を読んでみました。前は右から左に抜けてたんですが、ちゃんと読めるようになってました！
なんですが、「何を目指した方が良いのか」ばかりで、「これをすれば自然に良いPRになる」みたいな方法があれば良いのになあと思ってしまいました…。ということで、ちょっと自分なりの手順を文章化してみてます。

stmtk1 · 2023-08-21T17:21:17Z

PR周りに関してのガイドはあまり読めていなかった感じがあるので、ちょっとこの機会に色々記事を読んでみました。前は右から左に抜けてたんですが、ちゃんと読めるようになってました！なんですが、「何を目指した方が良いのか」ばかりで、「これをすれば自然に良いPRになる」みたいな方法があれば良いのになあと思ってしまいました…。ということで、ちょっと自分なりの手順を文章化してみてます。

ありがとうございます。文章化していただけると助かります

Hiroshiba · 2023-08-21T19:28:51Z

@thiramisu 了解です！！
貢献者ガイドラインを作成しようとしてくださった方がいらっしゃいます。参考になるかもなので共有です！

貢献者ガイドラインの作成 #1202

「何を目指した方が良いのか」ばかりで、「これをすれば自然に良いPRになる」みたいな方法があれば良いのになあ

目標と対応する手段列挙とかがいいのかな？とか思いました！
（僕は結構手段だけあっても気持ちが分からないと身につかないので、目標が書いてくれてると嬉しかったりします。どっちも必要そう。）

thiramisu · 2023-08-25T09:46:09Z

PRを分けられればその方が良さそうというのはなんとなく分かりました！
ちなみに手順としては、現状だとそもそもaudioPlayerに分割するための境目が無い状態なので、多分、まず関数分割＆命名（名前変更）をしてから次にファイルを分け（audioPlayer.tsへの移転）、その後に色々手を加える感じになるかなと思います。

#1492 (comment)
あと、切り分け作業をしつつ上のコメントを見ていたら思いついたのですが、将来をひとまず考えないなら、現状同時に複数再生する必要はないはずなので、

const audioElements: Record<AudioKey, HTMLAudioElement> = {};

を

const audioElement = new Audio();

としてしまうことで何か所か単純化できる気がしました。再生中の音声が高々一つになるので、そこら辺の実装を省けそうかなと。結構レビューしていただいた範囲も変更が必要そうなので、そこはアレなんですが…。
今のところは音声を複数再生できた方が良いissueは無いはず…？

Hiroshiba · 2023-08-26T03:57:02Z

@thiramisu
プルリクエストを分けていく方針に関して、やりやすい形でやっていただければと思います！！
確かに名前を変えてからの方が意味を切っていく上でやりやすいのかなと感じました。
もしコード分割と名称変更を同時にしないと意味が分かりづらかったら、そう伝えいただければこの2つをまとめて1つのプルリクエストにしていただければ！

現状同時に複数再生する必要はない

こちらに関しても賛成です。
仮に同時再生する機能があっても聞き比べ程度しか需要がなさそうで、もしその機能を実装する場合も任意の個数でなく2つあれば良さそうですし。
あとはCeVIOみたいにタイムライン編集できると嬉しいかもですが、これも別の実装になりそうなので、１つだけにするので良いのかなと思いました！

thiramisu · 2023-08-26T07:32:10Z

ちょっと再構成するのに新しいブランチから始めた方がやりやすそうなので、申し訳ないんですが一旦このPRはcloseさせていただこうかと思います。
まだ反映できてないコメントはありますが、再度送る際になるべく反映し、その時点でResolveしようと考えてます。
またよろしくお願いします。

Hiroshiba · 2023-08-27T17:09:10Z

承知しました！！
Resolveや議論コメントの転写がコメントであれば、一旦マサラのところから始めていただいても大丈夫です！
プルリクエストお待ちしています･･･！

thiramisu added 3 commits August 8, 2023 22:32

audioPlayer.tsを新規作成しaudio.tsの一部機能を移転

51090ba

一部をcomputed化

f20b0b3

不要なPromiseを削除

44e0e43

thiramisu requested a review from a team as a code owner August 8, 2023 13:58

thiramisu requested review from y-chan and removed request for a team August 8, 2023 13:58

thiramisu changed the title ~~Refacor: audioPlayer.tsを新規作成しaudio.tsの一部機能を移転~~ Refactor: audioPlayer.tsを新規作成しaudio.tsの一部機能を移転 Aug 8, 2023

thiramisu commented Aug 8, 2023

View reviewed changes

src/store/audioPlayer.ts Outdated Show resolved Hide resolved

thiramisu mentioned this pull request Aug 8, 2023

store/audio.ts周りのリファクタリング #1475

Open

26 tasks

Hiroshiba requested review from Hiroshiba and removed request for y-chan August 13, 2023 15:23

sevenc-nanashi reviewed Aug 14, 2023

View reviewed changes

src/components/AudioDetail.vue Outdated Show resolved Hide resolved

src/components/AudioDetail.vue Outdated Show resolved Hide resolved

NOW_PLAYING_CONTINUOUSLYの名前をIS_...に変更

2e3c382