-
Notifications
You must be signed in to change notification settings - Fork 382
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
feat: capture Electron IPC messages for opensumi devtools #1583
Conversation
Codecov ReportBase: 57.69% // Head: 57.81% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1583 +/- ##
==========================================
+ Coverage 57.69% 57.81% +0.12%
==========================================
Files 1254 1271 +17
Lines 78152 79119 +967
Branches 16333 16575 +242
==========================================
+ Hits 45088 45745 +657
- Misses 30093 30365 +272
- Partials 2971 3009 +38
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
bb48aaf
to
9a2e7f8
Compare
收到!学到了~
Dan ***@***.***>于2022年9月5日 周一11:02写道:
… ***@***.**** requested changes on this pull request.
------------------------------
In packages/connection/src/common/utils.ts
<#1583 (comment)>:
> @@ -37,8 +37,8 @@ export function parse(input: string, reviver?: (this: any, key: string, value: a
}
export function getCapturer() {
- if (typeof window !== 'undefined' && window.__OPENSUMI_DEVTOOLS_GLOBAL_HOOK__?.capture) {
- return window.__OPENSUMI_DEVTOOLS_GLOBAL_HOOK__.capture;
+ if (typeof window !== 'undefined' && window.__OPENSUMI_DEVTOOLS_GLOBAL_HOOK__?.captureRpc) {
那些地方的写法也是不规范的,有机会可以一起改掉,这种专有名词出现的时候还是要大写比较规范,例如 macOS,后面的 OS 全程是 Operating
System,所以简称一般用大写表示,这里的 IPC,RPC 同样也是缩写,因此规范的写法也要是大写,比较好识别
------------------------------
In packages/core-electron-main/src/bootstrap/window.ts
<#1583 (comment)>:
> @@ -94,6 +95,10 @@ export class CodeWindow extends Disposable implements ICodeWindow {
...this.appConfig.overrideBrowserOptions,
...options,
});
+
+ // initialize for OpenSumi DevTools
+ initForDevtools(this.browser);
一般对于这种开发工具代码,有点类似“后门”的程序,最好还是把是否集成的逻辑交给集成方去决定,默认开启的话会造成一些不必要的问题,用户可能在未知的情况下升级版本过程中集成了你这个特性,这里比较建议通过
AppCofig 中追加配置项来控制,例如 devtool: boolean, 默认值在开发态时为 True,这样不会影响已集成的用户
—
Reply to this email directly, view it on GitHub
<#1583 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHXOSWAOCMI2RMGKKPQH2Y3V4VPDTANCNFSM57WMZDEQ>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
@erha19 老师,“专有名词大小写”的问题已经解决,在devtools项目中也有对应的修改。 但是,“增加配置项”这个要求我虽然push了一个commit,但还没有完成,需要帮助。我的思路和遇到的困难是这样的: 截止到此PR最新的commit,在opensumi/core中,与devtools相关的代码有多处:
目前,通过新加入的 此外需要注意的是,对于Electron端来说,控制devtools的开关需要考虑两个地方:
所以在此PR中,我分别在 (可能描述得不是很清晰,再配一个图) |
IClientAppOpts 里面配置 devtools 就好了,至于 electron preload 的内容,可以加到 metadata 中,通过 metadata 去获取是否开启 devtools |
谢谢 @yantze 老师!metadata这部分能不能稍微再展开一下~ |
这里的 electron 的 appConfig 和 browser 的 appConfig 是两个实例,确实都要声明的 |
感谢 @yantze 老师的保姆级指南,使得browser-preload在现在也能被配置了! 此外,利用依赖注入能力,我在chrome-devtools.contribution.ts中根据
目前唯一遗留下来的是connection模块的配置,ide-core-browser模块似乎不能在这里被import,于是没法使用
其实分开设置也能得到合理的解释:如果把core-electron-main的 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@erha19 老师的requested changes里,最重要的是上面这个,因为影响功能的正确性,所以我仔细捋了一下,在此更新: 图中提到的套娃问题是刚刚发现并验证的,之前没有考虑到;第二个问题“ipcMain.on从各个renderer收到的消息都会被每一个窗口捕获”不知道是不是Dan老师说的“串了”,这的确是不合适的。
这个测试是这样的:打开两个窗口,拖移其中一个窗口,两个窗口的IPC消息列表中都出现了“ipcRenderer.on”事件。于是我马上回复了“的确有这个Bug!”。但今天在捋思路的时候,我发现了这个: core/packages/core-electron-main/src/bootstrap/api.ts Lines 99 to 104 in ebe8d66
opensumi/core中已经存在对Electron API的代理,上面代码是其中的一部分。会遍历app的所有窗口并发送消息,昨天我遇到的是这种情况。所以其实不是我以为的“串了”,而是设计就是这样。Q:为什么要遍历发送消息呢? 以上是关于“消息串了”的一些思考,下一个comment是我的新思路。 |
新思路: 我认为不要捕获main进程这侧的消息了(ipcMain.on、ipcMain.handle...),在devtools中只呈现renderer侧的消息(ipcRenderer.on、ipcRenderer.send、ipcRenderer.invoke、ipcRenderer.sendSync)。 理由如下:
若采用这个新思路,上面的几个requested changes都迎刃而解了。 |
But capture the return values of ipcRenderer.sendSync and ipcRenderer.invoke
2129d90
to
b3840bc
Compare
以下是新思路下的IPC view,“↑”和“↓”都是以renderer(即窗口)的视角看待的: 由于core中使用ipcRenderer.sendSync()和ipcRenderer.invoke()基本没有,所以自己本地加了两个测试channel用于测试效果。OpenSumi DevTools那里也做了相应的修改。 |
@tyn1998 新思路可以的,也能减少对于 IPC 和 RPC 的兼容写法,只关注 Browser/Renderer 进程的消息就可以了 |
ipcRenderer.invoke 之后会有,目前只监听 renderer 侧也可以的 |
#1098 also requires implementing IPC messages capturing and presenting within opensumi devtools. This PR provides messages capturing support for opensumi devtools.
Types
Background or solution
By proxies of a few Electron IPC API to capture IPC messages for opensumi/devtools#5.
Also modify RPC capturing code (only name, not logic).
Changelog
Electron IPC messages capturing support for opensumi devtools.