Undo/Redoを実装したい

[Hiroshiba]

今はUndo/Redo機能がないので、行を間違ってdeleteすると、調整結果が消えてしまい、かなりしんどいです。
Undo/Redo機能はユーザーからも期待の声が上がっています。
https://twitter.com/idiotica/status/1427615544300032000

実はVOICEVOXはすでにcommand patternを実装しているので、Undo/Redoがあります。
https://github.com/Hiroshiba/voicevox/blob/630569e2f46da1956eef35278cc40ee8c3b24495/src/store/command.ts#L101-L107
vuexのstateの変更を一部記録しておき、自動的にundo操作を作り出しています。

ただ、stateの変更と、ユーザーが思っているundoがずれていることがよくあります。
たとえば「テキストの変更をundoしたい」場合、「テキストの変更」と「変更されたテキストから得られたイントネーションの変更」の２つのstate変更をundoしたいはずで、このずれを解消するような実装が必要です。

[Segu-g]

UNDO/RODOの実装方針についてです。
現在の実装はUNDO/REDOの為のpatchの生成をActionで行っており、これは異なるAction間の順序関係が保証されないのでpatchが壊れてしまいます。UNDO/REDOが正常に動作するためには、シンプルに実装したならば生成時の順序関係が保証されている必要があります。Vuexで生成を順序を保証するためにはActionではなくMutationで操作する必要があります。

Mutationにおいてpatchの生成を正しく行うための実装はいくつか思いつきますがどれが好ましいか意見を聞きたいです。

[Hiroshiba]

CommandをActionで管理すべきかMutationにすべきかを考えてもいいかなと思いました。

仰るとおり、コマンドを実装するには、CommandをQueueで管理する必要があると思います。
その方法は大まかに２つあって、１つがmutationのQueueを利用する方法、もう１つがCommand用のQueueを用意しておいてActionで叩くようなラッパーを用意する方法かなと思っています。

提案されているmutationで行う方法の場合、MutationからMutationを叩けないこと（裏技的に叩いた場合はImmerと馴染ませられるかどうか）が課題かなと感じています。
actionを採用する場合は、Queueとの結合をどう実装に落とし込むかが課題になるかなと感じています。

個人的にはAction使うルートのほうが、MutationからMutationを叩く裏技を使わなくていいのできれいかなと感じていますが、Queueを持つ必要があってそれはそれで面倒かなとも感じています。

[Segu-g]

MutationからMutationを叩けないというのはcommitできないという意味だと思いますがMuitationTree<S>はあくまでRecord<string, (state: S, payload?: any) => any>なので通常の関数を呼ぶようにthis.func(draft, payload)やxxStore.mutations.func(draft, payload)と関数を呼んでも何の問題もないと考えています。
今のようにas StoreOptions<State>でキャストすると型が消えてしまいプロパティの補完が効きませんが以下のようなラッパーを用いることで型を保持できます。

const wrapper = <Arg extends StoreOptions<State>>(arg: Arg): Arg => arg;
export const audioStore = wrapper({
  mutations: {
    [SET_ENGINE_READY](state, { isEngineReady }: { isEngineReady: boolean }) {
      state.isEngineReady = isEngineReady;
    },
    ...
  }
} as const);

---

追記(2021年8月23日)

voicevox上の環境で上記のようなコードを書くとtsconfig.jsonの設定で通る筈が、何故かstateがanyと解釈されるうえにその部分だけnoImplicitAnyが適応されトランスパイルが通りません。
簡潔にした以下のようなコードも同様にエラーが出ることを確認しています。

type ArgType = { a: number };
type SFunc<S> = (s: S) => void;

const wrapper = <Arg extends Record<string, SFunc<ArgType>>>(arg: Arg) => arg;
const x = wrapper({
  func: (s) => {
    console.log(s.a);
  },
} as const);

---

追記(2021年8月26日)
typescriptのバージョンの問題のようなので以上のコードを実行したい場合はtypescriptをアップデートしてください。

[Hiroshiba]

そうです、commitできないという意味でした。
直接叩く場合は、vuexにはmodule機能があって、これを使いたいとなったときに変な挙動しそうだなーとは思ってます。（が、this.を使う方針でうまく実装できるならこの問題を無視してそちらの方針で実装して良いと思いました。）

[Segu-g]

現在用いているStoreは全てglobalなStoreにスプレッド構文で展開しており浅いコピーが入っているので問題ありませんが、Moduleの様に直線参照を渡す必要のあるオブジェクトだと書き換えられないように外部に書いて一度コピーを挟む必要がありそうですね

