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

[CLOSED] Document syncing doesn't work properly in certain batching scenarios #1645

Open
core-ai-bot opened this issue Aug 29, 2021 · 14 comments

Comments

@core-ai-bot
Copy link
Member

Issue by njx
Tuesday Sep 18, 2012 at 23:59 GMT
Originally opened as adobe/brackets#1688


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.

@core-ai-bot
Copy link
Member Author

Comment by njx
Wednesday Sep 19, 2012 at 00:00 GMT


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

@core-ai-bot
Copy link
Member Author

Comment by peterflynn
Wednesday Sep 19, 2012 at 00:18 GMT


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.

@core-ai-bot
Copy link
Member Author

Comment by peterflynn
Wednesday Sep 19, 2012 at 00:20 GMT


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...

@core-ai-bot
Copy link
Member Author

Comment by pthiess
Thursday Sep 20, 2012 at 17:30 GMT


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

@core-ai-bot
Copy link
Member Author

Comment by njx
Thursday Sep 20, 2012 at 17:37 GMT


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

@core-ai-bot
Copy link
Member Author

Comment by pthiess
Thursday Sep 20, 2012 at 17:55 GMT


Thanks@njx !

@core-ai-bot
Copy link
Member Author

Comment by pthiess
Wednesday Sep 26, 2012 at 22:19 GMT


Reviewed.

@core-ai-bot
Copy link
Member Author

Comment by pthiess
Monday Oct 22, 2012 at 19:00 GMT


We need V3 integrated for this to be fixed.

@core-ai-bot
Copy link
Member Author

Comment by peterflynn
Monday Oct 22, 2012 at 19:32 GMT


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

@core-ai-bot
Copy link
Member Author

Comment by njx
Monday Oct 22, 2012 at 19:40 GMT


@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.

@core-ai-bot
Copy link
Member Author

Comment by peterflynn
Monday Oct 22, 2012 at 19:54 GMT


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

@core-ai-bot
Copy link
Member Author

Comment by njx
Saturday Feb 02, 2013 at 17:26 GMT


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

@core-ai-bot
Copy link
Member Author

Comment by njx
Saturday Feb 02, 2013 at 17:31 GMT


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

@core-ai-bot
Copy link
Member Author

Comment by njx
Wednesday Mar 06, 2013 at 19:34 GMT


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

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

No branches or pull requests

1 participant