Fix new result pane column behavior #582
Conversation
|
Hi @Raymondd, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
TTYL, MSBOT; |
|
Might be a good idea to write a simple unit test for this. |
| }, | ||
| "mssql.splitPaneSelection": { | ||
| "type": "string", | ||
| "description": "[Optional] Configuration options for which column new result panes should open in (Options: {'next'|'same'|'last'})", |
There was a problem hiding this comment.
I think you can remove the (Options: {'next'|'same'|'last'}) part - the enum values are listed as options by VSCode when altering them
| "default": "next", | ||
| "enum": [ | ||
| "next", | ||
| "same", |
There was a problem hiding this comment.
current instead of same, end instead of last unless you mean previous ?
There was a problem hiding this comment.
It does mean 'end'. Will change.
| @@ -269,4 +269,5 @@ export default class VscodeWrapper { | |||
| public get workspaceRootPath(): string { | |||
| return vscode.workspace.rootPath; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Seems like you can undo the change in this file
|
|
||
| // Check if the results window already exists | ||
| if (resultPaneURIMatch !== undefined) { | ||
| if (this.doesResultPaneExist(resultsUri)) { | ||
| // Implicity Use existsing results window by not providing an pane |
There was a problem hiding this comment.
typo existing instead of existsing (since you're changing this area anyhow)
| @@ -535,4 +534,51 @@ export class SqlOutputContentProvider implements vscode.TextDocumentContentProvi | |||
| } | |||
| return path.join(os.tmpdir(), columnName + '_' + String(Math.floor( Date.now() / 1000)) + String(process.pid) + '.' + linkType); | |||
| } | |||
|
|
|||
| /** | |||
| * Returns wether or not a result pane with the same URI exists | |||
| /** | ||
| * Returns wether or not a result pane with the same URI exists | ||
| * @param The string value of a Uri. | ||
| * @return Existence boolean |
There was a problem hiding this comment.
@return boolean true if pane exists
| * public for testing purposes | ||
| */ | ||
| public doesResultPaneExist(resultsUri: string): boolean { | ||
| let resultPaneURIMatch = vscode.workspace.textDocuments.find(tDoc => tDoc.uri.toString() === resultsUri); |
There was a problem hiding this comment.
please use this._vscodeWrapper.textDocuments() instead of direct references so it's easier to mock.
| viewColumn = this._vscodeWrapper.activeTextEditor.viewColumn; | ||
| break; | ||
| case 'last' : | ||
| viewColumn = vscode.ViewColumn.Three; |
There was a problem hiding this comment.
Is three the max value? Could this ever change?
There was a problem hiding this comment.
Three is the maximum number of panes one can have open in VSCode. Also vscode.ViewColumn only has properties one, two, and three.
| }); | ||
|
|
||
|
|
||
| // TODO: rewrite all the outputprodiver handle tests (old ones kept for reference) |
There was a problem hiding this comment.
is there an issue tracking it? if so what's the issue #? also typo outputprovider
| expectedColumn: number; | ||
| } | ||
|
|
||
| let cases: Case[] = [ |
There was a problem hiding this comment.
It would be nice if each of these was a test case. See https://mochajs.org/#dynamically-generating-tests for how to easily convert this (basically define cases at the suite level and in the foreach define a test.
There was a problem hiding this comment.
I could dynamically generate each test, but would it be worth it to have 9 test cases and an entire suite for this one function?
There was a problem hiding this comment.
Sounds good, leave as-is
Fixes #549.
splitPaneSelection configuration property added. Corresponding behavior added to vscodeWrapper class.