-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Open file in new window. #404
Conversation
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.
6d432c9
to
44621c9
Compare
@johnfn I've updated the code to latest :) |
BTW, this PR fixes #384 as well. |
} | ||
|
||
switch (active.viewColumn) { | ||
case vscode.ViewColumn.One: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, will do that.
Revise code per comments by @johnfn :) |
@@ -9,7 +9,7 @@ export function parseEditFileCommandArgs(args: string): node.FileCommand { | |||
} | |||
|
|||
var scanner = new Scanner(args); | |||
let name = scanner.nextWord(); | |||
var name = scanner.nextWord(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, why did you change more let
s to var
s? That's the opposite of what I want! 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 uselet
- 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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thoughts about variable types:
- Use
const
as much as possible - When
const
is impossible (the variable gets changed), uselet
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. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh your suggestion is using let
whenever possible.
so previously when you said use var, not let
in parseEditFileInNewWindowCommandArgs
is kind of typo? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OOPS! Yes, that was a typo! Sorry!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perfect. We are using let
totally the same way. let
always :) I'll give it a quick turnaround.
56eaa9a
to
cdeac3c
Compare
@johnfn and I misunderstood each other about the usage of Fixed all existing comments, I suppose it's a good time to merge. |
@rebornix 2 things left:
Then it should be good to go! |
cdeac3c
to
d44b0ba
Compare
I must be drunk last time I checked in code, I thought I already fixed those issues ... Follow your |
Yay! We love PRs! 🎊
Please include a description of your change & check your PR against this list, thanks:
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