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

Workaround surround bug #2830

Merged
merged 2 commits into from
Jul 20, 2018
Merged

Workaround surround bug #2830

merged 2 commits into from
Jul 20, 2018

Conversation

reujab
Copy link
Contributor

@reujab reujab commented Jul 11, 2018

What this PR does / why we need it:
When deleting surroundings using ds, this plugin doesn't recognize it as being handled, so the command list remains what it was before surround.ts handled it. This pull request fixes this by clearing the command list when a surround command finishes.

screencast

Which issue(s) this PR fixes
N/A

Special notes for your reviewer:
A better way to fix this bug would be to somehow have this line of code executed. However, I wasn't able to figure out how, so I came up with this workaround.

@xconverge
Copy link
Member

I think that there is a cleaner way to do it

If you have Finish() in surround.ts return true

We can then do something like this anytime we call TryToExecuteSurround():

    if (await CommandSurroundAddToReplacement.TryToExecuteSurround(vimState, position)) {
      this.isCompleteAction = true;
    }

@xconverge
Copy link
Member

there is also a vimState.recordedState.hasRunSurround that can be used in modeHandler perhaps

@xconverge
Copy link
Member

Do you think that you will get a chance to comment on my feedback in the near future?

@reujab
Copy link
Contributor Author

reujab commented Jul 20, 2018

Your patch is certainly better than mine, and it works.

I'm going to try to read more into the code and get a better understanding of this plugin so I can hopefully fix a couple other things I've encountered.

@xconverge
Copy link
Member

I would highly recommend fixing things that will be convenient for YOU! Thanks for coming back to this, let me know if you have any questions or want any pointers on where to start with an issue that annoys you!

@xconverge xconverge merged commit 1a5f358 into VSCodeVim:master Jul 20, 2018
@reujab reujab deleted the surround-workaround branch July 20, 2018 05:32
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