[Hiroshiba]

VuexのModuleはたぶん使わないので、仮にこの方針で行くとしてもモジュールは一旦無いものとしちゃいましょう。

[Segu-g]

バグは多いですがひとまずサンプルとして動くものが出来上がったので、この方針でいいか意見を聞きたいです。（当該コミット）
方針としてはCommandを用いるStoreを本来のStoreと分け、Storeを格納した変数を直接参照することでthisを使わずにmutationを関数として呼んでいます。
また、UNDO/REDOをMutaitonで完結させるため非同期的な処理が必要な処理では、あらかじめActoin内でMutationの為に必要な非同期処理を完結させてからMutationを呼んでいます。これにより、AudioCellのTextの入力、アクセント句の編集、テキスト読み込み、新規テキストセルの追加においてもFETCHが走るため、現在これらの処理を頻繁に行うとエンジンとの通信が詰まりロールバックしたり通信に失敗したりします。

//commands
export const COMMAND_REGISTER_AUDIO_ITEM = "COMMAND_REGISTER_AUDIO_ITEM";
...

export const audioCommandStore = typeAsStoreOptions({
  actions: {
    [COMMAND_REGISTER_AUDIO_ITEM]: async (
      { state, commit, dispatch },
      {
        audioItem,
        prevAudioKey,
      }: { audioItem: AudioItem; prevAudioKey: string | undefined }
    ) => {
      const audioKey = await dispatch(ISSUE_AUDIO_KEY);
      const index =
        prevAudioKey !== undefined
          ? state.audioKeys.indexOf(prevAudioKey) + 1
          : state.audioKeys.length;
      commit(COMMAND_REGISTER_AUDIO_ITEM, { audioItem, audioKey, index });
      return audioKey;
    },
    ...
  },
  mutations: commandMutationsCreator({
    [COMMAND_REGISTER_AUDIO_ITEM]: (
      draft,
      payload: { audioItem: AudioItem; audioKey: string; index: number }
    ) => {
      audioStore.mutations[INSERT_AUDIO_ITEM](draft, payload);
    },
    ...
  } as const),
} as const);

PS
かなり既存のコードを壊しながら書いたので差分が醜いです。申し訳ありません。

[Hiroshiba]

actionで非同期処理をしておき、コマンド用mutationで各mutationを叩く設計、なるほどです。
この設計であれば、提案されている実装方針が良い感じだなーと思いました。

自分が気になったのはQueueの部分です。
ロールバックが起こり得ない設計にしておくと、気づきにくいバグを抑制できそうなので嬉しいかなと思っています。
この設計を見た今気づいたんですが、mutationが持つqueue機能をコマンドキューの代わりにすることはできないかも・・・？
（仰るとおり非同期処理が終わってからコマンド用mutationを実行する必要があり、非同期処理終了の順番は制御できないので）

となると https://github.com/Hiroshiba/voicevox/issues/116#issuecomment-903135384 で言っていたqueue周りを解決する２つの手法のうち片方はそもそも破綻していたことになりそうです。申し訳ないです。

ユーザーの行動から最終的なstateが一意に定まらないという課題はありますが、Queueを管理する手間が省かれていて実装面で利点があることもあり、いったん提案されている方針で良いかなと思いました。

[Segu-g]

私はこの問題はQueueの問題というよりは非同期な処理を必要とするコマンドを扱う時全般で起こり得る問題だと考えています。
例えばActionが発火されたコマンドに対し、前のコマンドの完了を待つようにするような処理を入れたとしても、多くの場合そのコマンドはActionを完了する前の不正な状態に依存した（前提とした）コマンドであり、前までのコマンドが完了した正常な状態に対して作用させても、目的の状態になるとは思えません。
恐らく、本当に必要なのはMutexのような非同期処理中にコマンドを発行できなくするようなロックではないかと考えます。

[Hiroshiba]

コマンド実行中はロックして、他のコマンドにはロック解除まで待ってもらう（無限ループで待機させる）感じでしょうか？

[Segu-g]

待機ではなく破棄です。UILockのようにコマンドの実行中は他のコマンドを実行出来なくする感じですね。（とは言ってもこの実装はバグった時に致命的になりそうなのと、そもそもそんな頻繁にコマンドが叩かれる状況が好ましくないと思いますが）
より現実に適した実装をするとしたら、各状態に対して更新する為のMutexを用意し、更新されていない値に関するコマンドを破棄する、といった感じでしょうか

