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.

Sign up for GitHub

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

Support "+ system clipboard register (#780) #782

Merged
merged 6 commits into from
Sep 21, 2016
Merged

Support "+ system clipboard register (#780) #782

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
55582a1
Support "+ system clipboard register
bdchauvette Sep 19, 2016
d3bae99
Merge branch 'master' into register-quoteplus
bdchauvette Sep 19, 2016
e3e6d58
Merge branch 'master' into register-quoteplus
bdchauvette Sep 19, 2016
ae5f010
Add isClipboardRegister? to IRegisterContent
bdchauvette Sep 20, 2016
d6e1352
Merge branch 'master' into register-quoteplus
bdchauvette Sep 21, 2016
6d46247
Make IRegisterContent.isClipboardRegister required
bdchauvette Sep 21, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 26 additions & 11 deletions src/register/register.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { VimState } from './../mode/modeHandler';
import * as clipboard from 'copy-paste';

/**
* There are two different modes of copy/paste in Vim - copy by character
* and copy by line. Copy by line typically happens in Visual Line mode, but
Expand All @@ -14,14 +15,15 @@ export enum RegisterMode {
};

export interface IRegisterContent {
text : string | string[];
registerMode: RegisterMode;
text : string | string[];
registerMode : RegisterMode;
isClipboardRegister?: boolean;
}

export class Register {
/**
* The '"' is the unnamed register.
* The '*' is the special register for stroing into system clipboard.
* The '*' and '+' are special registers for accessing the system clipboard.
* TODO: Read-Only registers
* '.' register has the last inserted text.
* '%' register has the current file path.
Expand All @@ -30,9 +32,20 @@ export class Register {
*/
private static registers: { [key: string]: IRegisterContent } = {
'"': { text: "", registerMode: RegisterMode.CharacterWise },
'*': { text: "", registerMode: RegisterMode.CharacterWise }
'*': { text: "", registerMode: RegisterMode.CharacterWise, isClipboardRegister: true },
'+': { text: "", registerMode: RegisterMode.CharacterWise, isClipboardRegister: true }
};

public static isClipboardRegister(registerName: string): boolean {
const register = Register.registers[registerName];

if (register && register.isClipboardRegister) {
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 just do return register && register.isClipboardRegister?

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 actually tried that initially, but tsc doesn't like it very much:

> vim@0.1.10 vscode:prepublish c:\Users\Ben\Code\vscode-vim
> node ./node_modules/vscode/bin/compile -p ./
src/register/register.ts(42,12): error TS2322: Type 'boolean | undefined' is not assignable to type 'boolean'.
  Type 'undefined' is not assignable to type 'boolean'.

It works fine if I change the signature to isClipboardRegister(string): boolean | undefined, but I wasn't sure if that was preferable to having a messier function body.

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 show me the exact line(s) of code that you're using to get that error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  public static isClipboardRegister(registerName: string): boolean {
    const register = Register.registers[registerName];
    return register && register.isClipboardRegister;
  }

capture

I think it's being caused by the isClipboardRegister property being optional: the property has a type signature of boolean | undefined, but the Register.isClipboardRegister method expects to only return booleans.

If we try to return the property without first converting it to a boolean, it's possible for the value to undefined (b/c it's optional), which violates the signature for the checking method.

(I think) this would also explain why it works if I change the allowed return type to boolean | undefined.

I'm pretty new to typescript, so I'm not sure what the idiomatic way to handle this kind of thing is 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Each of these allow the code to compile without a hitch:

// Cast to boolean at runtime
return register && Boolean(register.isClipboardRegister);

// or:
return register && !!register.isClipboardRegister;
// Type assertion
return register && (register.isClipboardRegister as boolean);
// No return type annotation
public static isClipboardRegister(registerName: string) {
  const register = Register.registers[registerName];
  return register && register.isClipboardRegister;
}

Copy link
Member

Choose a reason for hiding this comment

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

Ohhhhh. It's because it's an optional property, which has type boolean | undefined, which we are returning directly. That makes sense... in a weird way. :)

I guess TypeScript is pointing out that we did a dumb thing by saying the type was isClipboardRegister?: boolean since now we can't distinguish undefined from false in an easy way.

Best is probably to change the type to isClipboardRegister: boolean, and add it to all registers. Then this code will work and also our undefined vs false case will go away.

TypeScript caught a (minor) bug! Yay, TypeScript!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yay, TypeScript! 🙌

Looks like I just broke Travis, though. Any ideas what's going on there? 😕

Copy link
Member

@jpoon jpoon Sep 21, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

¯_(ツ)_/¯ restarted the Travis build.

return true;
}

return false;
}

public static isValidRegister(register: string): boolean {
return register in Register.registers || /^[a-z0-9]+$/i.test(register);
}
Expand All @@ -48,13 +61,14 @@ export class Register {
throw new Error(`Invalid register ${register}`);
}

if (register === '*') {
if (Register.isClipboardRegister(register)) {
clipboard.copy(content);
}

Register.registers[register] = {
text : content,
registerMode: vimState.effectiveRegisterMode(),
text : content,
registerMode : vimState.effectiveRegisterMode(),
isClipboardRegister: Register.isClipboardRegister(register),
};
}

Expand All @@ -65,13 +79,14 @@ export class Register {
throw new Error(`Invalid register ${register}`);
}

if (register === '*') {
if (Register.isClipboardRegister(register)) {
clipboard.copy(content);
}

Register.registers[register] = {
text : content,
registerMode: registerMode || RegisterMode.FigureItOutFromCurrentMode,
text : content,
registerMode : registerMode || RegisterMode.FigureItOutFromCurrentMode,
isClipboardRegister: Register.isClipboardRegister(register),
};
}

Expand All @@ -94,7 +109,7 @@ export class Register {
}

/* Read from system clipboard */
if (register === '*') {
if (Register.isClipboardRegister(register)) {
const text = await new Promise<string>((resolve, reject) =>
clipboard.paste((err, text) => {
if (err) {
Expand Down
8 changes: 8 additions & 0 deletions test/register/register.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,21 @@ suite("register", () => {
});

clipboard.copy("12345");

newTest({
title: "Can access '*' (clipboard) register",
start: ['|one'],
keysPressed: '"*P',
end: ["1234|5one"],
});

newTest({
title: "Can access '+' (clipboard) register",
start: ['|one'],
keysPressed: '"+P',
end: ["1234|5one"],
});

newTest({
title: "Can use two registers together",
start: ['|one', "two"],
Expand Down