[go: nahoru, domu]

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

feat: capture Electron IPC messages for opensumi devtools #1583

Merged
merged 10 commits into from
Sep 26, 2022

Conversation

tyn1998
Copy link
Member
@tyn1998 tyn1998 commented Aug 26, 2022

#1098 also requires implementing IPC messages capturing and presenting within opensumi devtools. This PR provides messages capturing support for opensumi devtools.

Types

  • 🎉 New Features

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.

@codecov
Copy link
codecov bot commented Aug 26, 2022

Codecov Report

Base: 57.69% // Head: 57.81% // Increases project coverage by +0.12% 🎉

Coverage data is based on head (b3840bc) compared to base (0a64da0).
Patch coverage: 60.00% of modified lines in pull request are covered.

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     
Flag Coverage Δ
jsdom 52.61% <60.00%> (+0.11%) ⬆️
node 15.83% <60.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/connection/src/common/utils.ts 94.44% <50.00%> (ø)
packages/core-browser/src/bootstrap/app.ts 64.68% <66.66%> (-0.10%) ⬇️
packages/utils/src/date.ts 80.00% <0.00%> (-20.00%) ⬇️
packages/editor/src/browser/history/index.ts 65.71% <0.00%> (-18.04%) ⬇️
...owser/monaco-contrib/tokenizer/textmate.service.ts 38.76% <0.00%> (-4.97%) ⬇️
...extension/src/hosted/api/sumi/ext.host.api.impl.ts 89.65% <0.00%> (-3.21%) ⬇️
.../extension/src/hosted/api/sumi/ext.host.webview.ts 82.14% <0.00%> (-3.05%) ⬇️
...ages/quick-open/src/browser/quick-input-service.ts 65.11% <0.00%> (-2.39%) ⬇️
packages/extension/src/common/vscode/converter.ts 38.91% <0.00%> (-2.17%) ⬇️
...scode/api/debug/extension-debug-session-factory.ts 42.22% <0.00%> (-1.37%) ⬇️
... and 69 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.
📢 Do you have feedback about the report comment? Let us know in this issue.

@tyn1998 tyn1998 changed the title WIP: feat: capture Electron IPC messages for opensumi devtools feat: capture Electron IPC messages for opensumi devtools Aug 27, 2022
@tyn1998 tyn1998 marked this pull request as ready for review August 27, 2022 07:18
packages/connection/src/common/utils.ts Outdated Show resolved Hide resolved
packages/core-electron-main/src/bootstrap/window.ts Outdated Show resolved Hide resolved
packages/connection/src/common/utils.ts Outdated Show resolved Hide resolved
packages/core-electron-main/src/bootstrap/window.ts Outdated Show resolved Hide resolved
@tyn1998
Copy link
Member Author
tyn1998 commented Sep 5, 2022 via email

@tyn1998
Copy link
Member Author
tyn1998 commented Sep 11, 2022

@erha19 老师,“专有名词大小写”的问题已经解决,在devtools项目中也有对应的修改

但是,“增加配置项”这个要求我虽然push了一个commit,但还没有完成,需要帮助。我的思路和遇到的困难是这样的:

截止到此PR最新的commit,在opensumi/core中,与devtools相关的代码有多处

  1. 暴露global hook的地方
  2. connection模块
  3. chrome-devtools.contribution.ts(目前只用于获取网络延迟)
  4. core-electron-main/src/bootstrap/window.ts中捕获ipcMain的消息
  5. core-electron-main/browser-preload/index.js中捕获ipcRenderer的消息

目前,通过新加入的devtools: boolean配置项,我可以控制1和4的开关,但是对于2、3、5我没有找到合适方法在相应的文件中获取配置项的值,于是就没法判断是否要运行与devtools有关的代码了。这就是我遇到的困难,此处可能还需要 @yantze 老师。

此外需要注意的是,对于Electron端来说,控制devtools的开关需要考虑两个地方:

  1. core-browser捕获RPC的部分(对web端来说只要考虑这一个就行)
  2. core-electron-main捕获IPC的部分。

所以在此PR中,我分别在ElectronAppConfigIClientAppOpts两个配置项接口中加入了devtools这个配置项。于是,对于集成方来说,就要设置两个devtools,但事实上这两个devtools要么都是true要么都是false。所以,要是IClientAppOpts.devtools能通过ElectronAppConfig.devtools给出就好了,这又涉及到上面说的困难——如何把配置项传到需要的地方。

image

(可能描述得不是很清晰,再配一个图)

@yantze
Copy link
Member
yantze commented Sep 13, 2022

IClientAppOpts 里面配置 devtools 就好了,至于 electron preload 的内容,可以加到 metadata 中,通过 metadata 去获取是否开启 devtools

@tyn1998
Copy link
Member Author
tyn1998 commented Sep 13, 2022

可以加到 metadata 中,通过 metadata 去获取是否开启 devtools

谢谢 @yantze 老师!metadata这部分能不能稍微再展开一下~

@yantze
Copy link
Member
yantze commented Sep 13, 2022

可以加到 metadata 中,通过 metadata 去获取是否开启 devtools

谢谢 @yantze 老师!metadata这部分能不能稍微再展开一下~

  1. electron appConfig -> 2. electron window metadata -> 3. preload electronEnv.meta -> 4. renderApp(opts)

  2. electron appConfig 初始化
    https://github.com/tyn1998/opensumi-core/blob/fad5527511/packages/core-electron-main/src/bootstrap/app.ts#L81

  3. 传递到 metadata
    https://github.com/tyn1998/opensumi-core/blob/fad5527511/packages/core-electron-main/src/bootstrap/window.ts#L128

  4. 通过 preload 获取 meta
    https://github.com/tyn1998/opensumi-core/blob/fad552751128735acdc583f9ef832723c5f0a88c/packages/core-electron-main/browser-preload/index.js#L93

  5. 初始化 browser appConfig 通过 electronEnv.metadata 设置

