ソング：ノート追加のプレビュー処理をステートパターンで実装 #2171
Conversation
There was a problem hiding this comment.
コンポーネントとステートマシンをどう切り分けていくかの設計、とても難しいけど面白い課題だなと感じました！！
ちょっと色々コメントしちゃいました 🙇
でも基本的には正解が全くわからないので、なんとなく今考えている方針をすり合わせつつ、手探りで探していくのがいいのかなと思ってます！
なので最初の認識（未定なのであれば未定だという認識）を揃えておいて、ガシガシ進んで行けるといいのかなと！
ということでちょっと方針について2点聞きたいことが･･･！！
多分全部のStateを実装した後に置き換える、という流れを取ると思ってます！
流れは良さそうなのですが、ステートマシン関係のコードは今使われていないということがコードからわからず、mainブランチを見た新規のコミッターが混乱しちゃうかもです。
各ファイルの一番上に「今は実装中なので現在使われていません」的な文言と、リファクタリングissueへのリンクを貼っておくのとかどうでしょう？
いくつか設計に関して、もし考えたことがあれば聞いてみたく、いくつか最初に認識合わせしておきたいです･･･！
（ちょっと長くなってしまいましたがすみません。。）
- 1つのステートマシンは1つのコンポーネントと対応する？
今回だとSequencerStateMachineはScoreSequencerに1:1対応していると思っています。
個人的には、ステートマシンは兄弟コンポーネントを跨がない（子コンポーネントにわたすのはOK）設計にするのが妥当かなぁと思ってます！
まあここ柔軟に設計は変わっていきそうですが、一旦思いを聞いてみたく。
contextの中身の変更がStateMachine以外からもできるようになってるけど、意図した設計かどうか
多分ステートマシンのインスタンスを作る側でrefを作って投げる想定だと思います。
この形だとrefの中身を勝手に書き換えれちゃえそうです。
個人的には、それを承知で今の設計にするのが一旦良いと思ってます！
カプセル化するのであればコンポーザブル化するのも手かも。
こんな感じ？（Refを中で定義してComputedRefを返す）
// createStateMachineForSequencerの改変
export const useStateMachineForSequencer = () => {
const nowPreviewing = Ref(false)
// ほかも定義
const context: Context = {
nowPreviewing,
}
// dispatcherも似たように定義
const stateMachine = new StateMachine<States, Input, Context, Dispatcher>(
new IdleState(),
context,
dispatcher,
);
// get onlyで返す
return {
nowPreviewing: computed(() => nowPreviewing),
// dispatcherは返さなくて良いはず。コールバック用途なら`onHoge`関数を受け取る形にとか？
stateMachine
}
};- ステートマシン意外とのやり取り（Dispatcher）をイベント型にするか処理をDIする形にするか
たぶん一般的には、ステートマシンが扱う全部のコンテキストをContextに乗せるのが普通だと思うのですが、VOICEVOXは一部の処理がVuex管理になっているため外とのやり取りが発生すると思います！
この時コールバック方式（イベント型？onAtoBみたいな）にするか、今のプルリクエストみたいな形で関数を大量にDIするかの二択があると思うのですが、後者が一般的なのでしょうか･･･？
個人的にも今のプルリクエストの形の方が良いと思っていますが、何か情報お持ちであればと思ってお聞きしてみた次第です！
|
@Hiroshiba
ファイルの一番上にコメントを追加しました！
コンポーネントを
とりあえず
関数をDIする形が一般的かは分からないのですが、 |
| type StoreActions = { | ||
| readonly deselectAllNotes: () => void; | ||
| readonly commandAddNotes: (notes: Note[]) => void; | ||
| readonly selectNotes: (noteIds: NoteId[]) => void; | ||
| readonly playPreviewSound: (noteNumber: number, duration?: number) => void; | ||
| }; | ||
|
|
||
| type Context = ComputedRefs & Refs & { readonly storeActions: StoreActions }; |
There was a problem hiding this comment.
Dispatcherを無くして、storeActionsとしてContextに入れました。
There was a problem hiding this comment.
（ただのコメントです）
computedRefsとrefsも{computed: ComputedRefs}などとしても良いかも？
どれがrefでどれがcomputedRefかぱっとわかりやすく、間違えてsetしようと思いづらくなりそう！
まあ今は把握しきれそうなので、もっと複雑化してきたときでも良さそう。
|
なるほどです、もろもろ納得です！
これは完全にやりやすい方で、という感じです！ |
There was a problem hiding this comment.
ほぼLGTMです！！
ブランチ作って良さそうであれば作ります！！ブランチ名はproject-song-sequence-statemachineとか？
専用のissueと、そのissue内に箇条書きのタスクリストがあると他の方も分かりやすいかもです。
新しくissue作るかによらず、後から見返しやすいよう、全プルリクが同じissueをリンクし続ける感じにしたみ。
いくつか、ここできそうだな～でも思いつかないな～っていう設計を書いてみました！
ただどんな実装が良いのか現状分からないので、まずは細かいこと気にせずにガシガシ実装が良さそう！
１．たぶんStateごとに、何のInput・次Stateが許されているか定義できるようにも作れる。
AddNoteStateはクリックを離すInputのみ受付可能、みたいなのを型で指定できる感じ。
今のコードだと全てのStateが、全てのStateに存在しうる全てのInput・Stateを想定しないといけないはず。
想定しないものが来た場合は何も処理しない、言い換えると想定外の処理が来ても気付けないはず。（･･･もしかしたらそういう設計が普通･･･？）
たぶん型の実装をうまいことやるとこの辺りも制限できる。
２．各Stateのインスタンス変数は、初期値は不要なはず。
でも今はcontextが渡ってくるonEnter内でprocessで使うデータをから情報を作ってインスタンス変数に代入している関係で、undefinableになってる。
存在しない型が存在している関係で、ちょっとミスが起こりやすいコードになってる。
（AddNoteStateのthis.noteToAddがundefineなことはないはずだけど、undefinedが許されてる）
たーーーーーーぶんconstructerとonEnter、もしくはonEnterとprocessをくっつければ初期値不要にできる･･･はずだけど、どういう設計が良いのかわからない。。。
前者だとconstructerにcontextを渡す設計にすればOK。
後者だとonEnterの返り値でデータを返し、processに渡す設計にすればOK（型パズルがだいぶややこしい。僕の知識だとかなりきつい。。）
| process(input: Input) { | ||
| try { | ||
| this.currentState.process({ | ||
| input, | ||
| context: this.context, | ||
| setNextState: this.setNextState, | ||
| }); | ||
| if (this.nextState != undefined) { | ||
| this.currentState.onExit(this.context); | ||
| this.currentState = this.nextState; | ||
| this.currentState.onEnter(this.context); | ||
| } | ||
| } finally { | ||
| this.nextState = undefined; | ||
| } |
There was a problem hiding this comment.
こんな感じにしちゃえばthis.setNextState()とthis.nextStateなくせるかもです！
（インスタンス変数this.nextStateをなくせるのはバグ減らす意味で結構大きそう？）
process(input: Input) {
let nextState = undefined;
this.currentState.process({
input,
context: this.context,
setNextState: (s) => { nextState = s },
});
if (nextState != undefined) {
this.currentState.onExit(this.context);
this.currentState = nextState;
this.currentState.onEnter(this.context);
}
}あるいはprocessでState | undefinedをreturnしてもらう手もあるかも･･･？
必ず同期的に処理してほしい（promise内でsetNextStateを呼ばないでほしい）気がするので、どっちかというとreturnのが良い･････････のかも･･･？（自信なし）
There was a problem hiding this comment.
こちら変更しました！
State | undefinedをreturnしても良いかもですが、一旦今の形で実装できれば…！
type-challengesをやってみようと思います…ひとまず今の形で一旦実装できれば…！
Stateのprocessメソッド内で無視するのではなく、無視する（または受付可能な）Inputを各ステートで型で定義しておくということでしょうか？
constructorとonEnterをくっつけるのが良いかもと思いましたが、以下の問題もあるかもです。
|
|
そういうイメージでした！
なーるほどです。たしかに課題もありそうに感じます。 別案として、onEnterが終わったあとにできるインスタンス変数をまとめちゃうとか･･･？ class AddNoteState implements IState<State, Input, Context> {
private readonly cursorPosAtStart: PositionOnSequencer;
private innerContext: { // process終わったあとにできるものもありえるなら、enteredContextとかのが良いかも
noteToAdd: Note;
previewRequestId: number;
executePreviewProcess: boolean;
currentCursorPos: PositionOnSequencer;
} | undefined = undefined
onEnter(context: Context) {
// 色々処理する
this.innerContext = {noteToAdd, previewRequestId, }
}
onExit(context: Context) {
// 必要なのだけ展開する感じとか
const {previewRequestId} = this.innerContext;
}
}個人的に初期値は結構ミスを誘発する（初期化忘れとか）ので、型で防げる言語では防ぎたい気持ちがちょっと強めかもです。 |
|
ブランチ作成ありがとうございます！
インスタンス変数をinnerContextとしてまとめました！ |
sigprogramming
changed the base branch from
main
to
project-sequencer-statemachine
| | { | ||
| noteToAdd: Note; | ||
| previewRequestId: number; | ||
| executePreviewProcess: boolean; |
There was a problem hiding this comment.
should付けると更に意味がわかりやすいかも？
| executePreviewProcess: boolean; | |
| shouldExecutePreviewProcess: boolean; |
| private previewAdd(context: Context) { | ||
| if (this.innerContext == undefined) { | ||
| throw new Error("innerContext is undefined."); | ||
| } |
There was a problem hiding this comment.
（ただのコメントです）
undefinedではないinnerContextを引数に取る手もありそう！
|
あ、ブランチ分かれているので変更したい点あれば次のコミットに含めていただければ良さそう！ |
Hiroshiba
merged commit 521a8c0
into
VOICEVOX:project-sequencer-statemachine
内容
以下を行います。
getXInBorderBoxとgetYInBorderBoxとisSelfEventTargetをviewHelper.tsに移動関連 Issue
ScoreSequencer.vueのリファクタリング #2041その他
.eslintrc.jsを編集して、先頭に_がついている場合はエラーにならないようにしました