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

add AccentPhraseに個別idを設定 #1879

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

Conversation

P0ngCh4ng
Copy link
Contributor

AccentPhraseを継承したEditorAccentPhraseという型を作成し、EditorAudioQueryのプロパティとした。

AudioDetail.vueファイルの中でaccentPhrases変数の変更のタイミングで割り振るように

内容

  SET_ACCENT_PHRASES_EDITORID: {
    mutation(
      state,
      {
        audioKey,
      }: {
        audioKey: AudioKey;
      }
    ) {
      const accentPhrases = state.audioItems[audioKey].query?.accentPhrases;
      if (accentPhrases)
        accentPhrases.map((elem) => {
          if (!elem.editorID)
            elem.editorID = uuidv4();
        });
    },
    action({ commit }, { audioKey }: { audioKey: AudioKey }) {
      commit("SET_ACCENT_PHRASES_EDITORID", { audioKey });
    },
  },

このようなactionとmutationを作成し、AudioDetail.vueの中で呼ぶように。

関連 Issue

#1591

その他

動作的には問題なさそうですが、作成したSET_ACCENT_PHRASES_EDITORIDを呼び出す場所はここで良いでしょうか?
src/openapi/models/AccentPhrase.tsファイル内と迷いましたが、まずは実装が簡単そうだった方から選びました。

2024-02-27.15.17.16.mov

AccentPhraseを継承したEditorAccentPhraseという型を作成し、EditorAudioQueryのプロパティとした。

AudioDetail.vueファイルの中でaccentPhrases変数の変更のタイミングで割り振るように
@P0ngCh4ng P0ngCh4ng requested a review from a team as a code owner February 27, 2024 06:22
@P0ngCh4ng P0ngCh4ng requested review from Hiroshiba and removed request for a team February 27, 2024 06:22
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.

プルリクエストありがとうございます!!
動作は完璧だと思います、素晴らしいです!!

動作に関しては完璧だったのですが、ちょっと将来的に課題になりそうな点に思い当たったので相談コメントを書きました 🙇

Comment on lines 132 to 134
export interface EditorAccentPhrase extends AccentPhrase {
editorID?: string;
}
Copy link
Member

Choose a reason for hiding this comment

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

すみません、ちょっとご相談です!
editorID?をオプショナルじゃなくするか、あるいはAudioDetail.vue内でだけeditorIDが現れる形にするか、どちらかに倒す必要があるかもです 🙇

以下詳細です:

editorID?とオプショナルにするの、なるほどです!!
ただこうすると、プロジェクトファイルを保存した時にこの ID が保存されたりされなかったりしちゃうかもです。

実際にオプショナルをやめる設計を考えていたのですが、実際試してみると膨大な量の変更が必要になりました 😇
実際に実装してみたのが↓です。まだmerge/splitしたときにアニメーションしてしまう問題が残ってます。
Hiroshiba#2

あるいは、どうにかしてAudioDetail.vue内でだけアクセント句ごとにeditorIDを持たせるのも良さそうです。
ただその方法ずっと考えてみてたんですが、思いつきませんでした・・・・・・・・。もし思いついたらぜひ知りたいです。。

ということで、2通りの方法どちらかできそうだったりしますか・・・?
前者は大変ですが、僕が書いたとこから続きをやっていただければ・・・!
後者の方は方法が思いついたらぜひお願いしたいです 🙇

長くなってしまってすみません、何かあれば気軽に聞いていただければ!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ご提案ありがとうございます。
こちら了解しました!

後者は私の方でも検討してみますが、難しそうであればこのまま前者を実装してみます!

Copy link
Member

Choose a reason for hiding this comment

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

ありがとうございます!!
入り組み始めるかもなので、不明な点などあればお気軽にお聞きください!!

@P0ngCh4ng
Copy link
Contributor Author

これマージコミットって発生しても大丈夫ですか?
何も考えずにgit pull --rebaseしてしまった...

@Hiroshiba
Copy link
Member

