Open file in new window. #404

rebornix · 2016-07-07T12:09:29Z

Yay! We love PRs! 🎊

Please include a description of your change & check your PR against this list, thanks:

Commit message has a short title & issue references
Commits are squashed
It builds and tests pass (e.g gulp tslint)

Support :vsp, :vne and :vnew.

This feature is limited to VS Code's group editors, which only support 3 groups at most. Besides, all horizontal splitting commands wont' work in VS Code

johnfn · 2016-07-08T07:46:55Z

Sorry, would you mind fixing those merge conflicts?

This feature is limited to VS Code's group editors, which only support 3 groups at most.

rebornix · 2016-07-11T14:27:54Z

@johnfn I've updated the code to latest :)

rebornix · 2016-07-11T14:32:39Z

BTW, this PR fixes #384 as well.

johnfn · 2016-07-12T18:26:20Z

src/cmd_line/commands/file.ts

+        }
+
+        switch (active.viewColumn) {
+            case vscode.ViewColumn.One:


So what is this function doing? What is the sideBySide parameter?

It seems to me like you might want two separate functions. getActiveViewColumn() and getViewColumnToRight()

This function is used to get the correct column of group editors when we open new files. If sideBySide is true, it means, we want to get the column number of next Group Editor instead of current active editor. But since we can only have 3 columns at most, we need to do some validation on the column number of current editor, if it's already 3, we have to open the file in current editor.

If sideBySide is false, what we want to do is get the column of current active editor.

This is what we do in VS Code or any file-related extension (DefaultConfig/UserConfig, Markdown Preview, reStructuredText Preview, etc). Since people might want to tweak it a little bit sometimes, we don't make it an API of any VS Code libraries.

If we separate it to two functions, we may need move logics to the caller so I'd like to keep it this way.

Ok, I understand now.

If we separate it to two functions, we may need move logics to the caller so I'd like to keep it this way.

I don't think so. The caller still needs to determine whether to set sideBySide to be true or false. That would be the same as determining whether to call getActiveViewColumn() and getViewColumnToRight(). I suggest splitting the function since those feel to me like separate bits of functionality.

Sure, will do that.

rebornix · 2016-07-13T02:55:42Z

Revise code per comments by @johnfn :)

johnfn · 2016-07-13T09:00:55Z

src/cmd_line/subparsers/file.ts

@@ -9,7 +9,7 @@ export function parseEditFileCommandArgs(args: string): node.FileCommand {
    }

    var scanner = new Scanner(args);
-    let name = scanner.nextWord();
+    var name = scanner.nextWord();


Wait, why did you change more lets to vars? That's the opposite of what I want! 😉

Now I'm a little bit confused. Previously I was thinking your way of deciding when to use let or var is

If it's inside a block ({ }, for loop), then we can use let

If it's just in the very top level of the function, then we use var

Since JavaScript does hoisting and moves all declaration to the beginning this var name here is almost the same as var name = "" below in parseEditFileInNewWindowCommandArgs (https://github.com/VSCodeVim/Vim/pull/404/files#diff-16bc6b0f3306c3b8bdc5733a584411ffR27). You must have solid reason for this but I'm sorry that I'm not that clear now, do you mind pointing it out?

My thoughts about variable types:

Use const as much as possible

When const is impossible (the variable gets changed), use let

Even though let and var are equivalent in this case, let is generally superior to var and there is no disadvantage to using let; therefore we should always use let.

I'll whip up a style guide soon. 😄

oh your suggestion is using let whenever possible.

so previously when you said use var, not let in parseEditFileInNewWindowCommandArgs is kind of typo? :)

OOPS! Yes, that was a typo! Sorry!

perfect. We are using let totally the same way. let always :) I'll give it a quick turnaround.

rebornix · 2016-07-14T12:35:56Z

@johnfn and I misunderstood each other about the usage of let and var but actually we are saying the same thing: use let always possible.

Fixed all existing comments, I suppose it's a good time to merge.

johnfn · 2016-07-15T08:30:43Z

@rebornix 2 things left:

Split that function into 2
Switch that one var to a let that I commented on (sorry!)

Then it should be good to go!

rebornix · 2016-07-15T14:19:33Z

I must be drunk last time I checked in code, I thought I already fixed those issues ...

Follow your Better Naming suggestion and replace all var with let in my PR. Maybe we can create some gulp task fix-var similar to fix-whitespace :)

Open file in new window.

44621c9

This feature is limited to VS Code's group editors, which only support 3 groups at most.

rebornix force-pushed the FileInNewWindow branch from 6d432c9 to 44621c9 Compare July 11, 2016 14:24

johnfn reviewed Jul 12, 2016
View reviewed changes

johnfn reviewed Jul 13, 2016
View reviewed changes

rebornix force-pushed the FileInNewWindow branch from 56eaa9a to cdeac3c Compare July 14, 2016 12:34

Open file in new window.

d44b0ba

rebornix force-pushed the FileInNewWindow branch from cdeac3c to d44b0ba Compare July 15, 2016 14:17

Fix whitespace

3ff1655

jpoon merged commit e06daab into VSCodeVim:master Jul 16, 2016

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Open file in new window. #404

Open file in new window. #404

rebornix commented Jul 7, 2016

johnfn commented Jul 8, 2016

rebornix commented Jul 11, 2016

rebornix commented Jul 11, 2016

johnfn Jul 12, 2016 •

edited

Loading

rebornix Jul 13, 2016

johnfn Jul 13, 2016 •

edited

Loading

rebornix Jul 13, 2016

rebornix commented Jul 13, 2016

johnfn Jul 13, 2016

rebornix Jul 13, 2016

johnfn Jul 13, 2016 •

edited

Loading

rebornix Jul 14, 2016

johnfn Jul 14, 2016

rebornix Jul 14, 2016

rebornix commented Jul 14, 2016

johnfn commented Jul 15, 2016

rebornix commented Jul 15, 2016

Open file in new window. #404

Open file in new window. #404

Conversation

rebornix commented Jul 7, 2016

johnfn commented Jul 8, 2016

rebornix commented Jul 11, 2016

rebornix commented Jul 11, 2016

johnfn Jul 12, 2016 • edited Loading

Choose a reason for hiding this comment

rebornix Jul 13, 2016

Choose a reason for hiding this comment

johnfn Jul 13, 2016 • edited Loading

Choose a reason for hiding this comment

rebornix Jul 13, 2016

Choose a reason for hiding this comment

rebornix commented Jul 13, 2016

johnfn Jul 13, 2016

Choose a reason for hiding this comment

rebornix Jul 13, 2016

Choose a reason for hiding this comment

johnfn Jul 13, 2016 • edited Loading

Choose a reason for hiding this comment

rebornix Jul 14, 2016

Choose a reason for hiding this comment

johnfn Jul 14, 2016

Choose a reason for hiding this comment

rebornix Jul 14, 2016

Choose a reason for hiding this comment

rebornix commented Jul 14, 2016

johnfn commented Jul 15, 2016

rebornix commented Jul 15, 2016

johnfn Jul 12, 2016 •

edited

Loading

johnfn Jul 13, 2016 •

edited

Loading

johnfn Jul 13, 2016 •

edited

Loading