[Hiroshiba]

なるほどです。仰ることはよくわかるのですが、実装コストと別開発者のコストがかなり上がりそうだなと思いました。

[Segu-g]

私も同じ意見です。実装自体は現在のシンプルな方針のまま、一時バッファを用いたコマンドの遅延評価などを用いてコマンドの発行頻度を下げる事を考えた方が合理的でしょう。

[Segu-g]

ImmerとVue3はどちらも変更の検知にProxyを使っているため相性が悪く、ImmerのpoduceWithPatchesでは上手くPatchを生成することができないようです。

[Hiroshiba]

なるほどです。
（技術的に興味があり、もしよければ具体的にうまく行かない例orコードを見てみたいです。）

[Segu-g]

現在のmainブランチでも発生しており、vuex-loggerを使って確認することができます。実際に発生している例としてはREMOVE_AUDIO_ITEMを実行した時のundo\redoのoperarionはKeyの削除とItemの削除の2つが格納されているはずですがどちらかしか格納されていない筈です（片方はDraftが働かずにproduceWithPatches内で直接書き換えられている？action内でstateを書き換えてるので致命的かも？）。
最小化したデモは環境構築が面倒でできていません...😥

[Hiroshiba]

なるほど・・・　実際にためてみたら、たしかに２つ目以降の操作が記録されていませんでした。
vue側のproxyをどうにかして外すか、immerをやめるかの２択でしょうか。
vueの実装に依存してしまうという負債を抱えますが、immerをやめるよりは全体の開発工数は少ないかなと思うので、（結構苦渋の決断ですが）この２択なら前者かなと思いました。。

[Segu-g]

JSON.parse(JSON.stringfy(state))を用いてstateのスナップショットを撮る事でProxyを外し、immerで正しくpatchesを記録する方法にしました。この方法はコマンド発行毎にstate全体をコピーするため、stateが大きくなった時にパフォーマンス上のボトルネックになり得ることを留意してください。プルリクエストのほうにも注意書きを書いておきます。

[Segu-g]

Vue3のProxyはどうやら外すためのtoRaw関数が提供されているようです！調査不足でした🙇‍♀️
https://v3.vuejs.org/api/basic-reactivity.html#toraw

置き換えてみたところ外見上は問題なく動作しているように感じました。
また、同様にボトルネックになるvuexのloggerプラグイン、strictモードを無効化して動作確認したところ、stateの大きさによって重くなっていた動作が改善していました。

[Hiroshiba]

！！！　なるほどです、期待です！

[Hiroshiba]

テキストを入力した後、AudioQueryなどをfetchしたあとにsetTextが走ることでテキスト欄が一瞬フラッシュする現象が気になっています。
たぶん多くのユーザーが体験する現象なので、リリース前になんとかしたい気持ちがあります。

IMEが入力を開始したことを検知し、確定するまでをpreview状態とできれば、setTextでstateに更新があったときにstateを無視することができるかなと思いました。
もしくは、fetchを実行した後に更にテキストを入力する人は少なそうなので、とりあえずdebouceを外せば軽減できるかなと思いました。

[Segu-g]

現在、候補として挙がっている方式を整理します。

• previewAbleValue
  コマンド発行: Enter
  Viewにプレビュー用の変数を持たせ、プレビュー中とプレビュー中ではないという状態を持つことで表示する状態を切り替える。プレビューを終えた後に非同期にmutationが走るとその間変更前の状態が見える。その状態で入力してしまうとその前の入力した文字が無駄になってしまう。

• Buffer + watch
  コマンド発行: Enter
  Viewに変数を持たせ、watchを使ってstateの状態を監視することで変更を感知する。変更が蓄積されているときはstateを無視する。コマンドが失敗した時にバグる。

• debounce
  コマンド発行: debounce
  Viewに変数を持たせ、watchを使ってstateの状態を監視することで変更を感知する。変更が蓄積されているときはstateを無視する。コマンドが失敗した時にバグる。

フラッシュはdebounceがstateに変更を検知した際に、蓄積がないと判定されている(IMEの入力は@updateに通知されない)ため更新が起こってしまい、一瞬再描画が起こっているのだと思います。
Buffer + watch方式でbufferとstateの比較をして、同じ場合は更新しないようにし再描画を抑えられるか試してみようと思います。

[Hiroshiba]

Buffer + watch方式でbufferとstateの比較をして、同じ場合は更新しないようにし再描画を抑えられるか試してみようと思います。

なるほど、すごく良いと思います！！！