From 58ae8b3820a7f003779074cc6b61855dc2e8b0ac Mon Sep 17 00:00:00 2001 From: David Kutugata Date: Tue, 3 Mar 2020 14:55:18 -0800 Subject: [PATCH 1/5] Jupyter autocompletion will only show up on empty lines, instead of appearing in functions. --- news/2 Fixes/10023.md | 1 + .../intellisense/intellisenseProvider.ts | 33 +++++++++++++++---- 2 files changed, 27 insertions(+), 7 deletions(-) create mode 100644 news/2 Fixes/10023.md diff --git a/news/2 Fixes/10023.md b/news/2 Fixes/10023.md new file mode 100644 index 000000000000..c1820ff7b269 --- /dev/null +++ b/news/2 Fixes/10023.md @@ -0,0 +1 @@ +Jupyter autocompletion will only show up on empty lines, instead of appearing in functions. diff --git a/src/client/datascience/interactive-common/intellisense/intellisenseProvider.ts b/src/client/datascience/interactive-common/intellisense/intellisenseProvider.ts index 9a4821ca2057..4408e5ad888d 100644 --- a/src/client/datascience/interactive-common/intellisense/intellisenseProvider.ts +++ b/src/client/datascience/interactive-common/intellisense/intellisenseProvider.ts @@ -377,12 +377,21 @@ export class IntellisenseProvider implements IInteractiveWindowListener { request.cellId, cancelSource.token ); - const jupyterCompletions = this.provideJupyterCompletionItems( - request.position, - request.context, - request.cellId, - cancelSource.token - ); + + let jupyterCompletions = Promise.resolve(emptyList); + const isEmptyOrWhitespace = await this.includeJupyterCompletionItems(request.position, request.cellId); + let sleepTime = 0; + + if (isEmptyOrWhitespace) { + jupyterCompletions = this.provideJupyterCompletionItems( + request.position, + request.context, + request.cellId, + cancelSource.token + ); + + sleepTime = Settings.IntellisenseTimeout; + } // Capture telemetry for each of the two providers. // Telemetry will be used to improve how we handle intellisense to improve response times for code completion. @@ -397,7 +406,7 @@ export class IntellisenseProvider implements IInteractiveWindowListener { // Telemetry will prove/disprove this assumption and we'll change this code accordingly. lsCompletions, // Wait for a max of n ms before ignoring results from jupyter (jupyter completion is generally slower). - Promise.race([jupyterCompletions, sleep(Settings.IntellisenseTimeout).then(() => emptyList)]) + Promise.race([jupyterCompletions, sleep(sleepTime).then(() => emptyList)]) ]) ); }; @@ -409,6 +418,16 @@ export class IntellisenseProvider implements IInteractiveWindowListener { }); } + // This function returns weather the line the user is using intellisense on is an empty line or not. + // If it is not, handleCompletionItemsRequest will ignore jupyter compeltion items becasue its + // confusing to the user to receive magic commands in a function. + private async includeJupyterCompletionItems(position: monacoEditor.Position, cellId: string): Promise { + const doc = await this.getDocument(); + const pos = doc.convertToDocumentPosition(cellId, position.lineNumber, position.column); + const line = doc.lineAt(pos); + return line.isEmptyOrWhitespace; + } + private handleResolveCompletionItemRequest(request: IResolveCompletionItemRequest) { // Create a cancellation source. We'll use this for our sub class request and a jupyter one const cancelSource = new CancellationTokenSource(); From 24e31f90a2e7da2d34aab7366c511725c3576f5e Mon Sep 17 00:00:00 2001 From: David Kutugata Date: Tue, 3 Mar 2020 17:19:09 -0800 Subject: [PATCH 2/5] filter out magic commands instead of ignoring all the jupyter intellisense. --- news/2 Fixes/10023.md | 2 +- .../intellisense/intellisenseProvider.ts | 46 +++++++------------ 2 files changed, 17 insertions(+), 31 deletions(-) diff --git a/news/2 Fixes/10023.md b/news/2 Fixes/10023.md index c1820ff7b269..d335b8eaf47b 100644 --- a/news/2 Fixes/10023.md +++ b/news/2 Fixes/10023.md @@ -1 +1 @@ -Jupyter autocompletion will only show up on empty lines, instead of appearing in functions. +Jupyter autocompletion will only show magic commands on empty lines, preventing them of appearing in functions. diff --git a/src/client/datascience/interactive-common/intellisense/intellisenseProvider.ts b/src/client/datascience/interactive-common/intellisense/intellisenseProvider.ts index 4408e5ad888d..0b8aeac21eb3 100644 --- a/src/client/datascience/interactive-common/intellisense/intellisenseProvider.ts +++ b/src/client/datascience/interactive-common/intellisense/intellisenseProvider.ts @@ -378,20 +378,12 @@ export class IntellisenseProvider implements IInteractiveWindowListener { cancelSource.token ); - let jupyterCompletions = Promise.resolve(emptyList); - const isEmptyOrWhitespace = await this.includeJupyterCompletionItems(request.position, request.cellId); - let sleepTime = 0; - - if (isEmptyOrWhitespace) { - jupyterCompletions = this.provideJupyterCompletionItems( - request.position, - request.context, - request.cellId, - cancelSource.token - ); - - sleepTime = Settings.IntellisenseTimeout; - } + const jupyterCompletions = this.provideJupyterCompletionItems( + request.position, + request.context, + request.cellId, + cancelSource.token + ); // Capture telemetry for each of the two providers. // Telemetry will be used to improve how we handle intellisense to improve response times for code completion. @@ -406,7 +398,7 @@ export class IntellisenseProvider implements IInteractiveWindowListener { // Telemetry will prove/disprove this assumption and we'll change this code accordingly. lsCompletions, // Wait for a max of n ms before ignoring results from jupyter (jupyter completion is generally slower). - Promise.race([jupyterCompletions, sleep(sleepTime).then(() => emptyList)]) + Promise.race([jupyterCompletions, sleep(Settings.IntellisenseTimeout).then(() => emptyList)]) ]) ); }; @@ -418,16 +410,6 @@ export class IntellisenseProvider implements IInteractiveWindowListener { }); } - // This function returns weather the line the user is using intellisense on is an empty line or not. - // If it is not, handleCompletionItemsRequest will ignore jupyter compeltion items becasue its - // confusing to the user to receive magic commands in a function. - private async includeJupyterCompletionItems(position: monacoEditor.Position, cellId: string): Promise { - const doc = await this.getDocument(); - const pos = doc.convertToDocumentPosition(cellId, position.lineNumber, position.column); - const line = doc.lineAt(pos); - return line.isEmptyOrWhitespace; - } - private handleResolveCompletionItemRequest(request: IResolveCompletionItemRequest) { // Create a cancellation source. We'll use this for our sub class request and a jupyter one const cancelSource = new CancellationTokenSource(); @@ -488,6 +470,14 @@ export class IntellisenseProvider implements IInteractiveWindowListener { const jupyterResults = await activeNotebook.getCompletion(data.text, offsetInCode, cancelToken); if (jupyterResults && jupyterResults.matches) { + // If the line we're analyzing is empty or a whitespace, we filter out the magic commands + // as its confusing to see them appear after a . or inside (). + const pos = document.convertToDocumentPosition(cellId, position.lineNumber, position.column); + const line = document.lineAt(pos); + const filteredMatches = line.isEmptyOrWhitespace + ? jupyterResults.matches + : jupyterResults.matches.filter(match => !match.startsWith('%')); + const baseOffset = data.offset; const basePosition = document.positionAt(baseOffset); const startPosition = document.positionAt(jupyterResults.cursor.start + baseOffset); @@ -499,11 +489,7 @@ export class IntellisenseProvider implements IInteractiveWindowListener { endColumn: endPosition.character + 1 }; return { - suggestions: convertStringsToSuggestions( - jupyterResults.matches, - range, - jupyterResults.metadata - ), + suggestions: convertStringsToSuggestions(filteredMatches, range, jupyterResults.metadata), incomplete: false }; } From c46453200ceac3f03e8c62eb3f88f6002f3646b7 Mon Sep 17 00:00:00 2001 From: David Kutugata Date: Tue, 10 Mar 2020 12:06:24 -0700 Subject: [PATCH 3/5] moved the new code to a function and created tests --- .../intellisense/intellisenseProvider.ts | 26 +++++--- .../intellisense.functional.test.tsx | 66 ++++++++++++++++++- src/test/datascience/mockJupyterSession.ts | 2 +- src/test/datascience/testHelpers.tsx | 11 ++++ 4 files changed, 95 insertions(+), 10 deletions(-) diff --git a/src/client/datascience/interactive-common/intellisense/intellisenseProvider.ts b/src/client/datascience/interactive-common/intellisense/intellisenseProvider.ts index 0b8aeac21eb3..0712c78dd68e 100644 --- a/src/client/datascience/interactive-common/intellisense/intellisenseProvider.ts +++ b/src/client/datascience/interactive-common/intellisense/intellisenseProvider.ts @@ -36,7 +36,8 @@ import { IInteractiveWindowListener, IInteractiveWindowProvider, IJupyterExecution, - INotebook + INotebook, + INotebookCompletion } from '../../types'; import { ICancelIntellisenseRequest, @@ -470,13 +471,7 @@ export class IntellisenseProvider implements IInteractiveWindowListener { const jupyterResults = await activeNotebook.getCompletion(data.text, offsetInCode, cancelToken); if (jupyterResults && jupyterResults.matches) { - // If the line we're analyzing is empty or a whitespace, we filter out the magic commands - // as its confusing to see them appear after a . or inside (). - const pos = document.convertToDocumentPosition(cellId, position.lineNumber, position.column); - const line = document.lineAt(pos); - const filteredMatches = line.isEmptyOrWhitespace - ? jupyterResults.matches - : jupyterResults.matches.filter(match => !match.startsWith('%')); + const filteredMatches = this.filterJupyterMatches(document, jupyterResults, cellId, position); const baseOffset = data.offset; const basePosition = document.positionAt(baseOffset); @@ -507,6 +502,21 @@ export class IntellisenseProvider implements IInteractiveWindowListener { }; } + private filterJupyterMatches( + document: IntellisenseDocument, + jupyterResults: INotebookCompletion, + cellId: string, + position: monacoEditor.Position + ) { + // If the line we're analyzing is empty or a whitespace, we filter out the magic commands + // as its confusing to see them appear after a . or inside (). + const pos = document.convertToDocumentPosition(cellId, position.lineNumber, position.column); + const line = document.lineAt(pos); + return line.isEmptyOrWhitespace + ? jupyterResults.matches + : jupyterResults.matches.filter(match => !match.startsWith('%')); + } + private postTimedResponse( promises: Promise[], message: T, diff --git a/src/test/datascience/intellisense.functional.test.tsx b/src/test/datascience/intellisense.functional.test.tsx index bf4a8843ff4f..43ff7d806050 100644 --- a/src/test/datascience/intellisense.functional.test.tsx +++ b/src/test/datascience/intellisense.functional.test.tsx @@ -12,7 +12,7 @@ import { MonacoEditor } from '../../datascience-ui/react-common/monacoEditor'; import { noop } from '../core'; import { DataScienceIocContainer } from './dataScienceIocContainer'; import { getOrCreateInteractiveWindow, runMountedTest } from './interactiveWindowTestHelpers'; -import { getInteractiveEditor, typeCode } from './testHelpers'; +import { getInteractiveEditor, pressCtrlSpace, typeCode } from './testHelpers'; // tslint:disable:max-func-body-length trailing-comma no-any no-multiline-string suite('DataScience Intellisense tests', () => { @@ -69,6 +69,14 @@ suite('DataScience Intellisense tests', () => { assert.ok(innerTexts.includes(expectedSpan), 'Intellisense row not matching'); } + function verifyIntellisenseNotVisible( + wrapper: ReactWrapper, React.Component>, + expectedSpan: string + ) { + const innerTexts = getIntellisenseTextLines(wrapper); + assert.ok(!innerTexts.includes(expectedSpan), 'Intellisense row is showing'); + } + function waitForSuggestion( wrapper: ReactWrapper, React.Component> ): { disposable: IDisposable; promise: Promise } { @@ -230,4 +238,60 @@ suite('DataScience Intellisense tests', () => { return ioc; } ); + + runMountedTest( + 'Filtered Jupyter autocomplete, verify magic commands appear', + async wrapper => { + if (ioc.mockJupyter) { + // This test only works when mocking. + + // Create an interactive window so that it listens to the results. + const interactiveWindow = await getOrCreateInteractiveWindow(ioc); + await interactiveWindow.show(); + + // Then enter some code. Don't submit, we're just testing that autocomplete appears + const suggestion = waitForSuggestion(wrapper); + typeCode(getInteractiveEditor(wrapper), 'print'); + pressCtrlSpace(wrapper); + await suggestion.promise; + suggestion.disposable.dispose(); + verifyIntellisenseNotVisible(wrapper, '%%bash'); + + // Force suggestion box to disappear so that shutdown doesn't try to generate suggestions + // while we're destroying the editor. + clearEditor(wrapper); + } + }, + () => { + return ioc; + } + ); + + runMountedTest( + 'Filtered Jupyter autocomplete, verify magic commands are filtered', + async wrapper => { + if (ioc.mockJupyter) { + // This test only works when mocking. + + // Create an interactive window so that it listens to the results. + const interactiveWindow = await getOrCreateInteractiveWindow(ioc); + await interactiveWindow.show(); + + // Then enter some code. Don't submit, we're just testing that autocomplete appears + const suggestion = waitForSuggestion(wrapper); + typeCode(getInteractiveEditor(wrapper), ' '); + pressCtrlSpace(wrapper); + await suggestion.promise; + suggestion.disposable.dispose(); + verifyIntellisenseVisible(wrapper, '%%bash'); + + // Force suggestion box to disappear so that shutdown doesn't try to generate suggestions + // while we're destroying the editor. + clearEditor(wrapper); + } + }, + () => { + return ioc; + } + ); }); diff --git a/src/test/datascience/mockJupyterSession.ts b/src/test/datascience/mockJupyterSession.ts index 1b6485006d44..16c0caf30608 100644 --- a/src/test/datascience/mockJupyterSession.ts +++ b/src/test/datascience/mockJupyterSession.ts @@ -142,7 +142,7 @@ export class MockJupyterSession implements IJupyterSession { return { content: { - matches: ['printly'], // This keeps this in the intellisense when the editor pairs down results + matches: ['printly', '%%bash'], // This keeps this in the intellisense when the editor pairs down results cursor_start: 0, cursor_end: 7, status: 'ok', diff --git a/src/test/datascience/testHelpers.tsx b/src/test/datascience/testHelpers.tsx index d1c14722c323..6f3eadcf1236 100644 --- a/src/test/datascience/testHelpers.tsx +++ b/src/test/datascience/testHelpers.tsx @@ -677,6 +677,17 @@ export function typeCode( return textArea; } +export function pressCtrlSpace(editorControl: ReactWrapper, React.Component> | undefined) { + const textArea = getTextArea(editorControl); + assert.ok(textArea!, 'Cannot find the textarea inside the monaco editor'); + textArea!.focus(); + + const keyCode = ' '; + enterKey(textArea!, keyCode, false, true); + + return textArea; +} + function getTextArea( editorControl: ReactWrapper, React.Component> | undefined ): HTMLTextAreaElement | null { From 4ac30b5fa8bbafadc50657c0e084302564f2fa44 Mon Sep 17 00:00:00 2001 From: David Kutugata Date: Tue, 10 Mar 2020 13:48:12 -0700 Subject: [PATCH 4/5] removed pressCtrlSpace function --- src/test/datascience/intellisense.functional.test.tsx | 6 +++--- src/test/datascience/testHelpers.tsx | 11 ----------- 2 files changed, 3 insertions(+), 14 deletions(-) diff --git a/src/test/datascience/intellisense.functional.test.tsx b/src/test/datascience/intellisense.functional.test.tsx index 43ff7d806050..c8b781b263bb 100644 --- a/src/test/datascience/intellisense.functional.test.tsx +++ b/src/test/datascience/intellisense.functional.test.tsx @@ -12,7 +12,7 @@ import { MonacoEditor } from '../../datascience-ui/react-common/monacoEditor'; import { noop } from '../core'; import { DataScienceIocContainer } from './dataScienceIocContainer'; import { getOrCreateInteractiveWindow, runMountedTest } from './interactiveWindowTestHelpers'; -import { getInteractiveEditor, pressCtrlSpace, typeCode } from './testHelpers'; +import { enterEditorKey, getInteractiveEditor, typeCode } from './testHelpers'; // tslint:disable:max-func-body-length trailing-comma no-any no-multiline-string suite('DataScience Intellisense tests', () => { @@ -252,7 +252,7 @@ suite('DataScience Intellisense tests', () => { // Then enter some code. Don't submit, we're just testing that autocomplete appears const suggestion = waitForSuggestion(wrapper); typeCode(getInteractiveEditor(wrapper), 'print'); - pressCtrlSpace(wrapper); + enterEditorKey(wrapper, { code: ' ', ctrlKey: true }); await suggestion.promise; suggestion.disposable.dispose(); verifyIntellisenseNotVisible(wrapper, '%%bash'); @@ -280,7 +280,7 @@ suite('DataScience Intellisense tests', () => { // Then enter some code. Don't submit, we're just testing that autocomplete appears const suggestion = waitForSuggestion(wrapper); typeCode(getInteractiveEditor(wrapper), ' '); - pressCtrlSpace(wrapper); + enterEditorKey(wrapper, { code: ' ', ctrlKey: true }); await suggestion.promise; suggestion.disposable.dispose(); verifyIntellisenseVisible(wrapper, '%%bash'); diff --git a/src/test/datascience/testHelpers.tsx b/src/test/datascience/testHelpers.tsx index 6f3eadcf1236..d1c14722c323 100644 --- a/src/test/datascience/testHelpers.tsx +++ b/src/test/datascience/testHelpers.tsx @@ -677,17 +677,6 @@ export function typeCode( return textArea; } -export function pressCtrlSpace(editorControl: ReactWrapper, React.Component> | undefined) { - const textArea = getTextArea(editorControl); - assert.ok(textArea!, 'Cannot find the textarea inside the monaco editor'); - textArea!.focus(); - - const keyCode = ' '; - enterKey(textArea!, keyCode, false, true); - - return textArea; -} - function getTextArea( editorControl: ReactWrapper, React.Component> | undefined ): HTMLTextAreaElement | null { From a6a1736c59b3135e8945e78c49af6c590774a249 Mon Sep 17 00:00:00 2001 From: David Kutugata Date: Tue, 10 Mar 2020 14:13:24 -0700 Subject: [PATCH 5/5] added comments --- .../interactive-common/intellisense/intellisenseProvider.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/client/datascience/interactive-common/intellisense/intellisenseProvider.ts b/src/client/datascience/interactive-common/intellisense/intellisenseProvider.ts index 0712c78dd68e..b9882cfa32d7 100644 --- a/src/client/datascience/interactive-common/intellisense/intellisenseProvider.ts +++ b/src/client/datascience/interactive-common/intellisense/intellisenseProvider.ts @@ -502,6 +502,8 @@ export class IntellisenseProvider implements IInteractiveWindowListener { }; } + // The suggestions that the kernel is giving always include magic commands. That is confusing to the user. + // This function is called by provideJupyterCompletionItems to filter those magic commands when not in an empty line of code. private filterJupyterMatches( document: IntellisenseDocument, jupyterResults: INotebookCompletion,