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

Throttle save participants when low auto save delay is configured to allow for undo #102542

Closed
rebornix opened this issue Jul 14, 2020 · 7 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug verified Verification succeeded workbench-editors Managing of editor widgets in workbench window
Milestone

Comments

@rebornix
Copy link
Member

  • VSCode Version: 1.48-Insiders
  • OS Version: macOS

Steps to Reproduce:

  1. Turn on files.insertFinalNewline
  2. Open a file which has more than 1 line
  3. Select all, type a
  4. Try undo, it will never bring back the original data

Ran into this issue twice today when I was typing really fast and replaced several lines with random characters by mistake and then lost all my work.

Does this issue occur when all extensions are disabled?: Yes

@rebornix rebornix added bug Issue identified by VS Code Team member as probable bug undo-redo important Issue identified as high-priority labels Jul 14, 2020
@rebornix rebornix added this to the July 2020 milestone Jul 14, 2020
@jrieken
Copy link
Member

jrieken commented Jul 15, 2020

Doesn't reproduce for me...

Jul-15-2020 11-03-57

@jrieken jrieken added the editor-core Editor basic functionality label Jul 15, 2020
@rebornix
Copy link
Member Author

I have auto save on and its delay is 100ms, even though I tried to press CMD+Z real fast, it still

  • Undo the insertFinalNewline
  • Document saved
  • insertFinalNewline is executed again
  • Insert another insertFinalNewline into the Undo stack

Then I got stuck. Workaround for me is slowing down the auto save but it's still bad.

@rebornix rebornix assigned bpasero and unassigned jrieken Jul 15, 2020
@bpasero
Copy link
Member

bpasero commented Jul 15, 2020

I can reproduce but I cannot see how to fix this easily given autosave is at 100ms? Here is what seems to happen:

  • you undo once
  • the final new line is removed
  • auto save kicks in and adds the final new line
  • you undo again
  • the final new line is removed
  • ...and so on...

You could only go back if you managed to undo faster than auto save kicking in, right?

One possible solution maybe on the editor would be to merge the undo stack into one, e.g. the change the user did and the result of the save participants become one operation? Though that might cause a lot of issues when people want to undo the result of a formatter...

@jrieken
Copy link
Member

jrieken commented Jul 15, 2020

Though that might cause a lot of issues when people want to undo the result of a formatter...

Yeah, we code in place to make sure that automagically made edits are in one undo stack which is separate from manual edits.

Wouldn't be a simple fix to put a lower-cap on the auto save timeout? For instance it doesn't get faster than 700ms or so? It's like the font size which doesn't go below 6

@bpasero
Copy link
Member

bpasero commented Jul 16, 2020

Yeah we can put a cap as last resort, but then again maybe people really rely on save kicking in instantly. Two more ideas:

Cap auto save when undoing
Essentially, when you are undoing, auto-save will run only at 1s minimum to allow the user to undo.

Do not run save participants when undoing
Do not run any save participants when you are undoing. In other words, as long as the version of the model is from a state in the past, no participants would trigger until you type again. Could be a breaking change in behaviour though because it allows to get the model into a state where save participants did not have a chance to run, so it is very dangerous.

@bpasero
Copy link
Member

bpasero commented Jul 16, 2020

Pushed the following fix:

  • remember when the model content was changed from a undo/redo operation (that information is carried via model content change event)
  • measure the time it took from such an operation until auto save triggers (only auto save, explicit save will not be throttled ever)
  • if that time is below 500ms, delay the save operation until 500ms is reached

This should give a user a chance to trigger the undo command repeatedly, provided the user can do that multiple times below 500ms each.

@bpasero bpasero closed this as completed Jul 16, 2020
@bpasero bpasero added workbench-editors Managing of editor widgets in workbench window and removed editor-core Editor basic functionality important Issue identified as high-priority undo-redo labels Jul 16, 2020
@bpasero bpasero changed the title Editor data loss when files.insertFinalNewline is turned on Throttle save participants when low auto save delay is configured to allow for undo Jul 16, 2020
@rebornix
Copy link
Member Author

@bpasero thanks for the quick turnaround! I set the delay to a low number as I wish to see errors/warnings from build task as soon as possible, the solution is reasonable as whenever I found cmd+z not working, I try to cmd+z a couple of times more so the 500ms delay can solve this problem.

@connor4312 connor4312 added the verified Verification succeeded label Aug 6, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Aug 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug verified Verification succeeded workbench-editors Managing of editor widgets in workbench window
Projects
None yet
Development

No branches or pull requests

5 participants