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

ソング:ループの機能追加 #2506

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

romot-co
Copy link
Contributor

@romot-co romot-co commented Jan 26, 2025

内容

以下の内容を行います

  • ループ範囲をドラッグおよびメニューで追加・削除できるようにする
  • ループをON/OFFできるようにする
  • ループ設定をプロジェクトに保存し復帰できるようにする
  • ミニマップ(トラックリスト)からもループすることを見越す
  • storybookにLoopLaneを追加

関連 Issue

ref #2224
close #2224

スクリーンショット・動画など

2025-01-26.20.46.43.mp4
スクリーンショット 2025-01-26 20 50 04 スクリーンショット 2025-01-26 20 49 44

その他

  • マージの解決がややこしかったため、別ブランチ・Issueとして作成いたします
  • Grid/ValueChanges/Loopなどレーンごとにコンポーネントとして分割(...逆に複雑になった気もしますが、コンテキストメニュー制御などがややこしかったため
  • Containerにロジック/Presentationはなるべくステートレスかつ表示のみの責務を目指したつもり

@romot-co romot-co requested a review from a team as a code owner January 26, 2025 12:02
@romot-co romot-co requested review from Hiroshiba and removed request for a team January 26, 2025 12:02
@romot-co romot-co marked this pull request as draft January 26, 2025 12:03
@romot-co romot-co changed the title ループの機能追加 ソング:ループの機能追加 Jan 26, 2025
@voicevox-preview-pages
Copy link

voicevox-preview-pages bot commented Jan 26, 2025

🚀 プレビュー用ページを作成しました 🚀

更新時点でのコミットハッシュ:c6b5717

@Hiroshiba
Copy link
Member

コンフリクトの解決とか、不明な点とかあれば何でも聞いてください!!!!

@romot-co romot-co marked this pull request as ready for review January 27, 2025 15:03
@romot-co
Copy link
Contributor Author

@Hiroshiba
おつかれさまです!(おてすきで)
テストにつきまして、engineMockのあたりがわかっておらず…どのようにすればよさそうでしょうか…!
https://github.com/VOICEVOX/voicevox/actions/runs/12991557907/job/36229342596?pr=2506

@Hiroshiba
Copy link
Member

テストにつきまして、engineMockのあたりがわかっておらず…どのようにすればよさそうでしょうか…!

ちょっと試した感じ、変更前に戻せばよさそうでした!!
勝手ながらコミット追加させていただきます! 🙏

聞いてくださってありがとうございます!!! また何でも聞いてください!!!

@romot-co romot-co requested a review from Copilot January 28, 2025 12:04
@romot-co
Copy link
Contributor Author

storybookに問題出ているのでそちら修正予定

@romot-co
Copy link
Contributor Author

romot-co commented Jan 30, 2025

そのままやるとSequencerRulerのContainer責務重すぎ&実装グダグダになったので子にそれぞれミニContainer持たせてロジックわける形にしたものの
親Container+各ロジックはcomposableに切り出す+各子コンポーネントはPresentation相当のみ…もありだったかも

@romot-co
Copy link
Contributor Author

とりあえずStorybookをstore使う形で以前と同様に動作させた
screenshot-localhost_6006-2025_01_31-02_30_07

Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

ちょっとコード眺めさせていただきました!
Container / Presentation / コンポーザブルをどこをどうするかめちゃくちゃ難しいですね!!!!!
どうすればいいか全然思いつかない!!!

ちょっと迷走させてしまう可能性がある気がするのですが、結構思いついたことベースでコメントさせていただくと・・・

  • ルーラーの中の表示物をそれぞれContainerにするのは、割りとアリ・・・・な気がする・・・?
    • もちろんコンポーザブルでも良さそうだけど、どっちがいいのか。。。。。
    • いやーーーーーーわっからない。。。。。
  • story内でPresentationのみを見たほうが良いか(Containerは含めないほうが良いか)
    • やはり理想的にはPresentationのみ、useStoreなしが良さそうな気はする
    • けどパラメーターが膨大なので本当にそれが正しいのかわからない。。。
  • もしかしたら人間が理解しやすいデータをPresentationに渡し、計算はPresentation内で行った方が良い・・・・・・・・?
    • 例えばテンポや拍子の配列や倍率を渡して、Presentation内で座標を求めるとか。。。。
    • ChatGPT君に聴いたところ、計算はContainerでやりましょうと言われました 😇

全然まとまらなかったです。。。
ContainerとPresentationの切れ目というか、Presentation側のI/Oというかを何にすればいいのかが難しい。。。

あ! stories作るのはすごく良いと思いました!!!
右クリックメニューもコンテナから与えるの思いつかなかったです。なるほどぉ。

ちょっとこの辺りの設計に関して、ルーラーのstoriesを作ったことのある @sevenc-nanashi さん的にはどうでしょう・・・・・・?
(どうと言われてもって感じかもですが。。。。)

@romot-co
Copy link
Contributor Author

romot-co commented Jan 31, 2025

@Hiroshiba
ありがとうございます…!
正直、それぞれ責務は別な気がするけども状態…ドラッグとか含めて揮発的な状態も…共有するしでだいぶ悩みます。

まあでも一晩経ってみて考えたところ
ヒホさん他みなさんにとってそんなに見通し悪くなければ現状の設計でいくでどうでしょうか!

ルーラーの中の表示物をそれぞれContainerにするのは、割りとアリ・・・・な気がする・・・?
もちろんコンポーザブルでも良さそうだけど、どっちがいいのか。。。。。

確かにどっちもありえそうです!
ただ親のSequencerRulerはただの箱かもなので、子にContainer持たせる&共有状態はstoreの形でもいいかなーと。
親はあとは再生ヘッドしか持っていなくて、これをPlayheadなどとして分離すればあとは空箱…?

もしかしたら人間が理解しやすいデータをPresentationに渡し、計算はPresentation内で行った方が良い・・・・・・・・?
例えばテンポや拍子の配列や倍率を渡して、Presentation内で座標を求めるとか。。。。

おっしゃるとおり、どこまでがPresentationの役割かほんと難しいなと...。
人間に理解しやすい形のデータっていう線、よさそうに思えます!

感覚的にはPresentationというとなるべくステートレスにしてあとはアクションをコールバックするぐらい…?な雑さなのですが、Reactでありがちな方言とかかもです。わからない!


個人的には責務分担は以下の認識です。

共有する状態はStoreに持っていき、
共有するUIロジックは今後重なるものが出てきたらcomposableにまとめる形かなと思っていますが
方針としてこうした方がいいとか誤解とかあればお知らせください!

Container層:
UIのコンポーネント単位
ストアから依存を注入
イベントハンドリングやUI表示に必要な計算・ロジック
計算結果やイベントをPresentationに渡す

Presentation層:
表示やレイアウトに徹する
Containerから受け取った計算済みデータを表示する
イベントをContainerに返す

Composable:
Containerで行うような計算のうち共通化したいものを切り出す
ドメイン層との違いはVueのリアクティブシステムに組み込まれているか
Reactのカスタムhookよりもなんか自由

Domain層:
純粋なロジック
composableとの境界はVueに依存しているか否か

Store:
アプリ全体の状態管理
シングルソースに徹する
基本的にロジックは持たない

@Hiroshiba
Copy link
Member

僕もさらに考えたのですが、大体同じ意見だと思います!
一旦今の形でレビューさせていただくのも問題ないと思います!


1点追加で考えたことが。
完全に閉じているコンポーネント、つまりVuexに依存しないし、Domainにも依存しないようなものは、イベントや計算が多少生じてもContainerを用意しなくても良いよなーと思いました!
例えばよくあるスナップ付きのスライダーUIとか。

で↑はContainerが無い、つまりどっちかというとPresentationなんですよねー・・・。
なのでContainerがないなら、PresentationがイベントハンドリングやUI表示に必要な計算・ロジックを持っても良さそう(持つべきかは現状未定)な気もしました!

今回の場合だと、GridLaneあたりはGridLane Containerがなくとも、SequencerRuler Containerから直接データをもらっても良さそうだなぁとか。
・・・・・この場合もSequencerRuler Containerから値をもらうことも、SequencerRuler Presentationからもらうこともできそうですが。。。。

わからない・・・・・・・!!!!!!

@romot-co
Copy link
Contributor Author

romot-co commented Feb 1, 2025

@Hiroshiba
ありがとうございます!
あとレビューしづらくて申し訳ありません…!(今後ファイル移動と実装追加もわけるようにします

完全に閉じているコンポーネント、つまりVuexに依存しないし、Domainにも依存しないようなものは、イベントや計算が多少生じてもContainerを用意しなくても良いよなーと思いました!
例えばよくあるスナップ付きのスライダーUIとか。

で↑はContainerが無い、つまりどっちかというとPresentationなんですよねー・・・。
なのでContainerがないなら、PresentationがイベントハンドリングやUI表示に必要な計算・ロジックを持っても良さそう(持つべきかは現状未定)な気もしました!

今回の場合だと、GridLaneあたりはGridLane Containerがなくとも、SequencerRuler Containerから直接データをもらっても良さそうだなぁとか。

こちらたしかに!
Presentationだけでいいっていうのも結構ありそうで、
どうするか悩むぐらいならPresentationのみ・あとで必要になったらContainer追加して分割とかもよさそう?に思えました!

やたら複雑にはせず、読みやすさが第一なのかなみたいなことも思うもののうまくまとまらない…

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ソング: ループ範囲を設定できるようにする
3 participants