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

複数選択:話者変更するとAudioQueryのテキストがAudioCellのテキストと一致しなくなるバグを修正 #1665

Merged

Conversation

sevenc-nanashi
Copy link
Member

内容

タイトル通りです。

関連 Issue

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

(なし)

その他

(なし)

@sevenc-nanashi sevenc-nanashi requested a review from a team as a code owner December 2, 2023 13:34
@sevenc-nanashi sevenc-nanashi requested review from y-chan and removed request for a team December 2, 2023 13:34
@sevenc-nanashi sevenc-nanashi changed the title 複数選択:話者変更するとAudioQueryのテキストと一致しなくなるバグを修正 複数選択:話者変更するとAudioQueryのテキストがAudioCellのテキストと一致しなくなるバグを修正 Dec 3, 2023
@sevenc-nanashi
Copy link
Member Author

ふと気になって試してました、これUndoが一括でできなくなってますね。Draft行き。

@sevenc-nanashi sevenc-nanashi marked this pull request as draft December 3, 2023 15:19
@sevenc-nanashi
Copy link
Member Author

なんとかしました。
かなり構造が変わってしまった…

@sevenc-nanashi sevenc-nanashi marked this pull request as ready for review December 3, 2023 15:57
@Hiroshiba Hiroshiba requested review from Hiroshiba and removed request for y-chan December 4, 2023 17:40
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.

プルリクエスト見させていただきました!
バグに気づいてくださってありがとうございます、いつも感謝してます!!

処理の流れですが、エラーが起こった場合にユーザーが気づけない挙動になっているのでそこだけ修正をお願いできると・・・!
大量のリクエストが一度に走ることがありえそうなので、一つ一つ処理してもいいのかなとちょっと思いました。であればエラーハンドリングが楽だなーと。

@sevenc-nanashi
Copy link
Member Author

処理の流れですが、エラーが起こった場合にユーザーが気づけない挙動になっているのでそこだけ修正をお願いできると・・・!

エラーハンドリングで悩んでます、例えば複数回エラーが起きたらどんなエラーを投げるべきかとか

@Hiroshiba
Copy link
Member

Hiroshiba commented Dec 4, 2023

エラーハンドリングで悩んでます、例えば複数回エラーが起きたらどんなエラーを投げるべきかとか

とりあえず失敗したaudioKeyをエラー文に組み込んでthrow、とかで良いかなと!!(変更前もそうですし)
TODOかFIXMEで、任意数対象のエラーハンドリングが必要とか書きつつ。

あとはまあ…失敗したのはAudioQueryをundefinedにする、とか…?(ちゃんと考えれてないです)

@sevenc-nanashi
Copy link
Member Author

良い感じになったと思います。

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.

すごい読みやすいコードでした!!!

すみません! 追加で気づいたのですが、プリセット適用周りの処理が抜けているかもです 🙇

@sevenc-nanashi
Copy link
Member Author

プリセット周りを足しました。

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.

1点だけ問題がありそうだったのでコメントしました!
テスト書き出してもいいかも・・・?

Comment on lines 2046 to 2051
if (shouldApplyPreset) {
audioStore.mutations.SET_AUDIO_PRESET_KEY(state, {
audioKey,
presetKey: nextPresetKey,
});
}
Copy link
Member

Choose a reason for hiding this comment

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

セットする時はshouldApplyPresetのチェックいらないかもです!
MULTI化する前のCOMMAND_CHANGE_VOICEのコードに合わせればいいはずかなと
https://github.com/VOICEVOX/voicevox/pull/1228/files#diff-b86b438c7928773aac473007988ffb602afc8278b8e2226ce784442befb46b0dR2096

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.

LGTM!!!

結構この辺りバグになりやすい気がするので、もしよかったらテストをお願いできると心強いかもしれません・・・ 🙇
あるいはもう複数選択を一気に完成させるのもありかもです!!
(あと何が残ってるかちょっと覚えてないですが)

@Hiroshiba Hiroshiba merged commit 3d89f4f into VOICEVOX:main Dec 14, 2023
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.

2 participants