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

Added: : to start Command Palette Support #199

Merged
merged 5 commits into from
May 23, 2018

Conversation

Molunerfinn
Copy link
Contributor

@Molunerfinn Molunerfinn commented May 18, 2018

After reading the source code of the project and the VSCodeVim, I try to add : to start Command Palette Support.

Now I have finished some of the vim commands in Command Palette:

const commandParsers = {
    w: WriteCommand,
    write: WriteCommand,
    wa: WallCommand,
    wall: WallCommand,

    q: QuitCommand,
    quit: QuitCommand,
    qa: QuitAllCommand,
    qall: QuitAllCommand,

    wq: WriteQuitCommand,
    x: WriteQuitCommand,

    wqa: WriteQuitAllCommand,
    wqall: WriteQuitAllCommand,
    xa: WriteQuitAllCommand,
    xall: WriteQuitAllCommand,

    vs: VisualSplitCommand,
    vsp: VisualSplitCommand,

    new: NewFileCommand,
    vne: VerticalNewFileCommand,
    vnew: VerticalNewFileCommand
};

Here are some screenshots:

  1. in normal mode type : :
    tostartpalette
  2. :wq
    wq
  3. :vs
    vs
  4. :new
    new
  5. of course, go to line is also supported:
    gotoline

I hope that my contribution can help you! If you think it can be merged, you can remove the line in your readme:

image

Thank you for your nice project!

@Molunerfinn Molunerfinn changed the title Added: : to started Command Palette Support Added: : to start Command Palette Support May 19, 2018
Copy link
Owner

@aioutecism aioutecism left a comment

Choose a reason for hiding this comment

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

Cloud you lint the code with tslint?
I'm noticing a lot missing semicolons. They won't hurt much but let's follow the good practice.

return commands.executeCommand('workbench.action.gotoLine');
static async command(): Promise<void> {
// this is undefined
return await CommandLine.PromptAndRun();
Copy link
Owner

Choose a reason for hiding this comment

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

This seems redundant.

Shall we move src/CommandLine/ to src/Actions/CommandLine/ and use it directly like this?

{ keys: ':', actions: [ActionCommandLine.prompt] }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes we can

if (!vscode.window.activeTextEditor) {
return
}
let text = ''
Copy link
Owner

Choose a reason for hiding this comment

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

The variable is never used.

}
return await CommandLine.Run(cmd);
} catch (e) {
console.log(e)
Copy link
Owner

Choose a reason for hiding this comment

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

Shall we use console.error to indicate that something went wrong?

@@ -210,7 +210,8 @@ export class ModeNormal extends Mode {
{ keys: 'z M', actions: [ActionFold.foldAll] },
{ keys: 'z R', actions: [ActionFold.unfoldAll] },

{ keys: ':', actions: [ActionCommand.goToLine] },
// { keys: ':', actions: [ActionCommand.goToLine] },
Copy link
Owner

Choose a reason for hiding this comment

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

We can just delete lines we don't need in the future.

@@ -0,0 +1,4 @@
export abstract class CommandBase {
protected name: string;
Copy link
Owner

Choose a reason for hiding this comment

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

I don't see the use of name anywhere in this Pull Request.
Is it necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not necessary 😂

qall: QuitAllCommand,

wq: WriteQuitCommand,
writequit: WriteQuitCommand,
Copy link
Owner

Choose a reason for hiding this comment

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

writequit is not a Vim command.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, my fault

Copy link
Owner

Choose a reason for hiding this comment

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

Don't mind. I like the idea though 😄.

vs: VisualSplitCommand,
vsp: VisualSplitCommand,
sp: VisualSplitCommand,
split: VisualSplitCommand,
Copy link
Owner

Choose a reason for hiding this comment

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

sp and split are for the horizontal split. VSCode doesn't support this right now.

Copy link

Choose a reason for hiding this comment

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

VSCode should support horizontal split now

sp: VisualSplitCommand,
split: VisualSplitCommand,
vsplit: VisualSplitCommand,
ne: NewFileCommand,
Copy link
Owner

Choose a reason for hiding this comment

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

ne is for editing next file.

ne: NewFileCommand,
vne: NewFileCommand,
new: NewFileCommand,
vnew: NewFileCommand
Copy link
Owner

Choose a reason for hiding this comment

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

vne and vnew should create the new file in a vertically split pane.

export function parser(input: string): CommandBase | undefined {
if (commandParsers[input]) {
return commandParsers[input]
} else if (isNumber(input)) {
Copy link
Owner

Choose a reason for hiding this comment

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

I think we should use Number.isInteger 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.

The type of input is string, so if you just use Number.isInteger(input) will get false. While I can add it in the isNumber function to detect if the input is an interger

Copy link
Owner

Choose a reason for hiding this comment

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

Yes!

Copy link
Owner

@aioutecism aioutecism left a comment

Choose a reason for hiding this comment

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

Cloud you lint the code with tslint?
I'm noticing a lot missing semicolons. They won't hurt much but let's follow the good practice.

@Molunerfinn
Copy link
Contributor Author

Thanks for the reply! I'll fix them ASAP.

@Molunerfinn
Copy link
Contributor Author

All the new files have been tested by tslint 😁~

const lineNumber = Number(line);
let editor = vscode.window.activeTextEditor;
if (editor) {
ActionMoveCursor.byMotions({
Copy link
Owner

Choose a reason for hiding this comment

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

We should await this action.

import * as vscode from 'vscode';
import { CommandBase } from './Base';

class QuitCommand extends CommandBase {
Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't this be QuitAllCommand?

async execute(): Promise<void> {
await VisualSplit.execute();
await NewFile.execute();
await vscode.commands.executeCommand('workbench.action.closeOtherEditors');
Copy link
Owner

Choose a reason for hiding this comment

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

This may have unwanted effects when we execute the command in the third column.
We can use workbench.action.files.newUntitledFile then workbench.action.moveEditorToNextGroup.

import WriteAllCommand from './WriteAll';
import QuitAllCommand from './QuitAll';

class WriteQuitCommand extends CommandBase {
Copy link
Owner

Choose a reason for hiding this comment

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

And this should be WriteQuitAllCommand.

export function parser(input: string): CommandBase | undefined {
if (commandParsers[input]) {
return commandParsers[input];
} else if (isNumber(input)) {
Copy link
Owner

Choose a reason for hiding this comment

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

Since this is a simple expression, we cloud use Number.isInteger(Number(input)) directly.

Copy link
Owner

@aioutecism aioutecism left a comment

Choose a reason for hiding this comment

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

Thank you for the fixes!

@Molunerfinn
Copy link
Contributor Author

I can fix above now, just wait for a moment~

@Molunerfinn
Copy link
Contributor Author

Done 😀

@aioutecism aioutecism merged commit 8173e96 into aioutecism:master May 23, 2018
@aioutecism
Copy link
Owner

aioutecism commented May 23, 2018

Thank you for your contribution!
I'll do some cleanup and then publish a new version soon.

@Molunerfinn
Copy link
Contributor Author

Can't wait for it! Thank you for your nice job!

@Molunerfinn
Copy link
Contributor Author

@aioutecism I think you can change the README:

👍

@aioutecism
Copy link
Owner

Published 1.26.1.
Readme is updated too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants