Document syncing doesn't work properly in certain batching scenarios

[njx]

In this scenario:

1. Inline editor and main editor are open on the same doc
2. In the inline editor, an operation starts (e.g. because of keydown)
3. During that operation, an edit is made in the underlying doc (== the main editor)

The main editor gets corrupted. This is because the document syncing flag guards rely on all happening within a single operation batch, but in the case above, the outer operation in the inline editor doesn't complete until after all the document syncing flag guards are reset, and it then sends out a change event back to the main editor, doubling the edit.

[njx]

This might be hard to fix without getting a proper model/view separation in CodeMirror.

[peterflynn]

Some more notes:

This is relatively hard to hit because 'operation' batches created by Brackets are almost always in the master editor (via Document.batchOperation()). In this case, CodeMirror has automatically created a batch within the secondary editor, and it's relatively rare that some of our code runs within that window.

The prominent case where we've seen this is keyboard interaction with the code hints popup in an inline editor -- which doesn't happen in vanilla Brackets but could happen if an extension enables code hints in other file types, and/or adds new Quick Open modes.

One reason this is tricky to fix is that we don't know for sure what else is going on within the secondary editor's operation -- there may be other edits that were not related to master->secondary syncing. I think current CodeMirror never does anything to coalesce the separate edits, but it does send a single unified change event containing a list of multiple deltas, potentially only some of which we'd want to ignore.

[peterflynn]

Also: if we want to fix this without going for the full-blown model/view separation, I wonder if the new(ish) CodeMirror getHistory()/setHistory() APIs might let us clean up our syncing architecture in a way that makes this easier to fix...

[pthiess]

Reviewed and assigned to @peterflynn - is this a FBNC?

[njx]

No, it's not fixed. The pull request above is a temporary workaround for this issue.

[pthiess]

Thanks @njx !

[pthiess]

Reviewed.

[pthiess]

We need V3 integrated for this to be fixed.

[peterflynn]

Does v3 contain any changes that we think will help with this?

[njx]

@peterflynn We were thinking we could investigate the getHistory() stuff you mentioned once we've integrated v3. If that doesn't pan out, then we would want to drive the bigger model/view factoring in CM to address it.

[peterflynn]

Oh right, I forgot that we aborted the CM2 merge that would have brought these APIs into master.

[njx]

Hopefully this won't be an issue once we get the doc/view syncing stuff from v3 integrated.

[njx]

I added a link to this bug from the CM doc/view research card so we can check whether it's fixed.

[njx]

Closing since it's now part of the doc-linking card: https://trello.com/c/cRAl0YHB