refactor: gridInfoではなくnumMeasuresをinjectするようにする #2605
Conversation
|
🚀 プレビュー用ページを作成しました 🚀 更新時点でのコミットハッシュ： |
Hiroshiba
left a comment
Hiroshiba
left a comment
There was a problem hiding this comment.
単一の情報源から各コンポーネントが必要な情報をどこで計算するべきか、かなり悩みますね 😇 😇 😇
昔 @romot-co さんともどこかで議論した記憶。。そして同じように答えは出なかった。。
各計算結果をprovideすると、各引数に整合性が生じてStorybookでテストしづらい。（今のmainブランチ）
各コンポーネントで計算すると、複数箇所で同じ処理を書かないといけない。（このブルリク）
解決策としてはたぶんこうかな～～～と
- どちらにせよ単一の情報源からそのコンポーネントが必要な情報を計算する関数を切り出す
- 計算結果をprovideするなら
- 全計算をルートコンポーネントで行ってprovide
- Storybook内で必要最低限の関数を実行
- 👍 同じ計算を２度行わない
- 🤔 実際のコードとStorybookのコードがずれうる
- コンポーネント内で計算するなら
- 単一の情報源をprovideし、コンポーネントには必要な情報のみ引数渡しする
- 各コンポーネント内で必要最低限の関数を実行
- 👍 Storybookが簡単
- 🤔 同じ計算を行う
このどっちかなのかな～～～と思いました！！！
どっちが良いかはぶっちゃけ結論出せてないです。。。
一旦考え聞けると！！（よかった @sevenc-nanashi さんも）
| const endMeasureNumber = computed(() => numMeasures.value + 1); | ||
| const endTicks = computed(() => { | ||
| return measureNumberToTick( | ||
| endMeasureNumber.value, | ||
| timeSignatures.value, | ||
| tpqn.value, | ||
| ); | ||
| }); | ||
| const gridWidth = computed(() => { | ||
| return tickToBaseX(endTicks.value, tpqn.value) * zoomX.value; | ||
| }); | ||
| const gridHeight = computed(() => { | ||
| return getKeyBaseHeight() * getNumKeys() * zoomY.value; | ||
| }); |
There was a problem hiding this comment.
コードがコピペになっているのが気になりました！
https://github.com/sigprogramming/voicevox/blob/52700c33c963878c99e863f49d8d5bc1eeb0a1e4/src/components/Sing/SequencerGridSpacer.vue#L25-L41
https://github.com/sigprogramming/voicevox/blob/52700c33c963878c99e863f49d8d5bc1eeb0a1e4/src/components/Sing/SequencerGrid/Presentation.vue#L136-L153
この辺り多分整合性取れなくなりうるので、provideで与えるかどうかともかく、DRY原則に従って共通化していくのが良いかなと思いました！
ちなみにgridWidthを以前共通化を提案したときのコメントはこんな感じでした。
#2472 (comment)
There was a problem hiding this comment.
（まず前提として：Storybookでプロパティの値を変更してもSequencerGridが壊れないようにする）
データの流れ（依存関係）は
numMeasures→endMeasureNumber→endTicks→gridWidth
となっていて、numMeasuresが決まればgridWidthは一意に決まるので、整合性が取れなくなる（仕様がずれる）ことはたぶんないと思いますが、おっしゃる通りコードが重複しているので、endTicksまでを親コンポーネントで計算するのも良いかもです。
その場合、endTIcksのみをprovideする形になり、SequencerGridのnumMeasuresへの依存を無くす（描画ロジックを変更する）作業が必要になりそうです。
もしくはnumMeasures→endMeasureNumber→endTicksまでの算出値をオブジェクトでまとめて、そのオブジェクトを子コンポーネント（SequencerGridなど）に渡すという形もあると思います。
整理すると：
endTicksまでを親で計算し、子はendTicksを受け取る形- 算出ロジックを共通化できる
SequencerGridの描画ロジックを変更する必要がある- 子は
numMeasuresに依存しないように（endTicksに依存するように）しないといけない
endTicksまでを親で計算し、子はnumMeasures→endTicksまでの算出値（オブジェクト）を受け取る形- 算出ロジックを共通化できる
SequencerGridの描画ロジックの変更は必要ない- 子は
numMeasures（を含むオブジェクト）に依存できる - オブジェクト内の値の依存関係が分からなくなりそう
numMeasuresまでを親で計算し、子はnumMeasuresを受け取る形（このPR）- 変更されやすいのは、おそらく
numMeasures→gridWidthの変換処理ではなくnumMeasuresの算出処理なので、numMeasuresをprovideして、比較的安定している（関数化されている）numMeasures→gridWidthの変換処理を子コンポーネントで行う（少しの重複は許容する）という考え
- 変更されやすいのは、おそらく
There was a problem hiding this comment.
計算結果をprovideするか、比較的情報源に近いものをprovideするか、どちらがいいのかちょっと現段階ではまだ見えてこないですね･･･！
少なくとも処理は共通化した方が良い気がしています。
どれくらいまとまりがあった時に共通化すべきなのが若干分かりませんが、個人的にはnumMeasures→endMeasureNumber→endTicks→gridWidthの流れは共通化した方が良いだろうな～とは感じます。
コメントを見て思ったのですが、出てきた課題は実は「全部の計算結果をprovideで渡し、子は計算を一切してはいけない」設計で解決できるかも･･･？
-
子はnumMeasuresに依存しないように（endTicksに依存するように）しないといけない
- 子の中で計算してはいけない設計なので、numMeasuresに依存した計算は書けない
-
オブジェクト内の値の依存関係が分からなくなりそう
- 全部の計算を1箇所行うのでわかりやすそう
-
numMeasures→endMeasureNumber→endTicks→gridWidthの流れは共通化した方が良い- 計算処理をprovideする親の中でしか書けないので、必ず共通化される
There was a problem hiding this comment.
コメントが遅くなりすみません…！
コメントを見て思ったのですが、出てきた課題は実は「全部の計算結果をprovideで渡し、子は計算を一切してはいけない」設計で解決できるかも･･･？
「子は計算を一切してはいけない」という制約は少し強すぎる気がしています。
コンポーネントはそれ単体で機能するようにした方が良いと思っていて、
子でもコンポーネントとして必要な計算は行うようにして、複数の子コンポーネントで共有すべき値は子に近い親コンポーネントで算出するのが良さそうに思います。
考えてみたのですが、numMeasuresからgridWidthまでの値はGridやRuler固有の値ではなく、シーケンサー全体で共通の（楽譜の長さを表す）値なので、共通化するのであればgridWidthまでをScoreSequencerで算出するようにして、SequencerGridはgridWidthのみに依存するようにした方が良いかもです。
いずれにしてもSequencerGridはnumMeasuresに依存してしまっているので、描画ロジックの変更は必要そうです。
ひとまずこのPRではSequencerGridの依存を一つにする（numMeasuresのみにする）を行って、描画ロジックの変更は別のPRで行えればと思います。
There was a problem hiding this comment.
複数の子コンポーネントで共有すべき値は子に近い親コンポーネントで算出するのが良さそうに思います。
そうかもです。
ただ、表示用のロジックを含むコンポーネントの親子構造は、データ構造と相性が良くない形になりうると思うんですよね。。
ちょっと現状だとどっちが良いかはわからないです･･･！ 😇
全部１箇所で作ったほうがコード変更が容易かもしれません。
どこに何置くか、誰が値を計算するのかを迷わなくて済むので･･･。
SequencerGridはgridWidthのみに依存するようにした方が良いかもです。
なるほどです！！
ひとまずこのPRではSequencerGridの依存を一つにする（numMeasuresのみにする）を行って、描画ロジックの変更は別のPRで行えればと思います。
今回のPRは依存を１つにする代わりに、このSequencerGridSpacer.vueにコピペーコードが発生してるので、一長一短なコードになってるんですよね･･･。
gridInfoはコードを共通化していて、今回のPRはそれを解いてデータを共通化してる感じかなーと。
まあでも一旦このPRの形にするのでも良いと思います！！
個人的にはnumMeasuresもgridWidthもgridHeightもprovideする形にしたあと、numMeasuresのprovideを失くすのを目指すのでもありだと思います。
There was a problem hiding this comment.
ただ、表示用のロジックを含むコンポーネントの親子構造は、データ構造と相性が良くない形になりうると思うんですよね。。
コンポーネントの親子構造とデータ構造（データの依存関係）は、（難しいとは思いますが）できるだけ一致させるようにするのが良いのかもと思っています。
（一致しないものはstoreで管理するのが良いかも）
迷うのですが、provide周りは後で変更も可能だと思うので、一旦このPRの形でマージできればと思います。
There was a problem hiding this comment.
たしかに目指せると嬉しそうだとは思います！！
ミニマップとか、物理的に離れてるとこに同じものを表示しないといけないことがちょくちょくありそうですが、なるべく･･･！
一旦マージ了解です。
ちなみにもしこのPRを前提に実装したものがいろいろあったりしたら、言っていただければと思います 🙏
その最終形を見て判断とかもできるはずなので･･･！
| @@ -1,17 +1,44 @@ | |||
| <template> | |||
| <div class="sequencer-grid-spacer"></div> | |||
| </template> | |||
There was a problem hiding this comment.
（ただのコメントです）
このコンポーネント今思ったらなくせそうですね！！
widhtとheight指定してdiv作ってるだけなので、ScoreSequencer.vue側でこんな感じで書けば良さそう？
<div style={
width: `${gridWidth}px`,
height: `${gridHeight}px`,
"pointer-events": "none",
}/>あるいはコンポーネント化するにしても、GridSpacerではなくただのSpacerとできそうだな～と。
Hiroshiba
left a comment
Hiroshiba
left a comment
There was a problem hiding this comment.
LGTM！！
マージさせていただきます！
ちょっとデータ管理周りは難しくてしばらくは振動する気がしますね･･･！
この辺りみんな困ってるはずではあるので、ネットで情報色々調べてみるといろいろ出てくるかも。（状態管理というジャンルだと思います）
2 participants
内容
SequencerGridはnumMeasures（小節数）のほかにgridWidthとgridHeightを受け取るようになっていますが、gridWidthはnumMeasuresに依存しているので、ストーリーブックでどちらか一方のみ変更するとSequencerGridが壊れます。なので、
numMeasuresのみ渡して、gridWidthはSequencerGrid内で算出するようにします。gridHeightもinjectするほど（共通化が必要なほど）複雑ではないので、これもSequencerGrid内で算出するようにします。（
SequencerGridSpacerの方も、SequencerGridと同様に変更します）また、
numMeasuresはシーケンサーでのみ使用されているので、domainのgetNumMeasuresとstoreのSEQUENCER_NUM_MEASURESは削除して、ScoreSequencer内で算出するようにします。（書き出し時に使用するようになるかもしれませんが、そのときに
store.getterにするのが良いかなと思います）その他