[project-library-manage] ライブラリ管理画面のデータの持ち方などをリファクタリング #1584
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
y-chan
commented
Sep 25, 2023
y-chan
commented
Sep 25, 2023
Comment on lines
339
to
352
| const convertedInstalledLibrary = Object.fromEntries( | ||
| Object.entries(engineInstalledLibraries).map( | ||
| ([uuid, library]) => { | ||
| return [ | ||
| LibraryId(uuid), | ||
| { | ||
| ...library, | ||
| uuid: LibraryId(uuid), | ||
| speakers: libraryInfoToCharacterInfos(engineId, library), | ||
| }, | ||
| ]; | ||
| } | ||
| ) | ||
| ); |
Hiroshiba
reviewed
Sep 25, 2023
Hiroshiba
reviewed
Sep 25, 2023
Hiroshiba
reviewed
Sep 26, 2023
Hiroshiba
reviewed
Sep 26, 2023
Hiroshiba
reviewed
Oct 1, 2023
There was a problem hiding this comment.
ややこさの根幹を理解しました！！！
エンジン側で、InstalledLibraryがDownloadableLibraryを継承しているからですね！！！
本来はどちらもBaseLibraryなどの型を継承しているべきで、どちらかがどちらかを継承しているのはおかしい気がしました。
そこになぞってエディターも実装されていて、色々実装していく上でややこしさがわかるようになったんだろうなと思いました。
2点だけ、リファクタリングで気づいたボタン挙動の違和感と、downloadableという単語を取っ払う提案をしてみました。
Comment on lines
+104
to
111
| :disable=" | ||
| (library.type !== 'notInstalled' && | ||
| !library.uninstallable) || | ||
| library.type === 'notInstalled' | ||
| " | ||
| @click.stop="uninstallLibrary(engineId, library)" | ||
| > | ||
| アンインストール |
There was a problem hiding this comment.
あー。インストールしていないのにアンインストールボタンが表示されるのはちょっと変な気がしました。
状態ごとにどのボタン表示するかが良いのかなと感じました。
と言っても変更は大きくなくて、こうするだけだと思います。
<template v-if="未インストール">
インストールボタン
<template v-else>
アプデボタン or 最新です
アンインストールボタン or disable
</template>
// TODO: カスタムインストールについて追記
Comment on lines
+188
to
+202
| type LibraryType = DownloadableLibrary & | ||
| ( | ||
| | { | ||
| type: "notInstalled"; // 未インストールのライブラリ | ||
| } | ||
| | { | ||
| type: "installed"; // インストール済みのライブラリ | ||
| isLatest: boolean; | ||
| uninstallable: boolean; | ||
| } | ||
| | { | ||
| type: "customInstalled"; // ユーザーが独自にインストールしたライブラリ | ||
| uninstallable: boolean; | ||
| } | ||
| ); |
There was a problem hiding this comment.
LibraryTypeがDownloadableLibrary（ダウンロード可能なライブラリ）を継承しているのがややこしさを増やしてるかもです。
実際DownloadableLibraryという型はこれ以降使われないのですが、何とかして消せないですかね。。
Comment on lines
239
to
241
| const downloadableLibraries = ref< | ||
| Record<EngineId, BrandedDownloadableLibrary[]> | ||
| Record<EngineId, Map<LibraryId, LibraryType>> | ||
| >({}); |
There was a problem hiding this comment.
この変数の名称が「downloadable」なのって、元の型がそうだから以外に理由とかってありそうでしょうか。
なければもう名前長いだけなので全部取っ払っていい気がしました！
| @@ -323,7 +320,7 @@ watch(modelValueComputed, async (newValue) => { | |||
| return; | |||
| } | |||
| fetchStatuses.value[engineId] = "fetching"; | |||
| const fetchResult = await store | |||
| const convertedDownloadableLibraries = await store | |||
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
内容
題のとおりです、 #1568 (review) でのコメントをもとにリファクタリングを行いました。
各リファクタを分けてもいいかなとも考えたのですが、それぞれのリファクタリングが密結合な感じだったので、一気にやってしまいました。
Brand(ed)というユビキタス言語を使わないようにし、逆にエンジン側からのデータに対してEngineとつけるようにしました。DownloadableLibraryとInstalledLibraryや、エンジンにはあるがDownloadableLibraryにはないライブラリ(カスタムライブラリ)などを考える際に関数を通すと言ったことを意識しなくていいよう、これらを結合したLibraryTypeを作り、管理画面内ではそれのみをいじるようにしました。(これに合わせて、installedLibraries変数が消えました。)downloadableLibraries([project-library-manage] インストール済みライブラリ(ローカルにのみ存在するライブラリ)についても音声ライブラリ管理画面に表示する #1568 ではallLibrariesにあたるもの)を重複禁止のためRecordで管理するようにしました。関連 Issue
ref #1568
その他
リファクタと新たな変更は分けたほうがいいと思うので分けました。