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: reset yTextMap when file updated by system #2260

Merged
merged 1 commit into from
Feb 14, 2023
Merged

Conversation

pipiiiiii
Copy link
Contributor

Types

  • 🐛 Bug Fixes

Background or solution

#2238

问题原因是在打开文件时,协同模块会根据 yTextMap 内容更新文档,文件窗口关闭后通过其他方式直接修改文件后,yTextMap 内容未更新,导致打开文件时,依然是上次关闭的内容

@CLAassistant
Copy link

CLAassistant commented Feb 9, 2023

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added the 🐞 bug Something isn't working label Feb 9, 2023
@pipiiiiii pipiiiiii requested a review from Ricbet February 9, 2023 07:59
@@ -266,6 +281,16 @@ export class CollaborationService extends WithEventBus implements ICollaboration
}
};

private handleFileChange(e: FileChangeEvent) {
e.forEach((change) => {
// 只有从文件系统更新,并且窗口未打开情况,才重置 yTextMap
Copy link
Member

Choose a reason for hiding this comment

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

如果窗口已打开呢?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

窗口打开时候,文件更新会同步,相应的 map 内容也会更新

Copy link
Member

@situ2001 situ2001 Feb 9, 2023

Choose a reason for hiding this comment

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

曾经考虑过 简单地 进行监听文件系统更新,但以失败告终,供参考:situ2001#19

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这个 PR 主要是考虑非协同场景的 BUG,目前单用户加载了协同模块会出现通过 git 等操作修改代码后再打开文件,内容被恢复了。
协同场景的文件变更同步还需要修改 FileWatcher,现在文件监听在协作场景还有 BUG,需要单独开一个 Issue 解决了

Copy link
Member

Choose a reason for hiding this comment

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

一个粗略的想法:把 Model <-> Local File 这个直接依赖给切了。变为 Model <-> CRDT <-> Local File。Node.js端负责CRDT <-> Local File同步。前端剩下Model <-> CRDT。即文件保存不由前端调用RPC方法来实现(FileSchemeDocClientService.saveByContent),而是后端自己同步CRDT与文件来实现。

现有情况下,Monaco Model会与本地文件进行同步。(BaseFileSystemEditorDocumentProvider)
比如:频繁地改动保存本地文件。IDE前端右下角会出现文件不同步的报错。

当然,工作量会不小。

Copy link
Member

Choose a reason for hiding this comment

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

这个 PR 主要是考虑非协同场景的 BUG,目前单用户加载了协同模块会出现通过 git 等操作修改代码后再打开文件,内容被恢复了。 协同场景的文件变更同步还需要修改 FileWatcher,现在文件监听在协作场景还有 BUG,需要单独开一个 Issue 解决了

本地测试过,这个修复暂时没有问题。

@Ricbet Ricbet requested a review from erha19 February 9, 2023 08:06
@codecov
Copy link

codecov bot commented Feb 9, 2023

Codecov Report

Base: 57.85% // Head: 57.76% // Decreases project coverage by -0.10% ⚠️

Coverage data is based on head (3e8e262) compared to base (b23e9d0).
Patch coverage: 35.71% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##            v2.22    #2260      +/-   ##
==========================================
- Coverage   57.85%   57.76%   -0.10%     
==========================================
  Files        1319     1316       -3     
  Lines       83421    83104     -317     
  Branches    17401    17289     -112     
==========================================
- Hits        48260    48001     -259     
+ Misses      31937    31905      -32     
+ Partials     3224     3198      -26     
Flag Coverage Δ
jsdom 52.72% <35.71%> (+0.02%) ⬆️
node 16.67% <0.00%> (-0.03%) ⬇️

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

Impacted Files Coverage Δ
packages/collaboration/src/common/types.ts 100.00% <ø> (ø)
...collaboration/src/browser/collaboration.service.ts 69.10% <35.71%> (-2.53%) ⬇️
...rib/merge-editor/view/editors/currentCodeEditor.ts 55.93% <0.00%> (-18.21%) ⬇️
.../src/browser/merge-editor/merge-editor.provider.ts 24.00% <0.00%> (-4.58%) ⬇️
packages/electron-basic/src/browser/index.ts 73.78% <0.00%> (-4.25%) ⬇️
...owser/contrib/merge-editor/merge-editor.service.ts 70.43% <0.00%> (-2.72%) ⬇️
...s/terminal-next/src/browser/terminal.controller.ts 33.07% <0.00%> (-1.98%) ⬇️
packages/monaco/src/browser/schema-registry.ts 72.54% <0.00%> (-1.97%) ⬇️
packages/extension/src/hosted/ext.host.ts 78.59% <0.00%> (-1.93%) ⬇️
...es/editor/src/browser/editor-collection.service.ts 65.97% <0.00%> (-0.70%) ⬇️
... and 16 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.

@situ2001
Copy link
Member

situ2001 commented Feb 9, 2023

这条分支上没有这里的commit #2246

需要合并过来才知道影响

@pipiiiiii
Copy link
Contributor Author

这条分支上没有这里的commit #2246

需要合并过来才知道影响

理论上没有影响,相关代码没有变更

Copy link
Member

@situ2001 situ2001 left a comment

Choose a reason for hiding this comment

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

LGTM

@Ricbet
Copy link
Member

Ricbet commented Feb 10, 2023

/publish

@github-actions
Copy link
Contributor

🎉 PR Next version 2.22.2-next-1675996470.0 publish successful! You can install prerelease version via npm install package@2.22.2-next-1675996470.0 @Ricbet

2.22.2-next-1675996470.0

/home/runner/work/_temp/_runner_file_commands/step_summary_3b8e27c8-47db-41f3-b3ef-bcb107ad898d

@Ricbet
Copy link
Member

Ricbet commented Feb 10, 2023

/publish

@github-actions
Copy link
Contributor

🎉 PR Next version 2.22.2-next-1676022353.0 publish successful! You can install prerelease version via npm install package@2.22.2-next-1676022353.0 @Ricbet

2.22.2-next-1676022353.0

/home/runner/work/_temp/_runner_file_commands/step_summary_a0ba745e-bb34-4d5c-9394-a79eadda134f

@Ricbet Ricbet merged commit a5d663c into v2.22 Feb 14, 2023
@Ricbet Ricbet deleted the fix/collaboration-fs branch February 14, 2023 06:42
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.

5 participants