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

Prevent "too many nested groupings" xcode crash on undo #951

Closed

Conversation

jasonmay
Copy link

@jasonmay jasonmay commented May 8, 2016

When 954f200 was introduced, I don't think it accounted for the fact that the undo handlers would get triggers for r as well as R, which I believe resulted in #817 being reported.

I unfortunately was not able to test. I added set debug to my config, restarted Xcode many times, and looked all over the menu for XVim -> Run Test but didn't see anything. I'd be more than happy if someone who can run the tests could piggyback on this PR and write a test and run it on my behalf.

Thanks!

@squarefrog
Copy link
Contributor

The test menu can be found in Editor > XVim > Run Test

Turns out if it's not a printable key, it doesn't trigger the handler for grouping.
@jasonmay
Copy link
Author

Thanks @squarefrog. Test has been added. That test fails with "exception raised" without the fix.

@jasonmay
Copy link
Author

To expand on the new change, the test showed that it works just fine for any type of replace. I was throwing the baby out with the bathwater with my initial fix, but it's only for newlines that it crashed on, so I handle that specific case instead.

@squarefrog
Copy link
Contributor

👍 glad you were able to narrow down the fix!

@jasonmay
Copy link
Author

I am cherry-picking changes from #770 and adding tests (and fixing the edge cases mentioned). To save on hassle, how do I get the tests to see the updated code without having to restart Xcode every time?

@jasonmay
Copy link
Author

jasonmay commented Jun 1, 2016

Closing in favor of #960

@jasonmay jasonmay closed this Jun 1, 2016
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.

2 participants