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: getIpcMainHandle を追加 #2563

Merged
merged 4 commits into from
Feb 26, 2025

Conversation

sabonerune
Copy link
Contributor

内容

#2455 (comment) で提案されたリファクタリングです。

その他

特に大きな問題なく切り離せたと思います。

@sabonerune sabonerune requested a review from a team as a code owner February 25, 2025 08:11
@sabonerune sabonerune requested review from Hiroshiba and removed request for a team February 25, 2025 08:11
@voicevox-preview-pages
Copy link

voicevox-preview-pages bot commented Feb 25, 2025

🚀 プレビュー用ページを作成しました 🚀

更新時点でのコミットハッシュ:092dd7d

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!!!

良い切り出しだと思います!!!

*/
async function retryShowSaveDialogWhileSafeDir<
T extends Electron.OpenDialogReturnValue | Electron.SaveDialogReturnValue,
>(showDialogFunction: () => Promise<T>, appDirPath: string): Promise<T> {
Copy link
Member

Choose a reason for hiding this comment

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

(ただのコメントです)

このappDirPathとかもAppManagerを作って取得可能にしたいですねぇ!!

@Hiroshiba Hiroshiba enabled auto-merge February 26, 2025 16:26
@Hiroshiba Hiroshiba added this pull request to the merge queue Feb 26, 2025
Merged via the queue into VOICEVOX:main with commit da3e457 Feb 26, 2025
11 checks passed
@Hiroshiba
Copy link
Member

Hiroshiba commented Feb 26, 2025

次は最後のAppManager!!!
と思ってmain.tsをいろいろ眺めてたのですが、ちょっとよくわからないですね。。。。。。。。。 😇

なんかこう・・・・・・appを使っているコードをきれいに分離できないと言うか、何個か違う役割を担ってそうな気がするんですよね・・・・・・
雰囲気、VOICEVOXアプリとして起動・終了を担っているコードと、多重起動時の処理みたいなelectronの制御をするコードがいる感じがする・・・・・。

ちょっと自信ないのですが、一旦切り出してみる感じで、AppLifecycleManagerを作ってみるとかどうでしょう!!!
VOICEVOXに関わる起動・終了系のコードを集める感じになると思います。
やらないこととしてはログディレクトリの指定とか、app:プロトコル登録とか。

ちょっと読みきれてないのですが、こんな感じとかでどうでしょう!!

  • AppLifecycleManagerクラスを作る
  • appStateをプライベートインスタンス変数として持ち、setAppStategetAppStateを作る
    • willQuitしかないのでsetWillQuitgetWillQuitだけで良いかもしれない
    • AppStateオブジェクトをたらい回しにできうるの危ないので、このタイミングでwillQuitだけにしたいかも
  • initializeメソッドを作る
    • ここにapp.onだのapp.whenReadyだのを持っていく
    • もっと良いメソッド名がある気がする。。。
  • 2回目のapp.whenReady()を移す
    • configManager.initialize()から始まる方
    • onReadyメソッドを作って、thenで渡してるコールバックを全部onReadyに移動させ、app.whenReady().then(this.onReady)みたいにする
    • 関数分けて小分けにしやすいようにしつつ、ネスト数を下げるため
  • "before-quit"を移動する
    • whenReadyと同じくinitializeメソッドに
  • will-finish-launchingは移動しない
    • 代わりにinitialFilePathGetterをManagerに渡す形にする
    • 将来的にInitialFilePathManagerみたいなのを作ってそっちに移すのが良い気がする
      • ちょっと自信なし。。。
  • window-all-closedも移動する・・・・というかこれ要らなくない・・・?
    • app.quit()呼んでるだけで、かつドキュメント見た感じデフォルトでquit()は呼ばれる
    • 検証して不要そうだったら別PRで先に消してもらえるとありがたい
  • "second-instance"も移動しない
  • 型エラー潰して終わり。

うーーーーーーーーーーーーーーん。。。これで良いのかちょっと自信ないのですが。。。。
正直やってみないとわからない領域に入ってる気がするので、ちょっとやってみていただけると助かります!!!!!
提案あればぜひ。。。。

あ、先にリファクタリングをする手もあると思います。main.tsにある小さい括りを適当に関数にして呼ぶだけですが。
といっても関数は別ファイルに切り出さず、例えばelectronLog周りのコードをまとめてinitializeElectronLog関数にして直後に呼ぶ、みたいなことしていくだけみたいな。
見通しは立てやすくなるはずではあります。

あとinstallExtension周りとか、どう考えてもライフサイクルに関係ないものがwhenReadyに混じってるので先にそれを分けちゃうのもありかもです!
whenReadyは何度呼んでも良いので、whenReady().then(() => {installExtension周りのコード})みたいにして分けれるはず。
あとrequestSingleInstanceLock周りも避けれるはず・・・だけどこれを避けるのはちょっと厳しそう。待機しないといけないからpromise化しないとだけど、うまい方法が思いつかない。
configManager.initialize()も避けれるはずだけど、同じく起動順序を気にしないといけない。configManagerwhenInitializedを作ってapp.whenReadyと同じような実装でPromiseを返せば良いけど、やり方調べないとダメそう。

おすすめは~~~~~~まあ先にAppLifecycleManager作ってあとからリファクタリングする感じかなと!!!!!!!!!
またもしよければ!!!!

@sabonerune sabonerune deleted the refactor/ipcMainHandle branch February 27, 2025 01:20
@sabonerune
Copy link
Contributor Author

@Hiroshiba window-all-closedはいらなそうなので削除PRを作ります。

そういえばinstallExtensionなのですが前から機能していないようで調べてみるとElectron側で問題が発生しているようです。 MarshallOfSound/electron-devtools-installer#244

@Hiroshiba
Copy link
Member

Hiroshiba commented Feb 27, 2025

そういえばinstallExtensionなのですが前から機能していないようで調べてみるとElectron側で問題が発生しているようです。

な、なるほど。。。。
できれば実装したくはあるのでコードは実装しておいても良さそうだけど、動いてないのは問題ですねぇ。。。。

ここで迂回作とかが議論されていそう。もうちょっと待っても良いかも。

VOICEVOXエディタ内にもissue作っても良いかもですね!

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