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

clear history when content from disk is changed #703

Merged
merged 5 commits into from
Sep 3, 2016
Merged

clear history when content from disk is changed #703

merged 5 commits into from
Sep 3, 2016

Conversation

aminroosta
Copy link
Contributor

@johnfn Fix for #680 !
Let me know if you need something to change :-)

vscode.workspace.onDidChangeTextDocument((event) => {
/* TODO: Remove setTimeout (https://github.com/Microsoft/vscode/issues/11339) */
setTimeout(() => {
if (event.document.isDirty === false) {
Copy link
Member

Choose a reason for hiding this comment

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

I suppose you are typing too fast to check a boolean through === :)

@@ -114,6 +115,19 @@ export async function activate(context: vscode.ExtensionContext) {

vscode.window.onDidChangeActiveTextEditor(handleActiveEditorChange, this);

vscode.workspace.onDidChangeTextDocument((event) => {
/**
* Change from vscode eidtor should set document.isDirty to true but they initially don't!
Copy link
Member

Choose a reason for hiding this comment

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

Fix typo eidtor => editor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@johnfn Fixed

@aminroosta
Copy link
Contributor Author

aminroosta commented Sep 2, 2016

Any reason why git commit --amend -m "new message" create a new commit?!

Update: It was a github delay

@johnfn
Copy link
Member

johnfn commented Sep 2, 2016

Generally speaking it shouldn't! You might get yourself into trouble if you've already pushed to a remote though.

@aminroosta
Copy link
Contributor Author

I know :-)

@aminroosta
Copy link
Contributor Author

@johnfn 1 test failed related to clipboard registers which i have already fixed in implement ; and ,

@@ -217,6 +231,13 @@ async function handleKeyEvent(key: string): Promise<void> {
});
}

function handleContentChangedFromDisk(document : vscode.TextDocument) : void {
_.filter(modeHandlerToEditorIdentity, (modeHandler) => modeHandler.fileName === document.fileName)
.forEach((modeHandler) => {
Copy link
Member

Choose a reason for hiding this comment

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

Same thing about the brackets here :)

Copy link
Contributor Author

@aminroosta aminroosta Sep 2, 2016

Choose a reason for hiding this comment

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

What about them? @jpoon

Copy link
Member

Choose a reason for hiding this comment

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

That you can remove the brackets around (modeHandler)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

screen shot 2016-09-03 at 3 44 12 am

@jpoon I saw other places are always using brackets even for a single argument, that's why i added those brackets 😄

@aminroosta
Copy link
Contributor Author

Travis Ci is not friendly with clipboard tests, specially in mac builds.

@jpoon
Copy link
Member

jpoon commented Sep 2, 2016

I just restarted the build.

@johnfn
Copy link
Member

johnfn commented Sep 3, 2016

LGTM, thanks @aminroosta!

@johnfn johnfn merged commit 660df9a into VSCodeVim:master Sep 3, 2016
@rebornix
Copy link
Member

rebornix commented Sep 5, 2016

This commit is amazing, now undo is trustworthy!

This pull request was closed.
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.

4 participants