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

Replace onDidChange with an enhanced version of onDidChangeText #273

Merged
merged 3 commits into from Oct 26, 2017

Conversation

Projects
None yet
2 participants
@maxbrunsfeld
Contributor

maxbrunsfeld commented Oct 26, 2017

Background

The TextBuffer.onDidChange method has long been a source of performance problems because it causes its callback to be called for each individual change that happens to a TextBuffer. This behavior makes it very expensive to make a series of small changes to a buffer with any change observers. This is important for multi-cursor editing, and it also has shaped other decisions about how to mutate buffers, encouraging preprocessing strings before inserting them into text buffers rather doing the preprocessing in the buffer itself.

A while ago, we added a more efficient way of observing changes to buffers - TextBuffer.onDidChangeText. Callbacks passed to this method get called only once per transaction, and are passed an array of the consolidated changes that occurred in that transaction.

Solution

This PR unifies onDidChange and onDidChangeText; they are now identical. Listeners get called once per transaction.

I'm ensuring backwards-compatibility for existing users of onDidChange by synthesizing the old event properties oldRange, newRange, oldText and newText. For example, if two separate changes occur in a transaction, oldRange will be the smallest possible Range that encompasses both of the changes.

The oldText and newText properties are now deprecated. Callers who are interested in the text that was inserted or removed should use the changes property to examine individual changes.

/cc @nathansobo

@emitter.on 'did-change-text', callback
# Public: This is now identical to {onDidChange}.
onDidChangeText: (callback) -> @onDidChange(callback)

This comment has been minimized.

@nathansobo

nathansobo Oct 26, 2017

Contributor

I guess it's not worth the hassle of deprecating it? Probably not.

@nathansobo

nathansobo Oct 26, 2017

Contributor

I guess it's not worth the hassle of deprecating it? Probably not.

This comment has been minimized.

@maxbrunsfeld

maxbrunsfeld Oct 26, 2017

Contributor

I think we should, but I thought we should do that in a second pass, after we remove all of our own uses of it.

@maxbrunsfeld

maxbrunsfeld Oct 26, 2017

Contributor

I think we should, but I thought we should do that in a second pass, after we remove all of our own uses of it.

This comment has been minimized.

@maxbrunsfeld

maxbrunsfeld Oct 26, 2017

Contributor

Same with oldText and newText. I want to deprecate them soon.

@maxbrunsfeld

maxbrunsfeld Oct 26, 2017

Contributor

Same with oldText and newText. I want to deprecate them soon.

This comment has been minimized.

@nathansobo

nathansobo Oct 26, 2017

Contributor

👍

@nathansobo

nathansobo Oct 26, 2017

Contributor

👍

@maxbrunsfeld maxbrunsfeld merged commit bea60c0 into master Oct 26, 2017

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@maxbrunsfeld maxbrunsfeld deleted the mb-one-did-change-event-per-tx branch Oct 26, 2017

This was referenced Oct 30, 2017

This was referenced Oct 31, 2017

Evpok added a commit to Evpok/latex-autocomplete that referenced this pull request Nov 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment