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

Keep undo history even when file changes outside Code. #29655

Merged
merged 5 commits into from Aug 28, 2017

Conversation

stringham
Copy link
Contributor

@stringham stringham commented Jun 27, 2017

Implements #2908

@mention-bot
Copy link

@stringham, thanks for your PR! By analyzing the history of the files in this pull request, we identified @alexandrudima and @egamma to be potential reviewers.

@msftclas
Copy link

@stringham,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

@stringham stringham force-pushed the update-on-file-reload branch 3 times, most recently from 13f3cf5 to f92c844 Compare June 27, 2017 21:46
@stringham stringham force-pushed the update-on-file-reload branch 2 times, most recently from 87c0ac4 to 308df72 Compare July 16, 2017 16:20
@stringham
Copy link
Contributor Author

@alexandrudima are there plans to look at this?

@BridgeAR
Copy link

It would be great to get this in. There are quite some people waiting on this feature.

@alexdima
Copy link
Member

alexdima commented Aug 28, 2017

Sorry for not replying, I've looked at this multiple times, but I still need to figure out what's the best way to approach the problem.

The fundamental flaw is the implementation of a O(N^2) algorithm to find a LCS between the old model lines and the new model lines. This has two problems:

  • the algorithm is O(N^2), where N is the count of lines. In real-life, N can be in the millions range, which is pretty bad.
  • we already have an implementation for a superior runtime diffing algorithm in vs/base/common/diff/diff.ts -- "An O(ND) Difference Algorithm and its letiations" by Eugene W. Myers

I'm not comfortable to run either of the algorithms on the main thread, as this could take ... minutes (during which time the main thread is blocked). I'm also not comfortable to delegate the computation of the diff to a web worker thread, as that will mean twice the memory consumption and ... it will still potentially take minutes to show the changed file on disk.

I'll think today of a simpler heuristic to capture the log file scenario: e.g. a 200MB file with millions of lines to which a line is appended every N seconds. I'd like for us to at least cover that case.... The root problem here is memory consumption, if we keep 200MB in the undo stack for every file change on disk we're gonna run out of memory pretty quickly. If we compute a diff which takes minutes and the file changes on disk every N seconds, we're never gonna catch up with the latest content, as we'll always be computing diffs....

TL;DR: I'll reserve time today to figure out what's best to be done here.

@alexdima
Copy link
Member

I don't see a way to get around running the diff algorithm on the main thread, so I've simply replaced the usage of the custom LCS implementation with our existing algorithm.

@alexdima
Copy link
Member

Thank you @stringham for your contribution ❤️.

If all goes well with testing, this will make it into our August release!

@alexdima alexdima added this to the August 2017 milestone Aug 28, 2017
@alexdima alexdima merged commit 504898d into microsoft:master Aug 28, 2017
@@ -413,7 +413,6 @@ export class TextFileEditorModel extends BaseTextEditorModel implements ITextFil
this.blockModelContentChange = true;
try {
this.updateTextEditorModel(value);
this.setDirty(false);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexandrudima I'm pretty sure this change was necessary. We need the model to know that this is now the current saved state.

If after reloading the file on disk, if you press undo it shows that the file is still saved, after a redo it shows the file is unsaved, and if you attempt to close the file it prompts you to save it, even though it is up to date.

Here's a gif where I modify the file with vim, press undo, then redo:
dirty

Copy link
Member

Choose a reason for hiding this comment

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

@stringham thanks we are looking into restoring that, you are right, it is needed.

Copy link
Member

Choose a reason for hiding this comment

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

@stringham 👍 thank you ❤️ !

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants