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

IPC周りの呼び出しをdot記法で書けるように #2240

Merged
merged 8 commits into from
Aug 29, 2024

Conversation

MT224244
Copy link
Contributor

内容

#2088 の実現方法を参考に、IPC周りの呼び出しをdot記法で書けるようにしてみました。

関連 Issue

スクリーンショット・動画など

その他

@MT224244 MT224244 requested a review from a team as a code owner August 21, 2024 17:04
@MT224244 MT224244 requested review from Hiroshiba and removed request for a team August 21, 2024 17:04
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.

プルリクエストありがとうございます!!

もしかしたらより使い勝手が良いというか、メンテナンスしやすい形があるかもですが、とりあえず初学者の人がコードをコードジャンプで追えるようになってとても良いと思います!!
(まあ参照に移動しないといけないのでちょっと難度は高いのですが、)


@Segu-g さん
すみません!
もしお時間あれば型周りに関してアドバイスいただけると心強いです!!

@MT224244 MT224244 marked this pull request as draft August 22, 2024 15:27
@MT224244
Copy link
Contributor Author

受信側をオブジェクトで書くことで実装へコードジャンプが可能になりました。
なぜこの書き方でコードジャンプ可能になったのかは分かっていません…。

オブジェクトで書くようになった影響で未実装(未使用)の項目が見つかったので、それをどうするかが決まるまではDraftにしておこうと思います。

@MT224244 MT224244 marked this pull request as ready for review August 24, 2024 09:25
Copy link
Member

@Segu-g Segu-g left a comment

Choose a reason for hiding this comment

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

型を確認しました!
mainとrendererのどちらからもコードジャンプできていて良いと思います!

また、この辺りのコードが置き換え忘れているようです!
https://github.com/VOICEVOX/voicevox/pull/2240/files#diff-d2475d73ba5acb680e9dee5e4253a9bc70d470ed5835750c9832f21074090cd4R460-R476

src/backend/electron/preload.ts Outdated Show resolved Hide resolved
src/backend/electron/ipc.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.

ほぼLGTMです!!
実装ありがとうございます!!

型はもっと詰められそうかもですが(registerIpcMainHandleに渡すものがちゃんとVOICEVOXが想定するIPC通信用のオブジェクトになってるかなど)、まあこだわり過ぎなくても良いかもですね!

src/backend/electron/ipc.ts Show resolved Hide resolved
src/backend/electron/ipc.ts Outdated Show resolved Hide resolved
src/backend/electron/preload.ts Outdated Show resolved Hide resolved
src/type/ipc.ts Outdated Show resolved Hide resolved
src/backend/browser/sandbox.ts 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.

LGTM!!!!

コードジャンプができるようになって初見の方が迷うポイントが1つ減ったと思います!!
ありがとうございます!!!!!


ちょっと話変わるんですが、チラッと出ていたレンダラー側を「フロントエンド」と呼んでIPC通信周りのコードを整理していくタスク、ちょっと興味あったりされませんか!
多分この辺り一番詳しいのMTさんなので、お任せできれば心強いな~~~~と思った次第です!!

ただちょっとややこしいので、何か他に興味があったりしたら全然大丈夫です!
もしご興味あればissueの方に質問or提案コメントなんかをいただけると!!!

本当にご興味あればぐらいの気持ちです 🙏
まあ今この辺りの知識にフレッシュなので勢いあるうちにやっときたい、くらい!

src/backend/electron/ipc.ts Outdated Show resolved Hide resolved
src/backend/browser/sandbox.ts Outdated Show resolved Hide resolved
@Hiroshiba
Copy link
Member

あ、 @Segu-g さんのrequestedが未解決に残ってるのでマージは保留ということで!
問題ないと思うので明日ぐらいにマージさせていただこうと思います! 🙏
(早めにマージしないとコンフリクトがいっぱい出てきちゃう)

Copy link
Member

@Segu-g Segu-g left a comment

Choose a reason for hiding this comment

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

私の指摘箇所について修正されていることを確認しました!その他も問題なさそうです!
LGTM!

@Segu-g Segu-g merged commit b9ea2fa into VOICEVOX:main Aug 29, 2024
9 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.

4 participants