@P0ngCh4ng マージコミット大丈夫です!

ちなみにrebaseはgithubコメントとコミットの順番が変わるのでどちらかというとマージのが嬉しい感じです!(細かいのでどこにもこの案内書いてません🙇)
今回はまだ序盤なのでどっちでも大丈夫だったかなと!

@P0ngCh4ng
Copy link
Contributor Author

先日いただいた修正をもとに、とりあえず同じようにaudioDetails.tsの中でactionを走らせてみました。
動作はしているように見えますが、課題点の解決には至っていますかね?

@P0ngCh4ng
Copy link
Contributor Author

@Hiroshiba
こちらお手すきの際にご確認していただけると幸いです 🙏

@Hiroshiba
Copy link
Member

@P0ngCh4ng おまたせしてしまっていて申し訳ありません 🙇 🙇 🙇
もう少しだけお待ちいただけると・・・・ 🙇 🙇 🙇

@Hiroshiba
Copy link
Member

Hiroshiba commented Mar 18, 2024

すみませんお待たせしました!

先日いただいた修正をもとに、とりあえず同じようにaudioDetails.tsの中でactionを走らせてみました。 動作はしているように見えますが、課題点の解決には至っていますかね?

課題点は解決していますが、実はちょっとだけ危ない点があるかもと思いました!

vueの中でUI操作以外(Vuex.stateのwatch)からVuexの状態を変更してる点です。
ここだけだったら把握しやすいかもですが、至るところでこのような操作がされてしまうと、どこで誰がどのデータをどう変更しているのかが複雑に入り組んでしまうかなと!
(ちなみにそういう複雑化を解消するために「データの流れを一方向にして分かりやすくしよう」とした設計思想がVuexが参考にしてるFlux思想・・・なはず!)

実際、テキスト欄を削除するとエラーが出るのを発見しました!
もちろん例外処理をすれば大丈夫ですが、いくつも重なるとかなり大変になっていくかも・・・?


そもそもmerge/splitするときにidを変更してしまっても問題なさそうなことに気づきました。
Hiroshiba#2 (comment)

merge/splitは多分この辺なのですが、ここでidを変更するように処理の書き換えをお願いする事ってできたりしますか・・・? 🙇
https://github.com/P0ngCh4ng/voicevox/blob/d058e47dab9e4da2acda7c01aca7e8ef1d4a6338/src/store/audio.ts#L2224-L2271

何度も何度もすみません。。わからないとこがあれば何でも聞いてください!! 🙇

@P0ngCh4ng
Copy link
Contributor Author

了解しました
1から10まですみません 🙏
書き換えやってみます!

@P0ngCh4ng
Copy link
Contributor Author

@Hiroshiba
完了したので確認いただいてもよろしいでしょうか?

Copy link
Member

@Segu-g Segu-g left a comment

Choose a reason for hiding this comment

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

方針は良いと思います!

修正が必要な点として、過去のバージョンで作られたプロジェクトファイルを読み込む際にkeyを追加するマイグレーションが書かれていません。project.tsにマイグレーションのコードがあるので追加をお願いします🙏

src/store/audio.ts Outdated Show resolved Hide resolved
src/store/audio.ts Show resolved Hide resolved
src/store/utility.ts Outdated Show resolved Hide resolved
src/store/type.ts Outdated Show resolved Hide resolved
src/store/proxy.ts Outdated Show resolved Hide resolved
@P0ngCh4ng
Copy link
Contributor Author

レビューありがとうございます!
ご指摘のところ修正します。

Copy link
Member

@Segu-g Segu-g left a comment

Choose a reason for hiding this comment

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

修正ありがとうございます!
project.ts の修正についてなのですが、このPR以前のバージョンで作られたプロジェクトファイルではkeyプロパティが含まれていないので、それを追加する処理を書いてください。
具体的にはproject.tsの他のマイグレーション処理の後に、他のマイグレーションと同じようにsemverでバージョンを判別しマイグレーションを実行するコードを書いてください。

