Implement "q:" command #2618

KamikazeZirou · 2018-05-06T17:33:55Z

What this PR does / why we need it:
I implemented "q:" command.

Which issue(s) this PR fixes
#2617

Special notes for your reviewer:
This "q:" command slightly differ from original.
Command is not immediately executed when the command is selected from the history.
Instead of executing command, display it on command-line.
Currently, there is no way to edit past commands, so I thought that this kind of action was better.

KamikazeZirou · 2018-05-07T13:34:14Z

I'm sorry I fixed code before receiving a comment.
I noticed a bug with the first code.

Chillee · 2018-05-07T17:06:14Z

src/actions/commands/actions.ts

@@ -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>']];


I supposed we should also allow q" here.

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

Chillee · 2018-05-07T17:16:22Z

src/cmd_line/commandLine.ts

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


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.

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?

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.

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

I made file operations asynchronous.
0a635c3

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

Chillee · 2018-05-07T17:19:07Z

Oh this is dope! I've thought it was irritating for a while that we couldn't implement <up>/<down> in the command line, but I didn't even know this was a thing.

Chillee · 2018-05-07T17:29:35Z

extension.ts

@@ -265,6 +265,8 @@ export async function activate(context: vscode.ExtensionContext) {
    }
  }

+  CommandLine.SetHistoryDirPath(context.extensionPath);


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?

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?

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

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"

My vote is ~/.VSCodeVim/.cmdline_history.

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

Chillee

I'm mostly concerned about the synchronous file commands.

Chillee · 2018-05-07T20:58:04Z

src/cmd_line/commandLine.ts

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


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.

jpoon · 2018-05-07T22:04:50Z

src/actions/base.ts

@@ -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'))) {


Regex? ^[a-zA-Z]$

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

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

I'd optimize for readability.

I fix it.
1cb61f1

jpoon · 2018-05-08T05:30:03Z

src/cmd_line/commandLine.ts

@@ -8,7 +8,75 @@ import * as parser from './parser';
 import * as util from '../util';
 import { VimError, ErrorCode } from '../error';

+class CommandLineHistory {


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

Yes.
e43686d

jpoon · 2018-05-08T05:38:07Z

Thanks @KamikazeZirou! Also, please add tests.

KamikazeZirou · 2018-05-08T14:21:22Z

Thanks for yours comment!
I corresponded to some points.
Please wait for the test code and the location of the history file.

Chillee · 2018-05-10T01:21:06Z

@jpoon I'm not sure our testing framework allows us to test this kind of stuff.

KamikazeZirou · 2018-05-11T16:15:38Z

@jpoon . I added tests.

If there is no further indication, I think that this work is completed

Chillee · 2018-05-11T16:19:33Z

Thanks for the great work!

KamikazeZirou added 2 commits May 7, 2018 01:18

feat: add "q:" command

9749505

fix: Changing vim.history isn't reflected

8068c5f

Chillee reviewed May 7, 2018

View reviewed changes

Chillee mentioned this pull request May 7, 2018

Partial Search and Replace Missing Features/Behavior #2549

Closed

Chillee requested changes May 7, 2018

View reviewed changes

jpoon reviewed May 7, 2018

View reviewed changes

jpoon reviewed May 8, 2018

View reviewed changes

KamikazeZirou added 3 commits May 8, 2018 20:01

fix: Recording macro into double-quote doen't work

22aabc8

update: Change file operation to asynchronous

0a635c3

update: put CommandLineHistory in it's own separate file.

e43686d

KamikazeZirou added 2 commits May 9, 2018 20:25

update: improve method for determining one alphabet

1cb61f1

update: Change the location of history file.

a59b00a

Chillee approved these changes May 10, 2018

View reviewed changes

KamikazeZirou added 3 commits May 10, 2018 21:16

Merge branch 'master' into add-q-cron-command

32312a1

update: Use Logger instead of console.log().

a5b2971

update: add test code of commandLineHistory

011e6ff

Chillee merged commit 36bf0a9 into VSCodeVim:master May 11, 2018

xconverge mentioned this pull request May 16, 2018

[FEATURE REQUEST] "q:" command #2617

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement "q:" command #2618

Implement "q:" command #2618

KamikazeZirou commented May 6, 2018

KamikazeZirou commented May 7, 2018

Chillee May 7, 2018

KamikazeZirou May 8, 2018 •

edited

Loading

Chillee May 7, 2018

parkovski May 7, 2018

Chillee May 7, 2018

jpoon May 8, 2018

KamikazeZirou May 8, 2018

Chillee commented May 7, 2018

Chillee May 7, 2018

jpoon May 8, 2018

Chillee May 8, 2018

KamikazeZirou May 8, 2018 •

edited

Loading

jpoon May 9, 2018

KamikazeZirou May 9, 2018

Chillee left a comment

Chillee May 7, 2018

jpoon May 7, 2018

KamikazeZirou May 8, 2018

Chillee May 8, 2018

jpoon May 8, 2018

KamikazeZirou May 9, 2018

jpoon May 8, 2018

KamikazeZirou May 8, 2018

jpoon commented May 8, 2018

KamikazeZirou commented May 8, 2018 •

edited

Loading

Chillee commented May 10, 2018

KamikazeZirou commented May 11, 2018

Chillee commented May 11, 2018

Implement "q:" command #2618

Implement "q:" command #2618

Conversation

KamikazeZirou commented May 6, 2018

KamikazeZirou commented May 7, 2018

Choose a reason for hiding this comment

KamikazeZirou May 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chillee commented May 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KamikazeZirou May 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chillee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpoon commented May 8, 2018

KamikazeZirou commented May 8, 2018 • edited Loading

Chillee commented May 10, 2018

KamikazeZirou commented May 11, 2018

Chillee commented May 11, 2018

KamikazeZirou May 8, 2018 •

edited

Loading

KamikazeZirou May 8, 2018 •

edited

Loading

KamikazeZirou commented May 8, 2018 •

edited

Loading