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: WindowManagerを追加 #2455

Merged
merged 13 commits into from
Jan 2, 2025

Conversation

sabonerune
Copy link
Contributor

内容

#2339 (comment) で提案されたwin周りの処理をWIndowManagerに切り出すリファクタリングです。

関連 Issue

@voicevox-preview-pages
Copy link

voicevox-preview-pages bot commented Dec 31, 2024

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

更新時点でのコミットハッシュ:12632b6

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.

WindowManagerBrowserWindowインスタンスを保持するようにしています。
ただしダイアログやIPC周りがwindowを使うので取得できるようになっています。
この設計があまり良くないような感じがします。

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
@sabonerune sabonerune force-pushed the refactor/window-manager branch from 586baf4 to 64adc17 Compare December 31, 2024 07:20
@sabonerune sabonerune marked this pull request as ready for review December 31, 2024 07:20
@sabonerune sabonerune requested a review from a team as a code owner December 31, 2024 07:20
@sabonerune sabonerune requested review from Hiroshiba and removed request for a team December 31, 2024 07:20
@sabonerune sabonerune force-pushed the refactor/window-manager branch 2 times, most recently from 90511fd to d0e2ccf Compare December 31, 2024 12:08
@sabonerune
Copy link
Contributor Author

テストが通らないので一度ドラフトに戻します。

@sabonerune sabonerune marked this pull request as draft December 31, 2024 12:44
@sabonerune sabonerune marked this pull request as ready for review January 1, 2025 13:52
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です!!!
実装も読みやすく、いろいろ考えてくださっている気配も感じました!!ありがとうございます!!

ちょっとコード移動がかなり発生していつつ、ちょくちょくリファクタリングも入っているので、レビューを丁寧に行いました!
今のPRの形で良い感じなので、ここからはコード移動が発生しないような変更(その行の変更・追加・削除)だけを意識していただけると助かります・・・!!!

コメントがばらついちゃいましたが、たぶんこの3点の変更で綺麗に行けそう!!

  • isMaximizedなどをWindowManagerに生やす
  • whenReadyをon(ready)にする
  • 利用されてるdialog.show系関数をWindowManagerの下に生やし、_win有無で呼ぶ関数を切り替える

@sabonerune
Copy link
Contributor Author

sabonerune commented Jan 2, 2025

以下の作業をしました。

  • Window操作系関数をWindowManagerに生やす
  • 利用されてるdialog.show系関数をWindowManagerの下に生やし、_win有無で呼ぶ関数を切り替える
  • second-instanceイベント時にウインドウが無い場合のTODOコメントと警告を追加。

app.whenReady()はそのままです。

ふと気が付いたのですがConfigManagerの初期化とデフォルトエンジンのダウンロードは多重起動防止のために呼んでいるapp.requestSingleInstanceLock()の後に行わないと問題が起こりそうな気がします。

@sabonerune sabonerune requested a review from Hiroshiba January 2, 2025 09:26
@Hiroshiba
Copy link
Member

ふと気が付いたのですがConfigManagerの初期化とデフォルトエンジンのダウンロードは多重起動防止のために呼んでいるapp.requestSingleInstanceLock()の後に行わないと問題が起こりそうな気がします。

たしかに!!
ちょっとこれをついでに位置移動するのは怖いので、app.requestSingleInstanceLock()の近くにTODOコメント書くのお願いできればと!! 🙇

requestSingleInstanceLockの例見るに、そもそもready内で行う必要なさそうな気がしますね!
相当上の方に処理移動しちゃって良さそう感。
https://www.electronjs.org/ja/docs/latest/api/app#apprequestsingleinstancelockadditionaldata

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

細かいコメントをいくつかしていますが、もう自明なのでこちらで変更してマージさせていただこうと思います!!
app.requestSingleInstanceLock()の近くにTODOコメント書くのも)

かなりすっきりしましたね!!

WindowManagerにdialog.show()系を内蔵するのがだいぶ効いてそう!
たぶんまだ他にもelectronのdialog.showを叩いてるコードがいっぱいあると思うので、よかったらそれもgetWindowManger().show系に切り替えちゃったほうが良い気がしてます。(winインスタンス与え忘れ防止になるので)
もしよかったらPRお待ちしてます 🙏

src/backend/electron/main.ts Outdated Show resolved Hide resolved
src/backend/electron/engineAndVvppController.ts Outdated Show resolved Hide resolved
src/backend/electron/main.ts Outdated Show resolved Hide resolved
@Hiroshiba Hiroshiba enabled auto-merge January 2, 2025 10:11
@Hiroshiba Hiroshiba added this pull request to the merge queue Jan 2, 2025
Merged via the queue into VOICEVOX:main with commit fea3334 Jan 2, 2025
11 checks passed
@Hiroshiba
Copy link
Member

Hiroshiba commented Jan 2, 2025

次なるelectron/main.tsの解体ですが、いよいよipcMainHandleをごそっとファイル分けしちゃって良い気がしました!!
↓でregisterIpcMainHandleしているすべてを切り出す感じをイメージしてます。

registerIpcMainHandle<IpcMainHandle>({

ファイル名は何がいいかな・・・型名と被りますが、ipcMainHandle.tsとか・・・?
たぶん関連する関数などもごそっと移動するので、main.tsの見通しはかなり良くなるんじゃないかなと思ってます!!

設計ですが、このipcMainHandleだけは他のmanagerなどと違い、これは必ずregisterIpcMainHandleにしか渡されないので、クラスやinitializeは不要でそのままインスタンスを返しちゃって良い気がします。
getIpcMainHandle()関数だけがexportされてる感じ。
そしてapp系だけはmanagerがないので引数で渡す・・・!

たぶんこんな感じになるかなと!!

// ipcMainHandle.ts
export function getIpcMainHandle(
  app: 型不明, // TODO: AppManagerを作ってgetAppManager()に置き換える
  getAppState: 型不明,
): IpcMainHandle {
  const configManager = getConfigManager()
  // 他の必要なインスタンスを宣言

  return {
    GET_APP_INFOS: () => {
      // 実装もコピー
    },

    GET_TEXT_ASSET: async (_, textType) => {
      // 実装もコピー
    },

    // ...
  } satisfies IpcMainHandle
}
// electron/main.ts
registerIpcMainHandle(getIpcMainHandle())

もちろんipcMainHandle内で使ってたローカル関数等もipcMainHandle.tsに移動する感じで!!
(appとipcMainHandleで共有してる関数があるとどこに置くか考えないといけない)

この切り出しが本当に良いものなのか、あるいはもっと良いipcMainHandleの扱い方があるかもしれないのですが、ちょっと設計力がなくてこれ以上のが思いつかないというのが本音です 😇
1回分けてみたら見えてくるものもある、と信じてとりあえず切り分けてみる感じでいます。
きれいなリファクタリングは専用の考え方が必要っぽく、とにかくやってみて勉強かなと思ってます。。。

もしよければぜひ!!!
(他に案があればそちらもぜひ!!!)

@sabonerune sabonerune deleted the refactor/window-manager branch January 2, 2025 12:15
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