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

Insert Previous text #768

Merged
merged 5 commits into from
Oct 10, 2016
Merged

Insert Previous text #768

merged 5 commits into from
Oct 10, 2016

Conversation

rebornix
Copy link
Member

Yay! We love PRs! 🎊

Please include a description of your change & check your PR against this list, thanks:

  • Commit message has a short title & issue references
  • Each commit does a logical chunk of work.
  • It builds and tests pass (e.g gulp tslint)

This PR implements ctrl-A and ctrl-@ in Insert Mode, which inserts previously change. Instead of maintaining the change in history ourselves, I'm currently just leveraging documentContentChangeEvent. This way when we type ab and intellisense helps us autocomplete it to abstract, we can get the whole abstract through contentChange, not just ab.

@johnfn @xconverge this is totally a different way of handing last change which we currently maintained in HistorySteps, so for now I just put the plain array in HistoryTracker. Please take a look and see if it can inspire you on how to handle history.

The only catch here is there might be racing condition between documentChange and type. So basically if we write test cases for this feature and run case automatically, or a human types real quick, you might see bugs as ctrl-A may come before all documentChange.

@rebornix rebornix force-pushed the PreviousText branch 2 times, most recently from fcacd4b to 1a738d7 Compare September 14, 2016 17:41
Copy link
Member

@johnfn johnfn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, the advantages to using the document change event are huge! But I don't want to use it in half of our code and not use it in the other half; I think that would make the code base more confusing.

Do you want to refactor the history tracker? If not, I can add that to my TODO list :P

@rebornix
Copy link
Member Author

@johnfn Agree, personally I want to do the refactor as well since with content change, the tab complete issue we mentioned in microsoft/vscode#1431 and microsoft/vscode#10801 will be gone. Besides, we can directly operate on content changes instead of calculating all the key strokes. We don't need Code's customization on Undo Stop and history API as we can merge content change and pick what we need by will.

The only problem right now is the racing condition between handlers for docuementTextContentChange and type, like when we type ab<Tab> then c, we can't make sure when we handle c, we've already received all content change made by ab<Tab>. But this issue is already addressed and let's see what we can do these few days.

Copy link
Member

@johnfn johnfn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, didn't mean to approve.

@johnfn
Copy link
Member

johnfn commented Sep 15, 2016

Okay! The history tracker refactor will be a very significant change; I definitely encourage you to start a new PR on that one.

Honestly, I think it could actually be quite difficult for you to do since you didn't implement it in the first place, so no shame if you just want to wait for me to get to it!

@rebornix
Copy link
Member Author

I'd like to have this merged to avoid additional manual merging (like cursor related change) then see how to refactor History Tracker.

Right now I have 5 test cases to cover this feature but that's not enough as Code Snippet has some issues with cursor position yet, and auto-complete is not testable.

@rebornix rebornix merged commit 25b0a34 into VSCodeVim:master Oct 10, 2016
@rebornix
Copy link
Member Author

Have it merged first, there might be following code reviews or suggestions and let's see.

@rebornix rebornix deleted the PreviousText branch October 10, 2016 15:12
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