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.

Sign up for GitHub

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

Jump to bottom

複数エンジン対応：複数の辞書に対する操作を追加 #980

Merged
merged 18 commits into from
Oct 21, 2022
Merged

複数エンジン対応：複数の辞書に対する操作を追加 #980

merged 18 commits into from
Oct 21, 2022

Conversation

sevenc-nanashi
Copy link
Member

@sevenc-nanashi sevenc-nanashi commented Oct 14, 2022 •
edited
Loading

内容

複数の辞書に対する動作を追加します。

関連 Issue

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

image

その他

（なし）

@sevenc-nanashi sevenc-nanashi marked this pull request as draft October 14, 2022 12:27
@sevenc-nanashi sevenc-nanashi marked this pull request as ready for review October 14, 2022 13:21
@sevenc-nanashi
Copy link
Member Author

思ったより早く実装できました。Draft外します

@sevenc-nanashi
Copy link
Member Author

プレビューを複数のエンジンでやりたいときがありそうなので実装します。

@sevenc-nanashi sevenc-nanashi marked this pull request as draft October 15, 2022 11:07
@sevenc-nanashi sevenc-nanashi marked this pull request as ready for review October 15, 2022 11:52
Hiroshiba
Hiroshiba reviewed Oct 15, 2022
View reviewed changes
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.

プレビュー機能に関して、たしかにできることが増えるのですが、UIの複雑度が上がる一方で利用者はかなり少ないのかなと感じました。

目的としてはたぶん、馴染みのある声でアクセントを修正したいんじゃないかなと思いました。
となると、キャラクター並び替えのトップ１のキャラとそのエンジンで生成するとすると、全ユーザーにとってわかりやすいかもです！

Hiroshiba
Hiroshiba reviewed Oct 15, 2022
View reviewed changes
src/store/dictionary.ts Outdated Show resolved Hide resolved
Hiroshiba
Hiroshiba reviewed Oct 15, 2022
View reviewed changes
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です！！
コメントしたところに関して変えて頂ければ･･･！

辞書周りに詳しい @y-chan さんもレビューいただけると心強いです！

@sevenc-nanashi sevenc-nanashi force-pushed the add/dict-operation-for-multiple-engines branch from 968eff9 to 678c08e Compare October 15, 2022 15:03
Hiroshiba
Hiroshiba reviewed Oct 16, 2022
View reviewed changes
src/components/DictionaryManageDialog.vue Outdated
Comment on lines 365 to 366
if (store.state.engineIds.length === 0) return "";
if (!store.getters.USER_ORDERED_CHARACTER_INFOS) return "";
Copy link
Member

Choose a reason for hiding this comment

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

ちょっとまだドキュメントにかけていないのですが、予想外のものが入ってきたときは基本的にthrowでお願いします！
（エラーを追うのがとても難しくなってしまうので）

if文のあとに「throw」と書くとあとはいい感じのメッセージをcopilot君が出してくれると思います。

Copy link
Member Author

Choose a reason for hiding this comment

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

throwするようにしました。」

Hiroshiba
Hiroshiba approved these changes Oct 16, 2022
View reviewed changes
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 reviewed Oct 16, 2022
View reviewed changes
src/store/dictionary.ts Outdated
Comment on lines 66 to 67
// 同期処理により、一つのエンジンだけに登録しても、他のエンジンにも反映される。
// むしろ全てのエンジンに登録処理をするとUUIDが合わない。
Copy link
Member

Choose a reason for hiding this comment

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

このコメントは、ADD_WARDをしたあとにSYNCすると、という意味ですよね。
ADD_WARDはただ単語を追加する処理でSYNCとは独立しているため、SYNCを実行した場合のコメントをここに書くと、たぶんSYNCのことを知らない他の開発者の方が混乱すると思います。

このコメントを書くなら、辞書UIでADD_WARDを実行しているところのが適切かもです。

Copy link
Member Author

Choose a reason for hiding this comment

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

SYNCをADD_WORDにも書く、でも解決できそうな感じがしました。

Hiroshiba
Hiroshiba requested changes Oct 16, 2022
View reviewed changes
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.

すみません、１点コードが煩雑になっている箇所を見つけたのでコメントします！

今は起動時にLOAD(マージ)+SYNC、追加時にADD+SYNC、消去と編集は全エンジンに同時実行となっていると思います。
追加も全エンジンに同時実行する形にするのはどうでしょう。

そうすればSYNCを２箇所呼ぶ必要がなくなるためLOAD(マージ)+SYNCを１つのLOAD関数にまとめられるし、消去・編集と追加の処理が微妙に違う（あとでSYNCを呼ぶ有無が異なる）不揃いさを解消できるのかなと思いました。

あと、SYNCは「マージで消えたIDの単語を消す」処理をしていますが、この形だとマージしたあとに同じ関数内で消えたIDの単語を消すので、よりわかりやすくなるのかなと思いました！

Hiroshiba
Hiroshiba approved these changes Oct 17, 2022
View reviewed changes
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！！

「同じ単語は違うエンジンでも同じIDで登録されているべき」という仕様を想定したコードな場合、たしかにADDしたあとSYNCするのが合っていると感じました。
実際はまあ別に違うエンジンでは違うIDでも良いので、複数エンジンにADDでもどちらでも良さそうだという印象です！

このPRに関係ない設計レベルの話ですが、SYNC関数はエンジン側にあっても良いのかなと感じました。
サードパーティアプリからも需要があるかもだし、そっちのほうが何度もリクエストしないので処理も早そうだなと。
issue建ててみます！

src/store/dictionary.ts Outdated Show resolved Hide resolved
@Hiroshiba Hiroshiba requested a review from y-chan October 17, 2022 14:42
@sevenc-nanashi @Hiroshiba
Update src/store/dictionary.ts
296c9b3 
Co-authored-by: Hiroshiba <hihokaruta@gmail.com>
This was referenced Oct 17, 2022
Closed
Open
y-chan
y-chan approved these changes Oct 21, 2022
View reviewed changes
Copy link
Member

@y-chan y-chan 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
Copy link
Member

マージします！

@Hiroshiba Hiroshiba merged commit a01bd2b into VOICEVOX:main Oct 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@y-chan y-chan y-chan approved these changes

@Hiroshiba Hiroshiba Hiroshiba approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sevenc-nanashi @Hiroshiba @y-chan