if (semver.satisfies(projectAppVersion, "<0.17.0", semverSatisfiesOptions)) {
	... // アクセント句にkeyプロパティを追加する処理を書く
}

@@ -298,7 +300,7 @@ export const projectStore = createPartialStore<ProjectStoreTypes>({
engineId,
styleId: audioItem.characterIndex,
})
.then((accentPhrases: AccentPhrase[]) => {
Copy link
Member

Choose a reason for hiding this comment

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

申し訳ございません、説明不足でした!
このコードはvoicevoxの 0.5より前のバージョンで作られたプロジェクトファイルを編集している箇所なので、keyが含まれていないAccentPhraseのままで大丈夫だと思います!

@P0ngCh4ng
Copy link
Contributor Author

ご指摘ありがとうございます。
現在の最新バージョンに合わせればいいでしょうか?

@Segu-g
Copy link
Member

Segu-g commented Apr 14, 2024

マイグレーションのコードは リリースするバージョン未満 で実行するべきなので、とりあえず <0.19.0 の条件で書いておけばいいと思います。
また、コンフリクトが発生しているようなのでその解消もお願いします。

@Hiroshiba マイグレーションのバージョン判定を埋め込んでいるのでメンションしておきます。

@P0ngCh4ng
Copy link
Contributor Author

コンフリクトの解消の方法については、

  1. mainをPRブランチに取り込み
  2. ローカルでコンフリクトを解消
  3. マージコミットを作成

こちらの方法で問題ないでしょうか?

日にちだいぶ空いてしまいましたが、GW中は時間があるので、作業再開します 🙇

@Segu-g
Copy link
Member

Segu-g commented May 2, 2024

@P0ngCh4ng
はい!その方法で大丈夫です!

また、先日0.19.0がリリースされたため、バージョン判定は'<0.20.0'でよろしくお願いします!

@P0ngCh4ng P0ngCh4ng requested a review from a team as a code owner May 5, 2024 08:40
@P0ngCh4ng P0ngCh4ng requested review from Hiroshiba and removed request for a team May 5, 2024 08:40
@P0ngCh4ng
Copy link
Contributor Author

@Segu-g
ご返信ありがとうございます!
コンフリクトの修正と、マイグレーションコードの作成を行いましたが、
・マージ直後にもかかわらず、linterとfomtterを起動させると大量の変更が発生する
src/store/project.tsの93行目に発生したエラーの解決ができない

これらの問題が残っています。レビューしていただいてもよろしいでしょうか?
よろしくお願いします。

@sevenc-nanashi
Copy link
Member

・マージ直後にもかかわらず、linterとfomtterを起動させると大量の変更が発生する

#1992 かも?多分フォーマットかければ治ると思います

@P0ngCh4ng
Copy link
Contributor Author

npm ciをかけなおしたところ、想定通りの動作になりました。

Copy link
Member

@Segu-g Segu-g left a comment

Choose a reason for hiding this comment

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

src/store/project.tsの93行目に発生したエラーの解決ができない

すみません、エラーの件見逃してました!
ここのエラーはプロジェクトファイルの型定義である LatestProjectType の更新のし忘れによって起こっているエラーです。

src\domain\project\schema.ts にある accentPhraseSchemakey: accentPhraseKeySchema を追加すれば良いと思います!

src/store/project.ts Outdated Show resolved Hide resolved
src/store/project.ts Outdated Show resolved Hide resolved
accentPhraseKeySchema追加し忘れ
EditorAccentPhraseからAccentPhraseへ戻し忘れ
@P0ngCh4ng
Copy link
Contributor Author

エラーの修正完了しました!
お手隙の際レビューお願いします🙇

@Segu-g
Copy link
Member

Segu-g commented May 11, 2024

すみません!レビューが遅くなってしまいました🙇‍♂️

PRの修正内容は問題ないと思います!LGTMです!
申し訳ないのですが、またConflictが発生してしまったので修正してもらえると!!🙇‍♂️

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.

4 participants