search

cloudnativedaysjp / dreamkast-ui

MIT License
7 stars 2 forks source link

Fix: disable countdown #374

Closed ureuzy closed 1 year ago

ureuzy commented 1 year ago

カウントダウンを消しました。伴ってTrack側のstate変数名が適切ではなくなったので修正しました。

github-actions[bot] commented 1 year ago

Review app

hrk091 commented 1 year ago

ご対応ありがとうございます！ カウントダウンだけが消えて、セッション切り替わり時の「キャンセル」「すぐに再生」ボタンを押さない限り自動で切り替わらず、残り続ける仕様になっていることを確認しました。

image

@jacopen @takaishi 上記件について、私もちゃんと確認できておらず申し訳ないのですが、カウントダウンに加えてセッション切り替わり時に上記の画面が出ること自体を止めて出さないようにする、というのが今回改修の要件かと認識していました。 山本さんに実施頂いた改修内容で問題ないか、確認いただけないでしょうか？

jacopen commented 1 year ago

@hrk091 @ureuzy

カウントダウンに加えてセッション切り替わり時に上記の画面が出ること自体を止めて出さないようにする、というのが今回改修の要件かと認識していました

はい、そもそもこの「キャンセル」「すぐに再生」も出さず、そのまま切り替わるというのが今回のやりたいことです！

ureuzy commented 1 year ago

@hrk091 1点確認させてください

次のuseEffectはオフライン参加者 & カウントダウン表示時のみに副作用関数が実行される様ですが、"オフライン参加者"と絞ってある意図を教えていただけますか？ https://github.com/cloudnativedaysjp/dreamkast-ui/blob/8eac9d9a19aaf3dd5415a92526ba45dfba10817b/src/components/Track/Track.tsx#L149-L156

現状websocketから配信の切り替えを受信した際に実行されるように変更したのですが、問題ありますでしょうか？ https://github.com/cloudnativedaysjp/dreamkast-ui/blob/e90dc35e284d39dbcaa0f279edca1d449d3b6da4/src/components/Track/Track.tsx#L160-L164

もともとカウントダウン後にすぐに再生を押すことで、当該関数が実行されるようになっていたのでそこまで問題ないのかな。とも思っております。 https://github.com/cloudnativedaysjp/dreamkast-ui/blob/8eac9d9a19aaf3dd5415a92526ba45dfba10817b/src/components/IvsPlayer/IvsPlayer.tsx#L45-L48

hrk091 commented 1 year ago

@ureuzy オフライン限定としている理由は、updateViewを実行する責務をIvsPlayerが持っており、（dk-uiで配信を視聴しているユーザは実行される一方で）配信をオフにして現地講演を聞きつつチャットだけ見ている人のセッション情報が更新されない事象が出て、その対処として入った実装だからです。対象がオフラインユーザだけなのでオンライン側に影響を及ぼさないようにしつつ、CNDT2022前日のリハで突貫で入った修正なので、このような対処になっています。 https://github.com/cloudnativedaysjp/dreamkast-ui/pull/345

オフラインユーザ限定にするガード節を外すと、オンラインユーザの場合IvsPlayerも updateView() を呼ぶため、必ず2回呼ばれることになります。 updateView() 自体は、見る限り冪等性のある処理に見えるので、複数回呼んでも大丈夫だと思います。

一方で、そもそもIvsPlayer（ビデオ表示を有効にしないとrenderingされないコンポーネント）がセッション更新の責務を持っていたことが、この問題の発端になっています。なので、セッション更新の責務をIvsPlayerから剥がして、より上位のコンポーネントもしくはモデル側（redux側）に持たせるのがベターです。 山本さんの修正内容はIvsPlayerから更新責務を剥がす変更なのでこの方針でいいと思います！ 1点、IvsPlayer側に責務が残っているとupdateViewが複数回呼ばれる形になるので、（冪等性があるから大丈夫とは言いつつも）可能ならIvsPlayer側は消してしまうのが望ましいです

ureuzy commented 1 year ago

@hrk091 ありがとうございます！

セッション更新の責務をIvsPlayerから剥がして、より上位のコンポーネントもしくはモデル側（redux側）に持たせるのがベターです

はい、私も同じことを思っておりました。先程のコメントがわかりづらく申し訳なかったのですが、最後の引用はIvsPlayer.tsのmainブランチを参照していて、今回私が修正したブランチではすでにIvsPlayerからセッション更新に関わる責務を全て排除済みです。

updateView() 自体は、見る限り冪等性のある処理に見えるので、複数回呼んでも大丈夫だと思います。

ありがとうございます。では一旦現状の修正で動作確認していただけますでしょうか？もし可能であればMergeもお願い致します