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

[project-library-manage] インストール済みライブラリ(ローカルにのみ存在するライブラリ)についても音声ライブラリ管理画面に表示する #1568

Open
wants to merge 2 commits into
base: project-library-manage
Choose a base branch
from

Conversation

y-chan
Copy link
Member

@y-chan y-chan commented Sep 19, 2023

内容

題のとおりです。
ユーザー製音声ライブラリなどの、公式から公開されていない音声ライブラリがインストールされていた場合に、表示できるようにします。

関連 Issue

VOICEVOX/voicevox_project#21

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

image

ここでは「ついなちゃんプロジェクトボイドラ一期生音声ライブラリ」と「らごぱすたん音声ライブラリ」がローカルにのみ存在するライブラリとして扱われています。

その他

スクリーンショットの事項を確認するためには SHAREVOX Engine 0.3.0-preview.4が必要です。

@y-chan y-chan requested a review from a team as a code owner September 19, 2023 22:56
@y-chan y-chan requested review from Hiroshiba and removed request for a team September 19, 2023 22:56
const installedLibraries = ref<Record<EngineId, BrandedInstalledLibrary[]>>({});

const isLatest = (engineId: EngineId, library: BrandedDownloadableLibrary) => {
const isLatest = (engineId: EngineId, library: BrandedLibrary) => {
Copy link
Member Author

@y-chan y-chan Sep 19, 2023

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.

なんかそういう種類のものを特別なライブラリだと分かるように定義してあげて、そのラベルを書いとくとかどうでしょう?
例えば「カスタムライブラリ」とか。

以下こまごまとした経緯です(なんかちょっと論点がずれてるかも 🙇 )


ローカルにのみ存在するライブラリ

タイトルにも含まれていますが、この呼び方はちょっと誤解がありそうだなと思いました。
おそらくインストール済みのライブラリのうち、downlaodableLibraries(エンジンのdownlaodableLibrariesで得られるライブラリ)ではないもののことですよね。
それらもエンジンを経由しなければ普通にダウンロードは可能だと思うので、別にローカルにのみ存在してるわけじゃないよな~と。

でも良い呼び方を思いつかないんですよね。。
「公式ライブラリ」と「ユーザーライブラリ」かなと思ったんですが、エンジンから案内されていないだけで、特別に限定配布した公式ライブラリみたいなのもありえそうなので。。

もし説明するなら「エンジンから案内されていないライブラリ」とかですかね。
もし名称をつける場合はChatGPT君に聞いた感じ、カスタムライブラリとかがいいかも。

Copy link
Member Author

Choose a reason for hiding this comment

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

まー色々考えてカスタムライブラリが適切なのかなーと...!
その代わりtooltipとかで説明の部分をしっかり案内してあげるのが良さげですかね...!

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.

現状のコードだと、説明されていないコンテキストがいろいろ含まれたコードになっていそうに感じました。
3ヶ月後にはもうわかんなくなりそうな気がします 😇

違和感がある点を列挙した後、リファクタリングの方針を提案してみました 🙇


1つ目がBrandで、これが「エンジン側ではないエディタ側の型」を意味してるのが珍しい気がします。(一般的だったらすみません)
この前提に気づけないとコードがだいぶ読みづらく感じちゃうので、エンジン側の方をプレフィックスEngineをつけてインポートして、エディタ側の方をEditorとつけるか、逆に無印にするかすれば読みやすくできるかもです。

2つ目がDownloadableInstalledの型の差で、どっちが何のプロパティを持っているのか把握していないと正しくコードが書けなくなっていそうに感じました。
↓にも関係しますが、そもそも引数の型がDownloadableLibrary | InstalledLibraryといろんなパターンを想定しないといけないコードは、どこか設計が変かもです。
例えばisUninstallable関数はInstalledLibrary以外来ないはず。

3つ目が、そもそもエディター側でライブラリは状態によって3種類あるけれども、それらが明示的に書かれてないかもです。
多分この3種類がありますよね。

  1. ダウンロードリストにあり、インストールされていないライブラリ
  2. ダウンロードリストにあり、インストールされているライブラリ
  3. ダウンロードリストにないけどインストールされているライブラリ

islatestは2にしか定義することができない、1にはisUninstallableを定義できないと思います。

4つ目がallLibrariesで、重複禁止として作られていますが、普通のリストなので勘違いするかもしれません。


これらを踏まえて一旦リファクタリングを入れた方が良さそうに感じました。
方針としてはこんなのとかどうでしょう。

  • エンジン由来のライブラリの型はプレフィックスにEngineをつける
  • Brandという言葉を除く
  • isLatestとisUninstallableを関数ではなくプロパティとして持たせる
  • エディタ側のLibraryはtypeプロパティを持たせて、3種類のライブラリのどれなのか、なんのプロパティを持てるのか型で縛る
    • {type: "downloadable"} & {type: "installed", isLatest: boolean, isUninstallable: boolean} & {type: "customInstalled", isUninstallable: boolean}みたいな感じ・・・?
  • allLibrariesはarrayではなくrecordにする

ただ提案しといてなんですが、ちょっと実際にリファクタリングしてみないと本当にこれでいいのか若干自信がないです。。

@y-chan
Copy link
Member Author

y-chan commented Sep 21, 2023

1つ目がBrand
「エンジン側ではないエディタ側の型」を意味してる

確かに、かなり変かもです。ここらへん特によく考えずにコードを引き継いじゃった節があります。
エンジン側のデータに対してEngineとつけるのがいいのかなと...!

2つ目がDownloadableInstalledの型の差
例えばisUninstallable関数はInstalledLibrary以外来ないはず。

最初はDownloadableLibraryに対してしかisUninstallableかどうかの判定を行っていなかったから仕方なかった節があるかもですが、ちゃんと考えるとおかしいですね...

3つ目
エディター側でライブラリは状態によって3種類ある

そうですね、これが2種類の型に圧縮されているのは設計としてよくなさそうです。

4つ目
allLibrariesで、重複禁止として作られていますが、普通のリスト

これは確かに改善したほうが良さそうです。

これらを踏まえて一旦リファクタリングを入れた方が良さそうに感じました。
ただ提案しといてなんですが、ちょっと実際にリファクタリングしてみないと本当にこれでいいのか若干自信がないです。。

私も考えてみましたが、そんな感じでいいんじゃないのかなーと思います...!

@y-chan y-chan changed the title インストール済みライブラリ(ローカルにのみ存在するライブラリ)についても音声ライブラリ管理画面に表示する [project-library-manage] インストール済みライブラリ(ローカルにのみ存在するライブラリ)についても音声ライブラリ管理画面に表示する Sep 24, 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