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

Notebooks emit a content change event when switching profiles #180823

Closed
alexr00 opened this issue Apr 25, 2023 · 6 comments
Closed

Notebooks emit a content change event when switching profiles #180823

alexr00 opened this issue Apr 25, 2023 · 6 comments
Assignees
Labels
debt Code quality issues notebook
Milestone

Comments

@alexr00
Copy link
Member

alexr00 commented Apr 25, 2023

Testing #180767

After the profile switching, reload the window immediately, test if the warning/error actions on the editor view gives you clear guidance

Before the reload occurred, this error blocked it:

image

@bpasero
Copy link
Member

bpasero commented Apr 25, 2023

This can happen if a change to the notebook and a change of the profile happened fast enough that we did not have a chance to complete the backup of the notebook to disk. In that case there is really nothing we can do to prevent data loss on shutdown and have to prevent it from happening.

To clarify, is that what was happening here? Very fast profile switch after making a notebook dirty?

@bpasero bpasero added info-needed Issue requires more information from poster notebook labels Apr 25, 2023
@bpasero bpasero added this to the April 2023 milestone Apr 25, 2023
@bpasero bpasero added debt Code quality issues and removed info-needed Issue requires more information from poster labels Apr 26, 2023
@bpasero bpasero modified the milestones: April 2023, May 2023 Apr 26, 2023
@bpasero
Copy link
Member

bpasero commented Apr 26, 2023

I can reproduce easily.

@rebornix this is maybe notebooks could still improve next month. When I debug this I see an issue that makes the backup service bring up a dialog like this. Let me clarify what happens.

Normally, the backup service would not complain about a missing backup, when a backup exists that matches the content version of the notebook (which we track internally). Here, a backup actually exists for that content and thus I was wondering why the dialog appears anyway.

Looks like, when I change profiles, the notebook component emits a content change event which will trigger a call to backup:

backup (storedFileWorkingCopy.ts:767)
(anonymous) (workingCopyBackupTracker.ts:191)
setTimeout (async)
y (workingCopyBackupTracker.ts:181)
w (workingCopyBackupTracker.ts:167)
(anonymous) (workingCopyBackupTracker.ts:56)
invoke (event.ts:862)
deliver (event.ts:1075)
fire (event.ts:1031)
(anonymous) (workingCopyService.ts:177)
invoke (event.ts:862)
deliver (event.ts:1075)
fire (event.ts:1031)
ab (storedFileWorkingCopy.ts:727)
(anonymous) (storedFileWorkingCopy.ts:677)
invoke (event.ts:862)
deliver (event.ts:1075)
fire (event.ts:1031)
(anonymous) (notebookEditorModel.ts:196)
invoke (event.ts:862)
deliver (event.ts:1075)
fire (event.ts:1031)
(anonymous) (notebookTextModel.ts:260)
invoke (event.ts:862)
deliver (event.ts:1075)
fire (event.ts:1031)
fire (event.ts:1209)
C (notebookTextModel.ts:325)
(anonymous) (notebookTextModel.ts:293)
invoke (event.ts:862)
deliver (event.ts:1075)
fire (event.ts:1031)
set language (notebookCellTextModel.ts:106)
(anonymous) (notebookCellTextModel.ts:178)
invoke (event.ts:862)
deliver (event.ts:1075)
fire (event.ts:1031)
setLanguageId (tokenizationTextModelPart.ts:326)
Ab (textModel.ts:1938)
(anonymous) (textModel.ts:322)
invoke (event.ts:862)
deliver (event.ts:1075)
fire (event.ts:1031)
f (languageService.ts:198)
(anonymous) (languageService.ts:179)
invoke (event.ts:862)
deliver (event.ts:1075)
fire (event.ts:1031)
(anonymous) (languageService.ts:40)
invoke (event.ts:862)
deliver (event.ts:1075)
fire (event.ts:1031)
_registerLanguages (languagesRegistry.ts:151)
l (languagesRegistry.ts:119)
setDynamicLanguages (languagesRegistry.ts:108)
(anonymous) (languageService.ts:167)
d (extensionsRegistry.ts:144)
acceptUsers (extensionsRegistry.ts:135)
zb (abstractExtensionService.ts:948)
wb (abstractExtensionService.ts:878)
Z (abstractExtensionService.ts:273)
Y (abstractExtensionService.ts:203)
await in Y (async)
(anonymous) (abstractExtensionService.ts:137)
invoke (event.ts:862)
deliver (event.ts:1075)
fire (event.ts:1031)
(anonymous) (event.ts:1317)
invoke (event.ts:862)
deliver (event.ts:1075)
fire (event.ts:1031)
(anonymous) (event.ts:118)
invoke (event.ts:862)
deliver (event.ts:1075)
fire (event.ts:1031)
v (extensionManagementChannelClient.ts:97)
await in v (async)
(anonymous) (extensionManagementChannelClient.ts:28)
invoke (event.ts:862)
deliver (event.ts:1075)
fire (event.ts:1031)
updateCurrentProfile (userDataProfileService.ts:48)
n (userDataProfileManagement.ts:124)
switchProfile (userDataProfileManagement.ts:112)
await in switchProfile (async)
run (userDataProfile.ts:170)
handler (actions.ts:568)
invokeFunction (instantiationService.ts:68)
k (commandService.ts:95)
executeCommand (commandService.ts:60)
run (actions.ts:488)
k (actions.ts:194)
run (actions.ts:185)
l (contextmenuService.ts:251)
click (contextmenuService.ts:225)
o (contextmenu.ts:18)
onceWrapper (VM91 sandbox_bundle:2)
emit (VM91 sandbox_bundle:2)
onMessage (VM91 sandbox_bundle:2)

I think originating from a setLanguageId call for a cell.

Given this content change event, the working copy backup handler will try to perform another backup, even though the content did not change. And since that fails (because in the new profile there is no notebook extension installed), the backup handler will complain when you try to exit or reload.

Is it possible to not emit the content change event here when profiles change?

@bpasero bpasero removed their assignment Apr 26, 2023
@bpasero bpasero changed the title Modal error when reloading with dirty notebook Notebooks emit a content change event when switching profiles Apr 26, 2023
@bpasero
Copy link
Member

bpasero commented Apr 26, 2023

I have updated the title, I think the gist is that there is a content change event when you switch to a profile that does not have the NB extension installed and that causes all kinds of issues (including #180822)

@rebornix
Copy link
Member

@bpasero thanks for the analysis!

I think originating from a setLanguageId call for a cell.

This is a bit unexpected and should be fixed. At the same time, I agree that we shouldn't avoid content change when switching profiles.

@rebornix
Copy link
Member

rebornix commented May 2, 2023

This is an interesting issue. After switching to a new profile, which lacks of the GitHub issue notebook extension, we can't set a proper language on the cell anymore. The code cell used to have language "GitHub-issues" but now it's set to plain text as a fallback, Thus leading to the language update and triggering backup.

It's not trivial to fix and once we have #180514, this should not happen. Let's track it over there.

@rebornix rebornix closed this as completed May 2, 2023
@bpasero
Copy link
Member

bpasero commented May 3, 2023

@rebornix but why is a language change considered a content change? is the language part of the backup?

@github-actions github-actions bot locked and limited conversation to collaborators Jun 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debt Code quality issues notebook
Projects
None yet
Development

No branches or pull requests

3 participants