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
Add tab completion on vim command line #3639
Conversation
src/actions/commands/actions.ts
Outdated
) { | ||
commandLine.autoCompleteIndex += 1; | ||
} else if (/ /g.test(vimState.currentCommandlineText)) { | ||
var search = <RegExpExecArray>/(?:.* .*\/|.* )(.*)/g.exec(vimState.currentCommandlineText); |
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.
Best practice in typescript is to avoid var
(here's why). Use const
when you're only assigning to the variable when it's declared, and let
otherwise.
@J-Fields I replaced var with const or let let me know if there are any other things that need changed. |
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.
Sorry for the delay. Was only able to go through half of it before 👶 -duties.
package.json
Outdated
"vscode": "1.1.30" | ||
} | ||
} | ||
"name": "vim", |
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.
Is there a reason why you adjusted the tab sizes? This makes it difficult to review...
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.
sorry I have vscode tab size set to 4 for work thought I fixed most of the places that it affected
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.
Can you revert the whitespace changes you have to this file?
src/actions/commands/actions.ts
Outdated
public async exec(position: Position, vimState: VimState): Promise<VimState> { | ||
const key = this.keysPressed[0]; | ||
const searchState = vimState.globalState.searchState!; | ||
const statusbartext = |
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.
Add a get
method on StatusBar
instead of doing this.
src/actions/commands/actions.ts
Outdated
) { | ||
commandLine.autoCompleteIndex += 1; | ||
} else if (/ /g.test(vimState.currentCommandlineText)) { | ||
const search = <RegExpExecArray>/(?:.* .*\/|.* )(.*)/g.exec(vimState.currentCommandlineText); |
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.
Can you add a comment to the regex. Always takes me awhile to digest regexs...
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.
Why is it necessary to have a specific case for " "?
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.
the ?: is to make the group non capturing
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.
and I will add a comment to the regex.
src/actions/commands/actions.ts
Outdated
commandLine.autoCompleteIndex = 0; | ||
} | ||
let result = completionItems[commandLine.autoCompleteIndex]; | ||
if (result === vimState.currentCommandlineText && completionItems.length > 1) { |
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 can be simplified to:
if (result === vimState.currentCommandlineText) {
result = completionItems[++commandLine.autoCompleteIndex % completionItems.length];
}
Travis tests have failedHey @keith-ferney, Node.js: 8if [[ $(git diff-index HEAD -- *.js *.ts *.md) ]]; then git diff; echo "Prettier Failed. Run `gulp forceprettier` and commit changes to resolve."; exit 1; fi
npm test --silent
TravisBuddy Request Identifier: b2548740-564f-11e9-8811-9d5afad75740 |
@jpoon this should be ready for review again. |
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.
Sorry again for the delay. Can you add tests?
src/statusBar.ts
Outdated
text = text.replace(/^(?:\/|\:)(.*)(?:\|)(.*)/, '$1$2'); | ||
|
||
// This regex will remove the space at the end of the sting | ||
text.replace(/\s$/, ''); |
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.
.trim?
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.
yeah good point
@@ -55,6 +55,17 @@ class StatusBarImpl implements vscode.Disposable { | |||
this._statusBarItem.dispose(); | |||
} | |||
|
|||
public Get() { |
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.
:( why on earth did we do pascal case for this file? what idiot did that (fyi. it was me).
return `${leadingChar}${vimState.globalState.searchState!.searchString}`; | ||
|
||
let stringWithCursor = vimState.globalState.searchState!.searchString.split(''); | ||
stringWithCursor.splice(vimState.statusBarCursorCharacterPos, 0, '|'); |
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.
As per your comment in the Get()
method, isn't the pipe removed?
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.
yeah it is mainly doing that for the search I should probably move the trimming happening in the Get()
method to a different method eg GetTrimmed()
since it is not just getting the status bar text.
src/cmd_line/commands/file.ts
Outdated
@@ -95,6 +95,7 @@ export class FileCommand extends node.CommandBase { | |||
filePath = path.isAbsolute(this._arguments.name) | |||
? path.normalize(this._arguments.name) | |||
: path.join(path.dirname(editorFilePath), this._arguments.name); | |||
console.log(filePath); |
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.
Did you intend to keep this here?
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.
no sorry about that
yeah I will add tests. |
Travis tests have failedHey @keith-ferney, Node.js: 8if [[ $(git diff-index HEAD -- *.js *.ts *.md) ]]; then git diff; echo "Prettier Failed. Run `gulp forceprettier` and commit changes to resolve."; exit 1; fi
npm test --silent
TravisBuddy Request Identifier: ebbb9cf0-6859-11e9-9ccb-85565a33c18e |
I added some tests @jpoon |
src/actions/commands/actions.ts
Outdated
result = completionItems[++commandLine.autoCompleteIndex % completionItems.length]; | ||
} | ||
|
||
if (result === vimState.currentCommandlineText) { |
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.
else if?
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 if is superfluous so i removed it
src/actions/commands/actions.ts
Outdated
|
||
const editorFilePath = vscode.window.activeTextEditor!.document.uri.fsPath; | ||
|
||
let basePath = path.dirname(editorFilePath); |
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 probably belongs somewhere else. Can you put this in util
or something?
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.
it is in a util
now
src/cmd_line/commandLine.ts
Outdated
@@ -17,6 +17,24 @@ class CommandLine { | |||
*/ | |||
private _commandLineHistoryIndex: number = 0; | |||
|
|||
/** | |||
* for checking the last pressed key in command mode | |||
* |
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.
Extra star/carriage return on these comments.
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.
removed the extra star/carriage return
src/statusBar.ts
Outdated
return text; | ||
} | ||
|
||
public GetTrimmed() { |
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.
Where are you consuming this? This is also highly coupled with the status bar completion code -- I don't think this logic belongs here.
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.
moved this to actions.ts CommandNavigateInCommandlineOrSearchMode class
…ions CommandNavigateInCommandlineOrSearchMode
… tests for command line tab completion
@jpoon I think this should be ready for review again. |
package.json
Outdated
"vscode": "1.1.30" | ||
} | ||
} | ||
"name": "vim", |
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.
Can you revert the whitespace changes you have to this file?
src/util/path.ts
Outdated
/** | ||
* Get the base path based on a path | ||
*/ | ||
export function AbsolutePathFromRelativePath(partialPath: string): string { |
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.
Can you rename this to...GetAbsolutePath
and adjust the comment? should probably say something more along the lines of:
Given relative path, calculate absolute path
.
src/util/path.ts
Outdated
if (partialPath.startsWith('/')) { | ||
basePath = '/'; | ||
} else if (partialPath.startsWith('~/')) { | ||
basePath = os.homedir(); |
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.
We've been using untildify
for this.
Travis tests have failedHey @keith-ferney, Node.js: 8npm run forceprettier
if [[ $(git diff-index HEAD -- *.js *.ts *.md) ]]; then git diff; echo "Prettier Failed. Run `gulp forceprettier` and commit changes to resolve."; exit 1; fi
npm run build
npm test --silent
TravisBuddy Request Identifier: a934cf80-7017-11e9-91fe-d9d12b7facfa |
…mment to match and use untildify instead of os.homedir()
should be ready for review again @jpoon. |
package-lock.json
Outdated
@@ -131,13 +131,13 @@ | |||
"@types/diff-match-patch": { |
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.
There were no changes to package.json. Can you remove these changes?
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.
yeah
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.
those changes were removed.
What this PR does / why we need it:
This pull request will add tab completion on vim command line support as requested in #745.
And add a cursor to search mode. ( I am not aware of any issue/feature request for this one, I just added it while I was working in a similar area )
Which issue(s) this PR fixes
fixes #745
and adds a cursor to search mode
Special notes for your reviewer:
relatively new to typescript, sorry for any beginner mistakes I might have made.
also I built this testing in Linux so it might need some extra things to work on windows/mac