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 bug #613 ":wq command dows not work" #630

Merged
merged 9 commits into from
Aug 28, 2016
Merged

Fix bug #613 ":wq command dows not work" #630

merged 9 commits into from
Aug 28, 2016

Conversation

Platzer
Copy link
Contributor

@Platzer Platzer commented Aug 19, 2016

Bug fix is currently not working with Visual Studio Code - Insiders

Version 1.5.0-insider
Commit 3d9622208b2167921b6664e04fd1fbd51507a4a0

it seems that the isDirty property of the vscode.TextDocument is set false for newly created document. Going to check it tomorrow with normal vscode.

@Platzer
Copy link
Contributor Author

Platzer commented Aug 20, 2016

It was too late yesterday didn't get it how isDirty and isUntitled are ment to be used.
The changes don't include the save as dialog, should we consider a implementation in the WriteCommand?

@johnfn
Copy link
Member

johnfn commented Aug 20, 2016

No save as dialog is required because Vim doesn't use one. :)

Do you think this is good to go, or do you need any help?

@Platzer
Copy link
Contributor Author

Platzer commented Aug 21, 2016

I would like to implement the tests for w, q and wq but i need some time to figure out how the test framework works ;-) and how to implement it best.

@Platzer
Copy link
Contributor Author

Platzer commented Aug 22, 2016

hey guys (@johnfn, @rebornix) i'm not sure if i get how async work (i'm realy new to ts / js programming), could you please have a look if i'm doing something stupid here?

@johnfn
Copy link
Member

johnfn commented Aug 23, 2016

Nothing looks horrifically stupid to me. :) Anything in particular you want us to look at?

@Platzer
Copy link
Contributor Author

Platzer commented Aug 23, 2016

No nothing special, just a bit insecure about the async/await promise stuff ;). I just cleaned up the test code a bit (cut out the ugly polling). I think it's good to go.

@johnfn
Copy link
Member

johnfn commented Aug 23, 2016

:) you aren't the only one, we were having a discussion in slack about exactly that earlier today!

@@ -37,7 +37,7 @@ export class WriteCommand extends node.CommandBase {
return this._arguments;
}

execute(modeHandler : ModeHandler) : void {
async execute(modeHandler : ModeHandler) : Promise<void> {
Copy link
Member

Choose a reason for hiding this comment

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

Actually, this one doesn't need to be async because you don't do any awaiting inside it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i need it to return the encapsulating promise from the activeTextEditor.document.save() call in the save method. otherwise i wasn't able to call write cmd and quit cmd synchronously.

modeHandler.setupStatusBarItem(accessErr.message);
}
try {
fs.accessSync(this.activeTextEditor.document.fileName, fs.W_OK);
Copy link
Member

Choose a reason for hiding this comment

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

Hey can you help me understand why we use sync instead of async methods here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @rebornix, with my restricted type/javascript skills i couldn't return the Save method Promise, and if i'm not able to wait until the editor has saved the file i can not close it with the close-cmd.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not super concerned about blocking during a save... should I be?

@johnfn
Copy link
Member

johnfn commented Aug 26, 2016

Let me know if this one is good to go, @Platzer!

@Platzer
Copy link
Contributor Author

Platzer commented Aug 26, 2016

@johnfn I would say so, but i've got the developer sickness > never believe my code is done (especially on new terrain)!

@johnfn
Copy link
Member

johnfn commented Aug 28, 2016

Looks fine to me - thanks, @Platzer!

@johnfn johnfn merged commit 05c7e67 into VSCodeVim:master Aug 28, 2016
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.

3 participants