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

fix #3157: register single onDidChangeTextDocument handler and delegate to appropriate mode handler #3408

Merged
merged 4 commits into from
Jan 23, 2019

Conversation

jpoon
Copy link
Member

@jpoon jpoon commented Jan 23, 2019

What this PR does / why we need it:
This seemed to do the trick to fix #3157. Instead of having an event handler for each modehandler (ie. text editor), declared a single event handler and delegated the event to the right mode handler. This should have the added benefit of improving perf.

To be perfectly honest however, I'm not entirely sure how this fixed the issue as the same callback code to update the position was still being called, albeit unnecessarily for all the other open text editors.

This PR also includes a minor refactor to add a wrapper around registering event handlers to ensure that we are disposing the events properly.

Which issue(s) this PR fixes

#3157

Special notes for your reviewer:

@jpoon jpoon changed the title Fix3157 fix #3157: register single onDidChangeTextDocument handler and delegate to appropriate mode handler Jan 23, 2019
@jpoon jpoon merged commit 3bb8ea9 into master Jan 23, 2019
@jpoon jpoon deleted the fix3157 branch January 23, 2019 21:36
@jpoon
Copy link
Member Author

jpoon commented Jan 23, 2019

To be perfectly honest however, I'm not entirely sure how this fixed the issue as the same callback code to update the position was still being called, albeit unnecessarily for all the other open text editors.

Yep, this assessment was correct. This PR doesn't actually fix the issue. Turns out I was using the wrong steps to repro. Once you exit and re-enter insert mode, the issue goes away.

Keeping this PR though as I think it's still valid to just have one event handler instead of one per mode handler.

@jpoon jpoon mentioned this pull request Jan 23, 2019
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

Successfully merging this pull request may close these issues.

Cursor in different spot than where cursor appears and text gets deleted in different location
1 participant