这里的 electron 的 appConfig 和 browser 的 appConfig 是两个实例,确实都要声明的

@tyn1998
Copy link
Member Author
tyn1998 commented Sep 15, 2022

感谢 @yantze 老师的保姆级指南,使得browser-preload在现在也能被配置了!

此外,利用依赖注入能力,我在chrome-devtools.contribution.ts中根据appConfig.devtools的值进行了判断,于是也支持配置了。

目前唯一遗留下来的是connection模块的配置,ide-core-browser模块似乎不能在这里被import,于是没法使用appConfig。不过,这里就算不配置也问题不大,因为capture本身就会判断一些条件。


于是,对于集成方来说,就要设置两个devtools,但事实上这两个devtools要么都是true要么都是false。

其实分开设置也能得到合理的解释:如果把core-electron-main的devtools设置为false但保留core-browser的devtoolstrue,那么相当于不要捕获IPC只捕获RPC,于是我在OpenSumi DevTools中判断了这种情况,在这种情况下Electron的OpenSumi DevTools将和Web端一样,只会显示RPC视图。

yantze
yantze previously approved these changes Sep 19, 2022
Copy link
Member
@yantze yantze left a comment

Choose a reason for hiding this comment

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

LGTM

packages/core-electron-main/src/bootstrap/devtools.ts Outdated Show resolved Hide resolved
packages/core-electron-main/src/bootstrap/app.ts Outdated Show resolved Hide resolved
packages/core-electron-main/browser-preload/index.js Outdated Show resolved Hide resolved
packages/core-electron-main/src/bootstrap/devtools.ts Outdated Show resolved Hide resolved
@tyn1998
Copy link
Member Author
tyn1998 commented Sep 24, 2022

另外有个问题,一个 Electron 客户端只会有一个 Main 进程,即会存在一个 Main 进程对应多个 Renderer 进程的情况,如果这里把 Main 通信消息都往 Browser 进程发,是不是会导致窗口 A,B 的通信消息串了的问题?比较理想的方式可能需要根据 WindowId 把消息区分处理一下

@erha19 老师的requested changes里,最重要的是上面这个,因为影响功能的正确性,所以我仔细捋了一下,在此更新:

image

图中提到的套娃问题是刚刚发现并验证的,之前没有考虑到;第二个问题“ipcMain.on从各个renderer收到的消息都会被每一个窗口捕获”不知道是不是Dan老师说的“串了”,这的确是不合适的。

@erha19 老师,在Electron客户端打开了两个文件夹(两个窗口)测试,的确有这个Bug!我会修复的。

这个测试是这样的:打开两个窗口,拖移其中一个窗口,两个窗口的IPC消息列表中都出现了“ipcRenderer.on”事件。于是我马上回复了“的确有这个Bug!”。但今天在捋思路的时候,我发现了这个:

this.app.getCodeWindows().forEach((window) => {
const browser = window.getBrowserWindow();
if (!browser.isDestroyed()) {
browser.webContents.send('event:' + name, event, ...args);
}
});

opensumi/core中已经存在对Electron API的代理,上面代码是其中的一部分。会遍历app的所有窗口并发送消息,昨天我遇到的是这种情况。所以其实不是我以为的“串了”,而是设计就是这样。Q:为什么要遍历发送消息呢?

以上是关于“消息串了”的一些思考,下一个comment是我的新思路。

@tyn1998
Copy link
Member Author
tyn1998 commented Sep 24, 2022

新思路:

我认为不要捕获main进程这侧的消息了(ipcMain.on、ipcMain.handle...),在devtools中只呈现renderer侧的消息(ipcRenderer.on、ipcRenderer.send、ipcRenderer.invoke、ipcRenderer.sendSync)。

理由如下:

  1. 在RPC消息的呈现中,我们关心的就是客户端这侧的收发消息,服务端的不管的
  2. 现在虽然捕获了ipcMain.on和ipcMain.handle的消息,但都是接受的消息,这些消息的内容和channel另一侧ipcRenderer.send/invoke/sendSync的发送的内容是一样的,再呈现一次没什么意义
  3. 对用户来说,真正有意义的是renderer侧调用ipcRenderer.sendSync/invoke后从main进程得到的返回值——这反而还没有捕获
  4. DevTron也只代理了ipcRenderer,而没有涉及ipcMain(但它捕获了ipcRenderer.sendSync的返回值,我之前在注释里的TODO就是希望把返回值也捕获)

若采用这个新思路,上面的几个requested changes都迎刃而解了。

@yantze @erha19 两位老师怎么看?

But capture the return values of ipcRenderer.sendSync
and ipcRenderer.invoke
@tyn1998
Copy link
Member Author
tyn1998 commented Sep 25, 2022

以下是新思路下的IPC view,“↑”和“↓”都是以renderer(即窗口)的视角看待的:

image

由于core中使用ipcRenderer.sendSync()和ipcRenderer.invoke()基本没有,所以自己本地加了两个测试channel用于测试效果。OpenSumi DevTools那里也做了相应的修改

@erha19
Copy link
Member
erha19 commented Sep 26, 2022

@tyn1998 新思路可以的,也能减少对于 IPC 和 RPC 的兼容写法,只关注 Browser/Renderer 进程的消息就可以了

@yantze
Copy link
Member
yantze commented Sep 26, 2022

ipcRenderer.invoke 之后会有,目前只监听 renderer 侧也可以的

@erha19 erha19 merged commit 0469c5f into opensumi:main Sep 26, 2022
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.

3 participants