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

fix: uninstall extension failed in electron #2469

Merged
merged 2 commits into from
Mar 28, 2023

Conversation

yantze
Copy link
Member

@yantze yantze commented Mar 23, 2023

Types

  • 🐛 Bug Fixes

Background or solution

close opensumi/ide-electron#64

electron 环境卸载插件调用了 fileService.delete, delete 实现中判断如果是 electron 并且设置了 moveToTrash 就会使用 electron 的 api 删除文件。但如果没有设置 moveToTrash 标记,就会走原始的删除文件。原始的删除文件在 mac 下会报错, 找不到 app/node/macos-trash 可执行文件。

在 ide-electron 环境中,删除插件其实移到 Trash 可能更好,避免一些误操作导致文件无法恢复。

Changelog

修复 ide-electron 环境卸载插件失败的问题

@github-actions github-actions bot added the 🐞 bug Something isn't working label Mar 23, 2023
@yantze yantze force-pushed the fix/uninstall-ext-fail-electron branch from cf6ff94 to 1405b83 Compare March 23, 2023 03:44
@codecov
Copy link

codecov bot commented Mar 23, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +4.98 🎉

Comparison is base (90e08d7) 52.76% compared to head (74b1abb) 57.74%.

Additional details and impacted files
@@            Coverage Diff             @@
##            v2.23    #2469      +/-   ##
==========================================
+ Coverage   52.76%   57.74%   +4.98%     
==========================================
  Files        1323     1323              
  Lines       83472    83472              
  Branches    17374    17374              
==========================================
+ Hits        44046    48205    +4159     
+ Misses      35789    32045    -3744     
+ Partials     3637     3222     -415     
Flag Coverage Δ
jsdom 52.76% <100.00%> (-0.01%) ⬇️
node 16.76% <0.00%> (?)

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

Impacted Files Coverage Δ
...es/file-service/src/browser/file-service-client.ts 74.40% <100.00%> (ø)

... and 106 files with indirect coverage changes

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 in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@yantze
Copy link
Member Author

yantze commented Mar 23, 2023

/publish

@github-actions
Copy link
Contributor

🎉 PR Next version 2.23.2-next-1679551981.0 publish successful! You can install prerelease version via npm install [email protected] @yantze

2.23.2-next-1679551981.0

/home/runner/work/_temp/_runner_file_commands/step_summary_44383e2c-0cb6-42e1-b526-d772b8d50b5e

@github-actions
Copy link
Contributor

ChatGPT Code Review:

CodeReview:

这段代码修改的是 FileServiceClient 类中的 delete 方法,在删除文件时,如果设置了 moveToTrashtrue(这是在 electron 渲染进程中执行),会调用 moveToTrash 方法将文件移动到垃圾箱。如果不设置 moveToTrash 或设为 false,则直接删除文件而不放入垃圾箱。

在修改前的代码中,判断条件是 options && options.moveToTrash,表示需要同时存在 options 对象和 moveToTrash 属性为真。在修改后的代码中,判断条件则是 options?.moveToTrash !== false,这里使用了可选链和 nullish coalescing 运算符,表示只要 moveToTrash 不为 false 或 undefined,就会执行代码块。

修改后的代码如果 options?.moveToTrash 为 false,则直接执行删除操作,不再进行其他判断。这里做了一个细微的改动,但是可以提高代码的安全性和可读性。

代码修改建议:

无需修改,这段代码已经实现功能,且修改后的代码相较于修改前,更为健壮易读。

commit message:

chore(file-service): fix uninstall extension failed in electron

将 file-service 模块改动提交至代码仓库中,并修正 electron 情况下卸载扩展失败的问题。

Copy link
Member

@erha19 erha19 left a comment

Choose a reason for hiding this comment

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

LGTM, 相关问题 #2410

@erha19 erha19 merged commit 553a378 into v2.23 Mar 28, 2023
@erha19 erha19 deleted the fix/uninstall-ext-fail-electron branch March 28, 2023 08:53
@shilin8805
Copy link
Contributor

@yantze 这个在windows上还是有问题。windows上uriString传进来的是C:\xxx\xxx\这种格式的,没有scheme,导致uri.scheme === Schemes.file判断的时候没有走进去。
image

@yantze
Copy link
Member Author

yantze commented Apr 4, 2023

@yantze 这个在windows上还是有问题。windows上uriString传进来的是C:\xxx\xxx\这种格式的,没有scheme,导致uri.scheme === Schemes.file判断的时候没有走进去。 image

可以提供一下复现路径,目前没有在 OpenSumi 2.23.2 版本上复现这个问题

@shilin8805
Copy link
Contributor

shilin8805 commented Apr 4, 2023

@yantze 这个在windows上还是有问题。windows上uriString传进来的是C:\xxx\xxx\这种格式的,没有scheme,导致uri.scheme === Schemes.file判断的时候没有走进去。 image

可以提供一下复现路径,目前没有在 OpenSumi 2.23.2 版本上复现这个问题

@yantze 我也试的2.23.2版本,uriString是C:\这种格式的,前面没有scheme,new URI后,scheme变成了C,你重现的uriString是什么?

WX20230404-161228@2x

@yantze
Copy link
Member Author

yantze commented Apr 4, 2023

确实是 uriString 没有进行处理,我这边修复一下

@yantze
Copy link
Member Author

yantze commented Apr 4, 2023

已提交 #2548

@erha19 erha19 added this to the 2.24 milestone Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants