-
Notifications
You must be signed in to change notification settings - Fork 309
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
[project-library-manage] ライブラリのインストール・アンインストール処理を実装 #1497
base: project-library-manage
Are you sure you want to change the base?
[project-library-manage] ライブラリのインストール・アンインストール処理を実装 #1497
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ちょっと細かいところはまだ見られてないのですが、一旦設計レベルの話をちょこちょこ書いてみました 🙇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
結構時間かかってしまいましたが、レビューいただいた点を改善しました!
最初に比べて、かなりきれいな実装に持ってこれたんじゃないかなと思います...!
特にいじってない点や意見のある部分に関してはいじらずにそのままにして、コメントつけておきました(2箇所ほど)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
めちゃめちゃ良いコードになりましたね!!!
「バックグラウンド側でダイアログが出るからこっちではダイアログを出す必要がない」みたいなコメントもすごい優しいなと思いました。真似していきたいです。
もう1回ソースコード全部見直したいなと思っていますが、ちょっと時間がかかりそうだったので変更箇所周りだけコメントさせていただきました 🙇
Co-Authored-By: Hiroshiba <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ちょっと内部処理までは追えなさそうなのですが、主にUIに関して数か所コメントさせていただきました。
</div> | ||
<div class="q-px-md q-pt-md"> | ||
<span class="text-h5 text-bold"> | ||
各話者・キャラクターの利用規約をお読みください。 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"話者"という表現がエディタのユーザー向けに表示されるのは現状1箇所しかないので、単に「各キャラクター」の方が分かりやすい気がしますが、どうでしょう?
(もしstyleId毎を意図しているなら「各キャラクター・スタイル」ですかね)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nemo音声や他のエンジンのことも考えると確かに「話者」という表現の検討も必要かもと思いました!
でもとりあえずキャラクターで統一しておいて、後でどうするか決めるのが良さそう・・・・・・?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nemo音声や他のエンジンのことも考えると
私が意図していたのはここでした、他エンジンの場合必ずしもキャラクターではない場合があるかなと思ったので。
まあでもVOICEVOX UIに置いてはあらゆるところで「キャラクター」表記が使われているので、一旦統一してIssue化して議論するのがいいのかなと思いました。
P.S. e2eテストを書きたいなと思ったのですが、Nemoエンジンを使ってる関係上難しいのかなと思いました...どうするのがいいんですかね... |
これめっちゃ難しいですね。。。 あるいはVOICEOX OSS的には、キャラクターが何もインストールされていないプレーンなVOICEVOXエンジンをビルドできると最高にかっこいいかなと思いました。 |
内容
題のとおりです。
ライブラリインストール時は、ライブラリの利用規約を一度目に通すように経路を組みました。
ライブラリのダウンロードはフロントエンド側で行い、メモリ上のみに記憶しておくほうが楽そうに見えたのですが、ライブラリというものが基本大きいファイルであるため、一度一時ファイルとして保存するようにしました。
本当はcallbackを使ってインストール完了後にいい感じの処理ができればいいのですが、ipcしか使えない関係上少しごちゃっとしています。
関連 Issue
スクリーンショット・動画など
その他
動作確認に、音声ライブラリ管理機能を使えるようにしたSHAREVOXエンジンを使っていましたが、インストール機能周辺にバグがあったので
0.3.0-preview.2 0.3.0-preview.30.3.0-preview.4を作りました。以下からダウンロードして使えます。https://github.com/SHAREVOX/sharevox_engine/releases/tag/0.3.0-preview.2https://github.com/SHAREVOX/sharevox_engine/releases/tag/0.3.0-preview.3https://github.com/SHAREVOX/sharevox_engine/releases/tag/0.3.0-preview.4