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: managerクラスの取得方法を変更 #2339

Merged
merged 9 commits into from
Nov 5, 2024

Conversation

sabonerune
Copy link
Contributor

内容

#2298 (review) で提案されたリファクタリングです

その他

EngineAndVvppControllerがget~で煩雑になった感じがします。
ただ、複数のマネージャーをまとめて管理するためのものだから仕方がない?

@sabonerune sabonerune requested a review from a team as a code owner November 4, 2024 02:45
@sabonerune sabonerune requested review from Hiroshiba and removed request for a team November 4, 2024 02:45
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.

おー良さそう!! ありがとうございます!!!

EngineAndVvppControllerがget~で煩雑になった感じがします。

たしかにです!
ちょっと2つ案を思いついたので提案まで!

  1. managerを取ってくる部分はメソッドの最初に置く

↓before

// AltPortInfosを再生成する。
const engineInfoManager = getEngineInfoManager();
engineInfoManager.initializeAltPortInfo();

↓after

const engineInfoManager = getEngineInfoManager();

// AltPortInfosを再生成する。
engineInfoManager.initializeAltPortInfo();

ロジックに集中できるし、書き方も統一できて良さそう。
まあ定義のために一番上にget書きに行くのが面倒ではあるけど、そんなに長けりゃ分離したほうが良さそう!

  1. getter関数にする

ちょっと書式覚えてないのですが、@getterかなにかをつけてメソッドを定義すると、this.getHoge()ではなくthis.hogeでそのメソッドを呼べた気がします。
これを使えばコードを短縮しつつわかりやすいままにできるかも。

まあ毎回メソッドが呼ばれるのは少しもったいない気がしなくもないかも。
あとthis.が必要になっちゃうのでコードは長くなりがち。

うーーーーーーん。
ちょっと自信ないのですが、getterにしてみるのどうでしょう!!
(書式はproperty.getとかだったかも。)

src/backend/electron/manager/vvppManager.ts Outdated Show resolved Hide resolved
@sabonerune
Copy link
Contributor Author

@Hiroshiba とりあえずgetterにしてみました。

@sabonerune sabonerune requested a review from Hiroshiba November 5, 2024 13:26
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!!!

ちょっとこちらで微調整だけさせていただきます!!!

DIしてた関数の崩しもありがとうございました!
かなりコーティングしやすくなった気がします・・・!!!

src/backend/electron/engineAndVvppController.ts Outdated Show resolved Hide resolved
src/backend/electron/main.ts Outdated Show resolved Hide resolved
src/backend/electron/main.ts Outdated Show resolved Hide resolved
src/backend/electron/manager/RuntimeInfoManager.ts Outdated Show resolved Hide resolved
src/backend/electron/manager/engineProcessManager.ts Outdated Show resolved Hide resolved
@Hiroshiba
Copy link
Member

Hiroshiba commented Nov 5, 2024

マージします!!! ありがとうございました!!!

ちょっとまた次のmain.ts解体のリファクタリングのご提案なんですけど、もしよければ・・・・・!!! 🙇


main.tsはあと5つほどパーツが残ってそうでした!

  • app周り
    • 一番でかい、切り分け方がよくわからないから最後が良さそうな気がする
  • win周り
    • createWindowとかその他とか
    • 若干繋がりが見えづらいけど、外に出せそう
  • プロセス間通信を受け取るとこ
    • registerIpcMainHandle<IpcMainHandle>に集約されてる
    • 関数に切り出したら即終了ではある
    • でも繋がりがちょっと見えないので後にしたいかも
  • ログまわり
    • log.initialize({ preload: false });以下10行ほど
    • 関数に切り出し即終了なはず
    • 先にやっても良い
  • その他electron処理に必要なもの
    • protocol.registerSchemesAsPrivilegedとか
    • これはmain.tsに残るべきな気がする

次はログの関数切り出しか、win周りな気がしました!
ログの関数切り出しはinitializeLogger.tsとか作って切り出せば終わりだと思うので、ちょっとwin周りを追ってみました。
たぶんこうすれば良さそう・・・・・・・・?

WindowManagerの切り出し手順

大まかには WindowManagerを作ってcreateWindow()を移す 感じかなと!

  1. main.tsのstart()を解体する
    • start()を呼んでるとこにstart()の中身を移してstart()を消すだけ
    • 昔はstart()を複数箇所から呼んでたけど、RELOAD_APPが増えて1箇所になったから解体可能になったはず
  2. createWindow()内部でlet winへの代入をやめ、winをreturnするようにし、app.on("ready")内でwin = createWindow()する
  3. WindowManagerクラスを作り、createWindow()を移し、getWindowManager()を作る
    • initializeWindowManager()も要りそう
    • ここで依存解決が必要なので、全部洗い出して↓の「依存の解決方法」にまとめてみました
  4. (オプション)createWindow内のprotocol.handle("app")をmain.tsに戻す
    • protocol.handleは別にここでもいいんだけど、処理的にwindowというよりelectronっぽいので
    • main.tsのprotocol.registerSchemesAsPrivilegedの真下あたりに、app.whenReady().then()で呼ぶ
    • app.on("ready")とどっちが先に呼ばれるかわからない(loadUrlより先に↑を実行したい)ので、app.on("ready")側もapp.whenReady().then()に変える

createWindow()内からの依存の解決方法

  • mainWindowState
    • 普通にimport
  • configManager
    • getConfigManager()で解決!
  • ipcMainSendProxy
    • そのままipc.tsからimportすれば良さそう
  • loadUrl
    • WindowManagerのメソッドとして実装する
    • firstUrlもついでにWindowManagerに持っていく
      • global定数にせず、loadUrlの中で定義すれば良さそう
    • 関数名はloadUrlよりloadのが良い気がする、ついでに変えても良さそう
  • appState
    • 将来的にAppManagerが持つべき
    • でもまだAppManagerがないので、一旦getAppState()を作って渡す形が良さそう
    • initializeWindowManagerの引数でgetAppState: () => appStateを渡し、WindowManagerのコンストラクタにもgetAppStateを渡す
      • やってることはEngineProcessManageronEngineProcessErrorと一緒の感じ

今回は結構難しめですが、ちょっとがっつり手順を考えてみたのでもしよかったら挑戦してみていただけると非常に心強いです!!!!!
もしよければ・・・!!!

@Hiroshiba Hiroshiba merged commit 47f03f8 into VOICEVOX:main Nov 5, 2024
8 checks passed
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