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

Implement "q:" command #2618

Merged
merged 10 commits into from
May 11, 2018
2 changes: 2 additions & 0 deletions extension.ts
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,8 @@ export async function activate(context: vscode.ExtensionContext) {
}
}

CommandLine.SetHistoryDirPath(context.extensionPath);
Copy link
Member

Choose a reason for hiding this comment

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

Also, I'm not entirely sure how I feel about this, if only for the selfish reason that it clutters up the development folder.

Nothing wrong with adding it to the .gitignore, but is there a better place to put this?

Copy link
Member

Choose a reason for hiding this comment

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

As it's stored using the extension path which should be shared across vscode instances. we would effectively have one global history across workspaces/projects. do we want that?

Copy link
Member

Choose a reason for hiding this comment

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

I think so. That's what vim seems to do, at least.

Copy link
Contributor Author

@KamikazeZirou KamikazeZirou May 8, 2018

Choose a reason for hiding this comment

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

vim puts. viminfo in the home directory.
If so, is it better to put this history file in your home directory?
e.g. "~/.cmdline_history", "~/.VSCodeVim/.cmdline_history"

Copy link
Member

Choose a reason for hiding this comment

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

My vote is ~/.VSCodeVim/.cmdline_history.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now, I set the file location to ~/.VSCodeVim/.cmdline_history.
a59b00a


