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: デフォルトエンジンかつVVPPが設定できるよう型と実装を変更 #2242

Conversation

Hiroshiba
Copy link
Member

内容

の下準備です。

今の形だと、デフォルトエンジンだった場合type: "vvpp"を指定できませんでした。(typeの型が "default" | "vvpp" | "path"
このままだとデフォルトエンジンかつVVPPが指定できず、色々共通化とかで不便な気がしたので、isDefaultプロパティにしてみました。

関連 Issue

その他

とりあえず既存エンジンもVVPP経由で追加できることや、デフォルトエンジンできないことは確認しました。

将来、普通のVVPP追加エンジンと、VVPPでインストールされたデフォルトエンジンが混じりそうですが、これはデフォルトエンジンがインストールされるVVPPのディレクトリと、普通に追加されたVVPPディレクトリを分けることで解決できる気がしてます。

@Hiroshiba Hiroshiba requested a review from a team as a code owner August 21, 2024 18:09
@Hiroshiba
Copy link
Member Author

Hiroshiba commented Aug 21, 2024

正直、設計がこれでいいのか若干自信がないです。
{ type: "vvpp" | "path", isDefault: boolean }にせず、
{ type: "default" | "defaultVvpp" | "vvpp" | "path" }にする手もあるので・・・。

もっと先まで作ってからプルリクエストにする手もあるかも。
例えば実際にデフォルトエンジンとしてVVPPを指定&実行できるようにするとか。
だいぶ変更量多くなっちゃうけど・・・。

@sevenc-nanashi もしよかったら見ていただけると 🙇

Comment on lines +41 to +51

## マルチエンジン

エンジンの追加は VVPP ファイルをインストールする形と、エンジンディレクトリのパスを指定する形があります。

| | VVPP | パス |
| -------------------- | ------------------------------------ | ----------------------------------- |
| `EngineInfo`の`type` | `"vvpp"` | `"path"` |
| 追加時の処理 | zipファイルを所定のフォルダに展開 | エンジンのパスを`config.json`に保存 |
| 読み込み時の処理 | 所定のフォルダ内にあるものを読む | `config.json`に保存されたパスを読む |
| 削除時の処理 | 所定のフォルダ内のディレクトリを削除 | `config.json`からパスを削除 |
Copy link
Member Author

Choose a reason for hiding this comment

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

vvppとpathで保存形式が違っていてちょっとややこしかったのでまとめてみました。

Comment on lines -18 to +24
const engineInfos = await window.backend.engineInfos();
let engineInfos = await window.backend.engineInfos();

// マルチエンジンオフモード時はengineIdsをデフォルトエンジンのIDだけにする。
let engineIds: EngineId[];
// マルチエンジンオフモード時はデフォルトエンジンだけにする。
if (state.isMultiEngineOffMode) {
engineIds = engineInfos
.filter((engineInfo) => engineInfo.type === "default")
.map((info) => info.uuid);
} else {
engineIds = engineInfos.map((engineInfo) => engineInfo.uuid);
engineInfos = engineInfos.filter((engineInfo) => engineInfo.isDefault);
}
const engineIds = engineInfos.map((engineInfo) => engineInfo.uuid);
Copy link
Member Author

@Hiroshiba Hiroshiba Aug 21, 2024

Choose a reason for hiding this comment

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

マルチエンジンオフモードの時はデフォルトエンジンだけにするのですが、
engineIdsはフィルタしてengineInfosはフィルタしない設計だったことに気づいたので、ついでにどっちもフィルタするようにしてみました。

@sevenc-nanashi
Copy link
Member

{ type: "vvpp" | "path", isDefault: boolean }

大丈夫だと思います。気をつけるべきとしてはデフォルトエンジンを消し飛ばす(エンジン管理画面からとか)ことができないことを確かめる、とかですかね?

@Hiroshiba
Copy link
Member Author

Hiroshiba commented Aug 22, 2024

レビューありがとうございます!!! 心強いです 🙇

大丈夫だと思います。気をつけるべきとしてはデフォルトエンジンを消し飛ばす(エンジン管理画面からとか)ことができないことを確かめる、とかですかね?

ですね! 一応このPRではデフォルトのは消せないのを確かめ済みです。

あとは・・・デフォルトエンジンと同じIDを持つvvppをインストールできてしまう、とかがあり得るかも。
あれ、もしかしたらこれまだ塞いでないかもですね。。
とりあえずチェック項目に追加しておきます 🙇

@Hiroshiba Hiroshiba merged commit e49a2fc into VOICEVOX:main Aug 22, 2024
9 checks passed
@Hiroshiba Hiroshiba deleted the vvppをデフォルトエンジンニ指定可能にする branch August 22, 2024 04:59
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