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

Macro #894

Merged
merged 13 commits into from
Oct 18, 2016
Merged

Macro #894

merged 13 commits into from
Oct 18, 2016

Conversation

rebornix
Copy link
Member

Yay! We love PRs! 🎊

Please include a description of your change and ensure:

  • Commit message has a short title & issue references
  • Each commit does a logical chunk of work.
  • It builds and tests pass (e.g gulp)

Instead of tracking keystrokes, we track the translation of keystrokes, which are actions or document changes.

@rebornix rebornix changed the title initial version WIP: Macro Oct 11, 2016
@@ -15,7 +15,7 @@ export enum RegisterMode {
};

export interface IRegisterContent {
text : string | string[];
text : string | string[] | RecordedState;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't it be RecordedState[]?

Copy link
Member Author

Choose a reason for hiding this comment

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

I directly append each action to RecordedState.runActions so we don't necessarily need an array.

@@ -70,7 +77,8 @@ export class Register {
};
}

public static putByKey(content: string | string[], register = '"', registerMode = RegisterMode.FigureItOutFromCurrentMode): void {
public static putByKey(content: string | string[] | RecordedState, register = '"',
Copy link
Member

Choose a reason for hiding this comment

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

We should probably do type RegisterContent = string | string[] | RecordedState at this point.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point

@@ -611,6 +611,14 @@ class CommandInsertRegisterContent extends BaseCommand {

if (register.text instanceof Array) {
text = (register.text as string []).join("\n");
} else if (register.text instanceof RecordedState) {
vimState.recordedState.transformations.push({
Copy link
Member

Choose a reason for hiding this comment

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

Can we just immediately produce the stringified macro right here instead of shipping it off to some text transformation thing? The transformations are supposed to only be when you're actually changing the content in the text editor.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was planing to do so but later on I found we are doing this way for dot command, which is similar to Macros. Besides, actually I do want to delegate back the real content change/ action execution back to modeHandler, any ideas to do so if we don't leverage transformations? I use transformations as a way of lazy execution.

Copy link
Member

Choose a reason for hiding this comment

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

If we're actually performing a macro at this point, then we absolutely should use a transformation.

But it looked to me like we were just storing the content of the macro in a register, which doesn't require any text transformations.

Copy link
Member Author

Choose a reason for hiding this comment

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

hmn, I agree. In this particular case, we just need to insert all the keystrokes.

Copy link
Member Author

Choose a reason for hiding this comment

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

@johnfn just played with Vim again and found that running Ctrl-R {register} is actually performing the macro stored in {register}.

For example, you recored ab<BS>c<Esc> in register a, which represents Enter Insert Mode, insert b, backspace, insert c. Repeating this macro in Normal mode will just insert c and return back to Normal Mode. If you are in Insert Mode and run <C-r> a, what gets inserted is ac. The first a is translated to Insert a as it's Insert Mode and then perform all following keystrokes.

@johnfn johnfn mentioned this pull request Oct 16, 2016
@rebornix rebornix changed the title WIP: Macro Macro Oct 18, 2016
@rebornix
Copy link
Member Author

Change the code a little bit and fix several corner cases. @johnfn @jpoon @xconverge I think it's good to review then have it merged as I'm not a frequent Macro user so I'd like to have it released first and test in prod :)

@@ -84,4 +84,8 @@ export async function cleanUpWorkspace(): Promise<any> {
export function setTextEditorOptions(tabSize: number, insertSpaces: boolean): void {
Configuration.getInstance().tabstop = tabSize;
Configuration.getInstance().expandtab = insertSpaces;
let options = vscode.window.activeTextEditor.options;
options.tabSize = tabSize;
Copy link
Member

Choose a reason for hiding this comment

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

why is this section needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

This should be moved once we can really update Code's configuration through API, which relies on #913. So for now we still have to update the cache option in active text editor.

@xconverge
Copy link
Member

This is awesome!

@rebornix rebornix merged commit da8c437 into VSCodeVim:master Oct 18, 2016
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