registerCommand(context, 'toggleVim', async () => {
configuration.disableExt = !configuration.disableExt;
toggleExtension(configuration.disableExt);
Expand Down
15 changes: 15 additions & 0 deletions src/actions/base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,14 @@ export let compareKeypressSequence = function(one: string[] | string[][], two: s
);
};

const isSingleAlpha = (s: string): boolean => {
if (s.length === 1 && (('a' <= s && s <= 'z') || ('A' <= s && s <= 'Z'))) {
Copy link
Member

Choose a reason for hiding this comment

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

Regex? ^[a-zA-Z]$

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah!.
Should I use regex here?
isSingleNumber does not use regex, so I adapted it.

Copy link
Member

Choose a reason for hiding this comment

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

isSingleNumber could be regex too, but it's a little bit simpler so I'm more ambivalent.

Copy link
Member

Choose a reason for hiding this comment

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

I'd optimize for readability.

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 fix it.
1cb61f1

return true;
} else {
return false;
}
};

for (let i = 0, j = 0; i < one.length; i++, j++) {
const left = one[i],
right = two[j];
Expand All @@ -56,6 +64,13 @@ export let compareKeypressSequence = function(one: string[] | string[][], two: s
continue;
}

if (left === '<alpha>' && isSingleAlpha(right)) {
continue;
}
if (right === '<alpha>' && isSingleAlpha(left)) {
continue;
}

if (left === '<character>' && !containsControlKey(right)) {
continue;
}
Expand Down
27 changes: 26 additions & 1 deletion src/actions/commands/actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,7 @@ class CommandInsertRegisterContentInSearchMode extends BaseCommand {
@RegisterAction
class CommandRecordMacro extends BaseCommand {
modes = [ModeName.Normal, ModeName.Visual, ModeName.VisualLine];
keys = ['q', '<character>'];
keys = [['q', '<alpha>'], ['q', '<number>']];
Copy link
Member

Choose a reason for hiding this comment

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

I supposed we should also allow q" here.

Copy link
Contributor Author

@KamikazeZirou KamikazeZirou May 8, 2018

Choose a reason for hiding this comment

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

Thanks for pointing out bugs.
I fix this problem(22aabc8).
I wish I had seen the results of the vim command ":h recording" beforehand


public async exec(position: Position, vimState: VimState): Promise<VimState> {
const register = this.keysPressed[1];
Expand Down Expand Up @@ -1714,6 +1714,31 @@ class CommandShowCommandLine extends BaseCommand {
}
}

@RegisterAction
class CommandShowCommandHistory extends BaseCommand {
modes = [ModeName.Normal, ModeName.Visual, ModeName.VisualLine, ModeName.VisualBlock];
keys = ['q', ':'];

runsOnceForEveryCursor() {
return false;
}

public async exec(position: Position, vimState: VimState): Promise<VimState> {
vimState.recordedState.transformations.push({
type: 'showCommandHistory',
});

if (vimState.currentMode === ModeName.Normal) {
vimState.commandInitialText = '';
} else {
vimState.commandInitialText = "'<,'>";
}
vimState.currentMode = ModeName.Normal;

return vimState;
}
}

@RegisterAction
class CommandDot extends BaseCommand {
modes = [ModeName.Normal];
Expand Down
96 changes: 96 additions & 0 deletions src/cmd_line/commandLine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,75 @@ import * as parser from './parser';
import * as util from '../util';
import { VimError, ErrorCode } from '../error';

class CommandLineHistory {
Copy link
Member

Choose a reason for hiding this comment

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

Can you put this in it's own separate file?

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.
e43686d

private _history: string[] = [];
private _is_loaded: boolean = false;
private _filePath: string = '';

public add(command: string | undefined): void {
if (!command || command.length === 0) {
return;
}

let index: number = this._history.indexOf(command);
if (index !== -1) {
this._history.splice(index, 1);
}
this._history.unshift(command);

if (this._history.length > configuration.history) {
this._history.pop();
}
}

public get(): string[] {
if (!this._is_loaded) {
this.load();
this._is_loaded = true;
}

if (this._history.length > configuration.history) {
// resize history because "vim.history" is updated.
this._history = this._history.slice(0, configuration.history);
this.save();
}

return this._history;
}

public setFilePath(filePath: string): void {
this._filePath = filePath;
}

private load(): void {
const fs = require('fs');

if (!fs.existsSync(this._filePath)) {
return;
}

try {
let data = fs.readFileSync(this._filePath, 'utf-8');
let parsedData = JSON.parse(data);
if (Array.isArray(parsedData)) {
this._history = parsedData;
} else {
console.log('CommandLine: Failed to load history.');
}
} catch (e) {
console.error(e);
}
}

public save(): void {
const fs = require('fs');
fs.writeFileSync(this._filePath, JSON.stringify(this._history), 'utf-8');
Copy link
Member

Choose a reason for hiding this comment

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

Should this be asynchronous? I'd prefer to avoid synchronous file calls in general, but in this case, I don't see any advantage to do it synchronously.

Perhaps the other file calls should also be synchronous too, but I don't have a strong opinion on that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have an external HDD that takes a while to spin up. It looks like right now history is being written to the extension folder, but if it was ever made per-project, I'm worried these sync calls could cause a ton of lag. No way around it for loading history, but at least for saving, can this be run async at the same time as the command (e.g. don't await it)?

Similarly for load, there's no way around a slow disk read, but will the sync call cause the editor to freeze if loading takes a long time?

Copy link
Member

Choose a reason for hiding this comment

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

Well, if it's made asynchronous, it shouldn't lock up the editor. It won't be able to display until it's done loading, but I don't think that's avoidable. It should only need to load once per VSCode session though.

I'm leaning towards making all the file operations asynchronous.

Copy link
Member

Choose a reason for hiding this comment

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

Async please. Especially considering you are reading it from memory on the gets so there's no reason to block on the file write.

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 made file operations asynchronous.
0a635c3

Is it too overworked to save the command executed while loading the file?

}
}

export class CommandLine {
private static _history: CommandLineHistory = new CommandLineHistory();

public static async PromptAndRun(initialText: string, vimState: VimState): Promise<void> {
if (!vscode.window.activeTextEditor) {
console.log('CommandLine: No active document.');
Expand All @@ -20,6 +88,9 @@ export class CommandLine {
cmd = cmd.slice(1);
}

this._history.add(cmd);
this._history.save();

await CommandLine.Run(cmd!, vimState);
}

Expand Down Expand Up @@ -66,4 +137,29 @@ export class CommandLine {
],
};
}

public static async ShowHistory(
initialText: string,
vimState: VimState
): Promise<string | undefined> {
if (!vscode.window.activeTextEditor) {
console.log('CommandLine: No active document.');
return '';
}

this._history.add(initialText);

let cmd = await vscode.window.showQuickPick(this._history.get(), {
placeHolder: 'Vim command history',
ignoreFocusOut: false,
});

return cmd;
}

public static SetHistoryDirPath(historyDirPath: string): void {
const path = require('path');
const filePath: string = path.join(historyDirPath, '.cmdline_history');
this._history.setFilePath(filePath);
}
}
8 changes: 8 additions & 0 deletions src/mode/modeHandler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -911,6 +911,14 @@ export class ModeHandler implements vscode.Disposable {
this.updateView(this.vimState);
break;

case 'showCommandHistory':
let cmd = await CommandLine.ShowHistory(vimState.commandInitialText, this.vimState);
if (cmd && cmd.length !== 0) {
await CommandLine.PromptAndRun(cmd, this.vimState);
this.updateView(this.vimState);
}
break;

case 'dot':
if (!vimState.globalState.previousFullAction) {
return vimState; // TODO(bell)
Expand Down
8 changes: 8 additions & 0 deletions src/transformations/transformations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,13 @@ export interface ShowCommandLine {
type: 'showCommandLine';
}

/**
* Represents pressing ':'
*/
export interface ShowCommandHistory {
type: 'showCommandHistory';
}

/**
* Represents pressing '.'
*/
Expand Down Expand Up @@ -234,6 +241,7 @@ export type Transformation =
| DeleteTextTransformation
| MoveCursorTransformation
| ShowCommandLine
| ShowCommandHistory
| Dot
| Macro
| ContentChangeTransformation
Expand Down