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

refactor: テキストファイルの受け渡しの処理のリファクタリング #2298

Merged
merged 5 commits into from
Oct 19, 2024

Conversation

sabonerune
Copy link
Contributor

内容

#2282 (review) で提案されたリファクタリングです。
それぞれ別の関数から取得していたリソースを一つの関数から取得するようにします。

@sabonerune sabonerune requested a review from a team as a code owner October 14, 2024 05:27
@sabonerune sabonerune requested review from Hiroshiba and removed request for a team October 14, 2024 05:27
src/store/index.ts Outdated Show resolved Hide resolved
src/type/preload.ts Outdated Show resolved Hide resolved
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.

めちゃめちゃ良い感じだと思いました!!!!!!!!!
ありがとうございます!!!!!


(これはただのコメントですが)

themesも同じテキストファイルの枠組みに押し込められないかとちょっと感じました。
現状のコードだと可能ではありそう。ライトテーマ・ダークテーマ分ける形で。
けどまあ、カスタムテーマとかの可能性もあって他のファイルとは若干異質だし、別にまとめなくても良いかも。

まあでもまとめちゃっても良さそうではある!というメモまで!

Copy link
Contributor Author

@sabonerune sabonerune left a comment

Choose a reason for hiding this comment

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

一通り修正が終わりました。

src/backend/electron/preload.ts Outdated Show resolved Hide resolved
src/backend/electron/main.ts Outdated Show resolved Hide resolved
src/store/index.ts Outdated Show resolved Hide resolved
src/backend/electron/main.ts Outdated Show resolved Hide resolved
@sabonerune sabonerune requested a review from Hiroshiba October 15, 2024 09:42
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!!!!!

色々と調整いただきありがとうございました!!!

すみませんまたリファクタリングお願いしても良いでしょうか 🙇
同じくmain.tsの解体を手伝っていただきたいな~~~と思っており。。。


main.ts周りの設計ですが、各クラスのインスタンスを取得できる関数をそれぞれのファイル内に作り、お互いのインスタンスから結構自由に使えるようにする、というのが良さそうに感じました!
今もインスタンスは作っていますが、main.tsに集まっているので、各々のマネージャークラスに渡さないといけなくて大変なんですよね。
各マネージャーが各マネージャーを自由に使えるのがいいのかなと。

もちろんやたらめったお互い利用しまくると大変なので、ある程度どのマネージャーがどのマネージャーに依存するかの向きを揃えておきたいとは思います。
ただ強制しないというだけ。これで成長するソフトウェアの設計に柔軟に対処できるようにしつつ、ある程度の秩序と見やすさを確保できるかなぁと。

雰囲気としてはフロント側の状態管理と似てると思います。各々関心ごとのStoreがあり、各Storeはお互いのStoreを参照できる形。
そもそもVOICEVOXが成長していて、今の僕たちでは設計を想像できないから、柔軟性を保ちつつ分解するのが良いのかなぁと!

Discordの方でもう少し詳しく書いているので、もしよかったら!
https://discord.com/channels/879570910208733277/893889888208977960/1295453161477242972

ここからが本題なのですが、このためにはそれぞれのクラスファイル内でインスタンスをgetできる必要があります。
この作業お願いできないかなと・・・!
やることは意外とシンプルで、

  • RuntimeInfoManager.ts
  • engineInfoManager.ts
  • engineProcessManager.ts
  • vvppManager.ts
  • engineAndVvppController.ts

それぞれにgetなんとかかんとかというglobal関数と、必要であれば(引数が必要なら必要)initialiなんとかかんとか関数を用意するとかどうでしょうか!!

例えばEngineInfoManagerはmain.tsではこう↓なので、

const engineInfoManager = new EngineInfoManager({
  configManager,
  defaultEngineDir: appDirPath,
  vvppEngineDir,
});

EngineInfoManager.ts側でこういう実装を書いて

let engineInfoManager: EngineInfoManager| undefined;

export function initializeEngineInfoManager(defaultEngineDir: string, vvppEngineDir: string) {
  engineInfoManager = new EngineInfoManager({
    // configManager, ← getConfigManager()すれば良くなるので不要なはず
    defaultEngineDir,
    vvppEngineDir,
  })
}

export function getEngineInfoManager() {
  // engineInfoManagerがundefinedならthrow、値があるなら返す
}

みたいな!

例えばEngineAndVvppControllerの場合、main.ts内でこうありますが

const engineAndVvppController = new EngineAndVvppController(
  runtimeInfoManager,
  configManager,
  engineInfoManager,
  engineProcessManager,
  vvppManager,
);

渡してるのは全部マネージャーなので、おそらくinitializeは不要で、こう書けるはず!

let engineAndVvppController: EngineAndVvppController| undefined;

export function getEngineAndVvppController() {
  // engineAndVvppControllerがundefinedなら値を作る、違うなら返す
}

ちょうどelectronConfigがこの実装のはず。

let configManager: ElectronConfigManager | undefined;
export function getConfigManager(): ElectronConfigManager {
if (!configManager) {
configManager = new ElectronConfigManager();
}
return configManager;
}

避けたいのはgetなんとかがasyncになっちゃうことです。awaitまみれになっちゃうので。。

ちょっとわかってないのが、マネージャークラスによってはメソッドごとに大量にget~~を呼ぶ必要が出てきて、なんかコードが煩雑になっちゃう可能性もありえそうなんですよねー。。。。
実装中にその気配を感じたら一旦相談いただけると嬉しいかもです!
(一番いいのは1回実装してみることですが、手戻りになってしまうので申し訳ないなと。。。)

長くなっちゃってすみません!!!
もし、もしよければまたお力お借りしたく・・・・・!!!!!

@Hiroshiba Hiroshiba merged commit 5f2b79f into VOICEVOX:main Oct 19, 2024
8 checks passed
@sabonerune sabonerune deleted the refactor/asset-text branch October 22, 2024 10:25
@sabonerune
Copy link
Contributor Author

とりあえず挑戦してみます。

@Hiroshiba
Copy link
Member

ぜひぜひ!! 不明な点とかあったら気軽に聞いてください!!

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.

3 participants