From a51796eb3b40f6522c07772e7875ffd686f77da7 Mon Sep 17 00:00:00 2001 From: Matthew Runyon Date: Mon, 27 Mar 2023 16:46:34 -0500 Subject: [PATCH 1/6] Add signature help and hover providers --- packages/components/scss/BaseStyleSheet.scss | 11 +- packages/console/src/ConsoleInput.tsx | 4 +- .../monaco/MonacoCompletionProvider.test.tsx | 98 ----- .../src/monaco/MonacoCompletionProvider.tsx | 134 ------- .../src/monaco/MonacoProviders.test.tsx | 231 ++++++++++++ .../console/src/monaco/MonacoProviders.tsx | 346 ++++++++++++++++++ packages/console/src/monaco/index.ts | 2 +- .../console/src/notebook/ScriptEditor.tsx | 4 +- packages/jsapi-types/src/dh.types.ts | 26 +- 9 files changed, 617 insertions(+), 239 deletions(-) delete mode 100644 packages/console/src/monaco/MonacoCompletionProvider.test.tsx delete mode 100644 packages/console/src/monaco/MonacoCompletionProvider.tsx create mode 100644 packages/console/src/monaco/MonacoProviders.test.tsx create mode 100644 packages/console/src/monaco/MonacoProviders.tsx diff --git a/packages/components/scss/BaseStyleSheet.scss b/packages/components/scss/BaseStyleSheet.scss index 68e1e1c703..1a63cfd69d 100644 --- a/packages/components/scss/BaseStyleSheet.scss +++ b/packages/components/scss/BaseStyleSheet.scss @@ -677,10 +677,19 @@ input[type='number']::-webkit-inner-spin-button { .monaco-editor { // Hide the "Loading..." and "No suggestions" message in the suggest widget in monaco to make it feel faster - .editor-widget.suggest-widget.message { + .editor-widget.suggest-widget.message, + .parameter-hints-widget.message { display: none; } + .monaco-hover hr { + margin-bottom: 4px !important; + } + + .parameter-hints-widget { + z-index: 30 !important; // Need to make above golden-layout, but below completion item widget at z=40 + } + .find-widget { &.visible { // For some reason the height of this is set improperly in 0.18.1 diff --git a/packages/console/src/ConsoleInput.tsx b/packages/console/src/ConsoleInput.tsx index f2ff811e24..292a494d17 100644 --- a/packages/console/src/ConsoleInput.tsx +++ b/packages/console/src/ConsoleInput.tsx @@ -14,7 +14,7 @@ import { CommandHistoryStorageItem, CommandHistoryTable, } from './command-history'; -import { MonacoCompletionProvider, MonacoTheme, MonacoUtils } from './monaco'; +import { MonacoProviders, MonacoTheme, MonacoUtils } from './monaco'; import './ConsoleInput.scss'; const log = Log.module('ConsoleInput'); @@ -513,7 +513,7 @@ export class ConsoleInput extends PureComponent< style={{ height: commandEditorHeight }} /> {model && ( - 1 } -) { - const wrapper = render( - - ); - - return wrapper; -} - -it('renders without crashing', () => { - const disposable = { dispose: jest.fn() }; - monaco.languages.registerCompletionItemProvider = jest.fn(() => disposable); - makeCompletionProvider(); -}); - -it('registers/deregisters completion provider properly', () => { - const disposable = { dispose: jest.fn() }; - - monaco.languages.registerCompletionItemProvider = jest.fn(() => disposable); - - const wrapper = makeCompletionProvider(); - - expect(monaco.languages.registerCompletionItemProvider).toHaveBeenCalledWith( - DEFAULT_LANGUAGE, - expect.objectContaining({ provideCompletionItems: expect.anything() }) - ); - expect(disposable.dispose).not.toHaveBeenCalled(); - - wrapper.unmount(); - - expect(disposable.dispose).toHaveBeenCalledTimes(1); -}); - -it('provides completion items properly', () => { - const disposable = { dispose: jest.fn() }; - const newText = 'test'; - const myRegister = jest.fn(() => disposable); - monaco.languages.registerCompletionItemProvider = myRegister; - const items = [ - { - label: newText, - kind: 0, - textEdit: { - range: { - start: { line: 5, character: 30 }, - end: { line: 10, character: 60 }, - }, - text: newText, - }, - }, - ]; - const promiseItems = Promise.resolve(items); - const language = DEFAULT_LANGUAGE; - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const session = new (dh as any).IdeSession(language); - session.getCompletionItems = jest.fn(() => promiseItems); - - const model = { uri: { path: 'test' }, getVersionId: jest.fn(() => 1) }; - makeCompletionProvider(language, session, model); - const position = { lineNumber: 1, column: 1 }; - expect(myRegister).toHaveBeenCalledTimes(1); - expect.assertions(4); - - const calls: { - provideCompletionItems: ( - model: unknown, - position: unknown - ) => Promise<{ suggestions: unknown[] }>; - }[] = myRegister.mock.calls[0]; - const fn = calls[1]; - - return fn.provideCompletionItems(model, position).then(resultItems => { - expect(session.getCompletionItems).toHaveBeenCalled(); - - const { suggestions } = resultItems; - expect(suggestions.length).toBe(items.length); - expect(suggestions[0]).toMatchObject({ - insertText: newText, - label: newText, - }); - }); -}); diff --git a/packages/console/src/monaco/MonacoCompletionProvider.tsx b/packages/console/src/monaco/MonacoCompletionProvider.tsx deleted file mode 100644 index f09a98d192..0000000000 --- a/packages/console/src/monaco/MonacoCompletionProvider.tsx +++ /dev/null @@ -1,134 +0,0 @@ -/** - * Completion provider for a code session - */ -import { PureComponent } from 'react'; -import * as monaco from 'monaco-editor'; -import Log from '@deephaven/log'; -import { IdeSession } from '@deephaven/jsapi-shim'; - -const log = Log.module('MonacoCompletionProvider'); - -interface MonacoCompletionProviderProps { - model: monaco.editor.ITextModel; - session: IdeSession; - language: string; -} - -/** - * Registers a completion provider with monaco for the language and session provided. - */ -class MonacoCompletionProvider extends PureComponent< - MonacoCompletionProviderProps, - Record -> { - constructor(props: MonacoCompletionProviderProps) { - super(props); - - this.handleCompletionRequest = this.handleCompletionRequest.bind(this); - } - - componentDidMount(): void { - const { language } = this.props; - this.registeredCompletionProvider = monaco.languages.registerCompletionItemProvider( - language ?? '', - { - provideCompletionItems: this.handleCompletionRequest, - triggerCharacters: ['"', '.', "'", '(', '[', '{', ','], - } - ); - } - - componentWillUnmount(): void { - this.registeredCompletionProvider?.dispose(); - } - - registeredCompletionProvider?: monaco.IDisposable; - - handleCompletionRequest( - model: monaco.editor.ITextModel, - position: monaco.Position, - context: monaco.languages.CompletionContext - ): monaco.languages.ProviderResult { - const { model: propModel, session } = this.props; - if (model !== propModel) { - return null; - } - - const params = { - textDocument: { - uri: `${model.uri}`, - version: model.getVersionId(), - }, - // Yes, the Monaco API Position is different than the Position received by the LSP - // Why? I do not know. - position: { - line: position.lineNumber - 1, - character: position.column - 1, - }, - context, - }; - - const completionItems = session.getCompletionItems(params); - - log.debug('Completion items received: ', params, completionItems); - - const monacoCompletionItems = completionItems - .then(items => { - // Annoying that the LSP protocol returns completion items with a range that's slightly different than what Monaco expects - // Need to remap the items here - const suggestions = items.map(item => { - const { - label, - kind, - detail, - documentation, - sortText, - filterText, - textEdit, - insertTextFormat, - } = item; - - const { start, end } = textEdit.range; - - // Monaco expects the columns/ranges to start at 1. - const range = { - startLineNumber: start.line + 1, - startColumn: start.character + 1, - endLineNumber: end.line + 1, - endColumn: end.character + 1, - }; - return { - label, - kind, - detail, - documentation, - sortText, - filterText, - insertText: textEdit.text, - // We are basically guessing that LSP's insertTextFormat===2 is - // semantically equivalent to monaco's insertTextRules===4. - // Why microsoft is using almost-but-not-LSP apis is beyond me.... - insertTextRules: insertTextFormat === 2 ? 4 : insertTextFormat, - range, - }; - }); - - return { - incomplete: true, - suggestions, - }; - }) - .catch((error: unknown) => { - log.error('There was an error retrieving completion items', error); - return { suggestions: [] }; - }); - - return monacoCompletionItems; - } - - render(): null { - return null; - } -} - -export default MonacoCompletionProvider; diff --git a/packages/console/src/monaco/MonacoProviders.test.tsx b/packages/console/src/monaco/MonacoProviders.test.tsx new file mode 100644 index 0000000000..1777e78144 --- /dev/null +++ b/packages/console/src/monaco/MonacoProviders.test.tsx @@ -0,0 +1,231 @@ +import React from 'react'; +import { render } from '@testing-library/react'; +import * as monaco from 'monaco-editor'; +import dh from '@deephaven/jsapi-shim'; +import MonacoProviders from './MonacoProviders'; + +const DEFAULT_LANGUAGE = 'test'; + +function makeProviders( + language = DEFAULT_LANGUAGE, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + session = new (dh as any).IdeSession(language), + model = { uri: {}, getVersionId: () => 1 } +) { + const wrapper = render( + + ); + + return wrapper; +} + +it('renders without crashing', () => { + const disposable = { dispose: jest.fn() }; + monaco.languages.registerCompletionItemProvider = jest.fn(() => disposable); + makeProviders(); +}); + +it('registers/deregisters completion provider properly', () => { + const disposable = { dispose: jest.fn() }; + + monaco.languages.registerCompletionItemProvider = jest.fn(() => disposable); + + const wrapper = makeProviders(); + + expect(monaco.languages.registerCompletionItemProvider).toHaveBeenCalledWith( + DEFAULT_LANGUAGE, + expect.objectContaining({ provideCompletionItems: expect.anything() }) + ); + expect(disposable.dispose).not.toHaveBeenCalled(); + + wrapper.unmount(); + + expect(disposable.dispose).toHaveBeenCalledTimes(1); +}); + +it('provides completion items properly', async () => { + const disposable = { dispose: jest.fn() }; + const newText = 'test'; + const myRegister = jest.fn(() => disposable); + monaco.languages.registerCompletionItemProvider = myRegister; + const items = [ + { + label: newText, + kind: 0, + textEdit: { + range: { + start: { line: 5, character: 30 }, + end: { line: 10, character: 60 }, + }, + text: newText, + }, + }, + ]; + const promiseItems = Promise.resolve(items); + const language = DEFAULT_LANGUAGE; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const session = new (dh as any).IdeSession(language); + session.getCompletionItems = jest.fn(() => promiseItems); + + const model = { uri: { path: 'test' }, getVersionId: jest.fn(() => 1) }; + makeProviders(language, session, model); + const position = { lineNumber: 1, column: 1 }; + expect(myRegister).toHaveBeenCalledTimes(1); + expect.assertions(4); + + const calls: { + provideCompletionItems: ( + model: unknown, + position: unknown + ) => Promise<{ suggestions: unknown[] }>; + }[] = myRegister.mock.calls[0]; + const fn = calls[1]; + + const resultItems = await fn.provideCompletionItems(model, position); + expect(session.getCompletionItems).toHaveBeenCalled(); + const { suggestions } = resultItems; + expect(suggestions.length).toBe(items.length); + expect(suggestions[0]).toMatchObject({ + insertText: newText, + label: newText, + }); +}); + +it('registers/deregisters signature help provider properly', () => { + const disposable = { dispose: jest.fn() }; + + monaco.languages.registerSignatureHelpProvider = jest.fn(() => disposable); + + const wrapper = makeProviders(); + + expect(monaco.languages.registerSignatureHelpProvider).toHaveBeenCalledWith( + DEFAULT_LANGUAGE, + expect.objectContaining({ provideSignatureHelp: expect.anything() }) + ); + expect(disposable.dispose).not.toHaveBeenCalled(); + + wrapper.unmount(); + + expect(disposable.dispose).toHaveBeenCalledTimes(1); +}); + +it('provides signature help properly', async () => { + const disposable = { dispose: jest.fn() }; + const newText = 'test'; + const myRegister = jest.fn(() => disposable); + monaco.languages.registerSignatureHelpProvider = myRegister; + const items = [ + { + label: newText, + documentation: { kind: 'plaintext', value: newText }, + parameters: [ + { + label: 'param', + documentation: { kind: 'plaintext', value: 'paramdoc' }, + }, + ], + activeParameter: 0, + }, + ]; + const promiseItems = Promise.resolve(items); + const language = DEFAULT_LANGUAGE; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const session = new (dh as any).IdeSession(language); + session.getSignatureHelp = jest.fn(() => promiseItems); + + const model = { uri: { path: 'test' }, getVersionId: jest.fn(() => 1) }; + makeProviders(language, session, model); + const position = { lineNumber: 1, column: 1 }; + expect.assertions(6); + expect(myRegister).toHaveBeenCalledTimes(1); + + const calls: { + provideSignatureHelp: ( + model: unknown, + position: unknown + ) => Promise<{ + value: { + signatures: unknown[]; + activeSignature: number; + activeParameter: number; + }; + }>; + }[] = myRegister.mock.calls[0]; + const fn = calls[1]; + + const resultItems = await fn.provideSignatureHelp(model, position); + expect(session.getSignatureHelp).toHaveBeenCalled(); + const { value } = resultItems; + const { signatures, activeSignature, activeParameter } = value; + expect(signatures.length).toBe(items.length); + expect(signatures[0]).toMatchObject({ + documentation: newText, + label: newText, + parameters: items[0].parameters, + }); + expect(activeSignature).toBe(0); + expect(activeParameter).toBe(0); +}); + +it('registers/deregisters hover provider properly', () => { + const disposable = { dispose: jest.fn() }; + + monaco.languages.registerHoverProvider = jest.fn(() => disposable); + + const wrapper = makeProviders(); + + expect(monaco.languages.registerHoverProvider).toHaveBeenCalledWith( + DEFAULT_LANGUAGE, + expect.objectContaining({ provideHover: expect.anything() }) + ); + expect(disposable.dispose).not.toHaveBeenCalled(); + + wrapper.unmount(); + + expect(disposable.dispose).toHaveBeenCalledTimes(1); +}); + +it('provides hover info properly', async () => { + const disposable = { dispose: jest.fn() }; + const newText = 'test'; + const myRegister = jest.fn(() => disposable); + monaco.languages.registerHoverProvider = myRegister; + const items = { + contents: { + value: newText, + }, + }; + const promiseItems = Promise.resolve(items); + const language = DEFAULT_LANGUAGE; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const session = new (dh as any).IdeSession(language); + session.getHover = jest.fn(() => promiseItems); + + const model = { uri: { path: 'test' }, getVersionId: jest.fn(() => 1) }; + makeProviders(language, session, model); + const position = { lineNumber: 1, column: 1 }; + expect(myRegister).toHaveBeenCalledTimes(1); + expect.assertions(3); + + const calls: { + provideHover: ( + model: unknown, + position: unknown + ) => Promise<{ + contents: { + value: string; + }[]; + }>; + }[] = myRegister.mock.calls[0]; + const fn = calls[1]; + + const { contents } = await fn.provideHover(model, position); + expect(session.getHover).toHaveBeenCalled(); + expect(contents[0]).toMatchObject({ + value: newText, + }); +}); diff --git a/packages/console/src/monaco/MonacoProviders.tsx b/packages/console/src/monaco/MonacoProviders.tsx new file mode 100644 index 0000000000..36061c742a --- /dev/null +++ b/packages/console/src/monaco/MonacoProviders.tsx @@ -0,0 +1,346 @@ +/** + * Completion provider for a code session + */ +import { PureComponent } from 'react'; +import * as monaco from 'monaco-editor'; +import Log from '@deephaven/log'; +import { IdeSession } from '@deephaven/jsapi-shim'; + +const log = Log.module('MonacoCompletionProvider'); + +interface MonacoProviderProps { + model: monaco.editor.ITextModel; + session: IdeSession; + language: string; +} + +/** + * Registers a completion provider with monaco for the language and session provided. + */ +class MonacoProviders extends PureComponent< + MonacoProviderProps, + Record +> { + /** + * Converts LSP CompletionItemKind to Monaco CompletionItemKind + * @param kind The LSP kind + * @returns Monaco kind + */ + static lspToMonacoKind(kind: number): number { + const monacoKinds = monaco.languages.CompletionItemKind; + switch (kind) { + case 1: + return monacoKinds.Text; + case 2: + return monacoKinds.Method; + case 3: + return monacoKinds.Function; + case 4: + return monacoKinds.Constructor; + case 5: + return monacoKinds.Field; + case 6: + return monacoKinds.Variable; + case 7: + return monacoKinds.Class; + case 8: + return monacoKinds.Interface; + case 9: + return monacoKinds.Module; + case 10: + return monacoKinds.Property; + case 11: + return monacoKinds.Unit; + case 12: + return monacoKinds.Value; + case 13: + return monacoKinds.Enum; + case 14: + return monacoKinds.Keyword; + case 15: + return monacoKinds.Snippet; + case 16: + return monacoKinds.Color; + case 17: + return monacoKinds.File; + case 18: + return monacoKinds.Reference; + case 19: + return monacoKinds.Folder; + case 20: + return monacoKinds.EnumMember; + case 21: + return monacoKinds.Constant; + case 22: + return monacoKinds.Struct; + case 23: + return monacoKinds.Event; + case 24: + return monacoKinds.Operator; + case 25: + return monacoKinds.TypeParameter; + default: + return monacoKinds.Variable; + } + } + + constructor(props: MonacoProviderProps) { + super(props); + + this.handleCompletionRequest = this.handleCompletionRequest.bind(this); + this.handleSignatureRequest = this.handleSignatureRequest.bind(this); + this.handleHoverRequest = this.handleHoverRequest.bind(this); + } + + componentDidMount(): void { + const { language } = this.props; + + this.registeredCompletionProvider = monaco.languages.registerCompletionItemProvider( + language ?? '', + { + provideCompletionItems: this.handleCompletionRequest, + triggerCharacters: ['.'], + } + ); + + this.registeredSignatureProvider = monaco.languages.registerSignatureHelpProvider( + language ?? '', + { + provideSignatureHelp: this.handleSignatureRequest, + signatureHelpTriggerCharacters: ['(', ','], + } + ); + + this.registeredHoverProvider = monaco.languages.registerHoverProvider( + language ?? '', + { + provideHover: this.handleHoverRequest, + } + ); + } + + componentWillUnmount(): void { + this.registeredCompletionProvider?.dispose(); + this.registeredSignatureProvider?.dispose(); + this.registeredHoverProvider?.dispose(); + } + + registeredCompletionProvider?: monaco.IDisposable; + + registeredSignatureProvider?: monaco.IDisposable; + + registeredHoverProvider?: monaco.IDisposable; + + handleCompletionRequest( + model: monaco.editor.ITextModel, + position: monaco.Position, + context: monaco.languages.CompletionContext + ): monaco.languages.ProviderResult { + const { model: propModel, session } = this.props; + if (model !== propModel) { + return null; + } + + const params = { + textDocument: { + uri: `${model.uri}`, + version: model.getVersionId(), + }, + // Monaco 1-indexes Position. LSP 0-indexes Position + position: { + line: position.lineNumber - 1, + character: position.column - 1, + }, + context, + }; + + const completionItems = session.getCompletionItems(params); + + const monacoCompletionItems = completionItems + .then(items => { + log.debug('Completion items received: ', params, completionItems); + + // Annoying that the LSP protocol returns completion items with a range that's slightly different than what Monaco expects + // Need to remap the items here + const suggestions = items.map(item => { + const { + label, + kind, + detail, + documentation, + sortText, + filterText, + textEdit, + insertTextFormat, + } = item; + + const { start, end } = textEdit.range; + + // Monaco expects the columns/ranges to start at 1. + const range = { + startLineNumber: start.line + 1, + startColumn: start.character + 1, + endLineNumber: end.line + 1, + endColumn: end.character + 1, + }; + + return { + label, + kind: MonacoProviders.lspToMonacoKind(kind), + detail, + documentation: + documentation?.kind === 'markdown' + ? documentation + : documentation?.value, + sortText, + filterText, + insertText: textEdit.text, + // We are basically guessing that LSP's insertTextFormat===2 is + // semantically equivalent to monaco's insertTextRules===4. + // Why microsoft is using almost-but-not-LSP apis is beyond me.... + insertTextRules: insertTextFormat === 2 ? 4 : insertTextFormat, + range, + }; + }); + + return { + incomplete: true, + suggestions, + }; + }) + .catch((error: unknown) => { + log.error('There was an error retrieving completion items', error); + return { suggestions: [] }; + }); + + return monacoCompletionItems; + } + + // eslint-disable-next-line class-methods-use-this + handleSignatureRequest( + model: monaco.editor.ITextModel, + position: monaco.Position, + token: monaco.CancellationToken, + context: monaco.languages.SignatureHelpContext + ): monaco.languages.ProviderResult { + const defaultResult: monaco.languages.SignatureHelpResult = { + value: { + signatures: [], + activeSignature: 0, + activeParameter: 0, + }, + dispose: () => { + /* no-op */ + }, + }; + + const { model: propModel, session } = this.props; + if (model !== propModel || !session.getSignatureHelp) { + return null; + } + + const params = { + textDocument: { + uri: `${model.uri}`, + version: model.getVersionId(), + }, + // Monaco 1-indexes Position. LSP 0-indexes Position + position: { + line: position.lineNumber - 1, + character: position.column - 1, + }, + context, + }; + + const signatureItems = session.getSignatureHelp(params); + + const monacoSignatures = signatureItems + .then(items => { + log.debug('Signatures received: ', params, signatureItems); + const signatures = items.map(item => { + const { label, documentation, parameters } = item; + + return { + documentation: + documentation?.kind === 'markdown' + ? documentation + : documentation?.value, + label, + parameters: parameters ?? [], + }; + }); + + if (signatures.length === 0) { + return defaultResult; + } + + // For now we will assume we only autocomplete Python w/ 1 signature + // For multiple signatures, this may need to be sent through the request as context + const activeSignature = 0; + + return { + value: { + signatures, + activeSignature, + activeParameter: items[activeSignature].activeParameter, + }, + dispose: () => { + /* no-op */ + }, + }; + }) + .catch((error: unknown) => { + log.error('There was an error retrieving completion items', error); + return defaultResult; + }); + + return monacoSignatures; + } + + // eslint-disable-next-line class-methods-use-this + handleHoverRequest( + model: monaco.editor.ITextModel, + position: monaco.Position + ): monaco.languages.ProviderResult { + const { model: propModel, session } = this.props; + if (model !== propModel || !session.getHover) { + return null; + } + + const params = { + textDocument: { + uri: `${model.uri}`, + version: model.getVersionId(), + }, + // Monaco 1-indexes Position. LSP 0-indexes Position + position: { + line: position.lineNumber - 1, + character: position.column - 1, + }, + }; + + const hover = session.getHover(params); + + const monacoHover = hover + .then(hoverItem => { + log.debug('Hover received: ', params, hoverItem); + const { contents: hoverContents } = hoverItem; + + return { + contents: hoverContents ? [hoverContents] : [], + }; + }) + .catch((error: unknown) => { + log.error('There was an error retrieving hover', error); + return { contents: [] }; + }); + + return monacoHover; + } + + render(): null { + return null; + } +} + +export default MonacoProviders; diff --git a/packages/console/src/monaco/index.ts b/packages/console/src/monaco/index.ts index e13c1fe28d..98cf56f2af 100644 --- a/packages/console/src/monaco/index.ts +++ b/packages/console/src/monaco/index.ts @@ -1,3 +1,3 @@ export { default as MonacoUtils } from './MonacoUtils'; -export { default as MonacoCompletionProvider } from './MonacoCompletionProvider'; +export { default as MonacoProviders } from './MonacoProviders'; export { default as MonacoTheme } from './MonacoTheme.module.scss'; diff --git a/packages/console/src/notebook/ScriptEditor.tsx b/packages/console/src/notebook/ScriptEditor.tsx index 8a22319d88..8b00caf5ca 100644 --- a/packages/console/src/notebook/ScriptEditor.tsx +++ b/packages/console/src/notebook/ScriptEditor.tsx @@ -8,7 +8,7 @@ import { IdeSession } from '@deephaven/jsapi-shim'; import { assertNotNull } from '@deephaven/utils'; import { editor, IDisposable } from 'monaco-editor'; import Editor from './Editor'; -import { MonacoCompletionProvider, MonacoUtils } from '../monaco'; +import { MonacoProviders, MonacoUtils } from '../monaco'; import './ScriptEditor.scss'; import SHORTCUTS from '../ConsoleShortcuts'; @@ -366,7 +366,7 @@ class ScriptEditor extends Component { /> {completionProviderEnabled != null && completionProviderEnabled && ( - ; getCompletionItems(params: unknown): Promise; + getSignatureHelp?(params: unknown): Promise; + getHover?(params: unknown): Promise; closeDocument(params: unknown): void; openDocument(params: unknown): void; changeDocument(params: unknown): void; From cbb36ad8fa5779bd46edad98bdbe78aa223086ee Mon Sep 17 00:00:00 2001 From: Matthew Runyon Date: Mon, 27 Mar 2023 16:47:13 -0500 Subject: [PATCH 2/6] Add css comment --- packages/components/scss/BaseStyleSheet.scss | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/components/scss/BaseStyleSheet.scss b/packages/components/scss/BaseStyleSheet.scss index 1a63cfd69d..06bb64d500 100644 --- a/packages/components/scss/BaseStyleSheet.scss +++ b/packages/components/scss/BaseStyleSheet.scss @@ -683,7 +683,7 @@ input[type='number']::-webkit-inner-spin-button { } .monaco-hover hr { - margin-bottom: 4px !important; + margin-bottom: 4px !important; // Monaco sets this to -4 which causes items below a dividing line to collide w/ the line } .parameter-hints-widget { From e0873dcec0e4fb363aa52e3c6ef5da89a302bca8 Mon Sep 17 00:00:00 2001 From: Matthew Runyon Date: Wed, 29 Mar 2023 13:04:24 -0500 Subject: [PATCH 3/6] Address review comments --- __mocks__/dh-core.js | 6 + .../src/monaco/MonacoProviders.test.tsx | 115 +++++++++++++++++- .../console/src/monaco/MonacoProviders.tsx | 114 +++++++++-------- 3 files changed, 185 insertions(+), 50 deletions(-) diff --git a/__mocks__/dh-core.js b/__mocks__/dh-core.js index 2f748b908e..364209f69d 100644 --- a/__mocks__/dh-core.js +++ b/__mocks__/dh-core.js @@ -1418,6 +1418,12 @@ class IdeSession extends DeephavenObject { getCompletionItems() { return Promise.resolve([]); } + getSignatureHelp() { + return Promise.resolve([]); + } + getHover() { + return Promise.resolve({}); + } closeDocument() {} newTable(columnNames, types, data, userTimeZone) { diff --git a/packages/console/src/monaco/MonacoProviders.test.tsx b/packages/console/src/monaco/MonacoProviders.test.tsx index 1777e78144..71ec1c7f70 100644 --- a/packages/console/src/monaco/MonacoProviders.test.tsx +++ b/packages/console/src/monaco/MonacoProviders.test.tsx @@ -47,7 +47,8 @@ it('registers/deregisters completion provider properly', () => { expect(disposable.dispose).toHaveBeenCalledTimes(1); }); -it('provides completion items properly', async () => { +// Enterprise provides no documentation. Community provides MarkupContent +it('provides completion items with no documentation object', async () => { const disposable = { dispose: jest.fn() }; const newText = 'test'; const myRegister = jest.fn(() => disposable); @@ -92,6 +93,82 @@ it('provides completion items properly', async () => { expect(suggestions[0]).toMatchObject({ insertText: newText, label: newText, + documentation: undefined, + }); +}); + +it('provides completion items properly with documentation object', async () => { + const disposable = { dispose: jest.fn() }; + const newText = 'test'; + const myRegister = jest.fn(() => disposable); + monaco.languages.registerCompletionItemProvider = myRegister; + const items = [ + { + label: newText, + kind: 0, + textEdit: { + range: { + start: { line: 5, character: 30 }, + end: { line: 10, character: 60 }, + }, + text: newText, + }, + documentation: { + value: 'markdown', + kind: 'markdown', + }, + }, + { + label: `${newText}2`, + kind: 0, + textEdit: { + range: { + start: { line: 5, character: 30 }, + end: { line: 10, character: 60 }, + }, + text: `${newText}2`, + }, + documentation: { + value: 'plaintext', + kind: 'plaintext', + }, + }, + ]; + const promiseItems = Promise.resolve(items); + const language = DEFAULT_LANGUAGE; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const session = new (dh as any).IdeSession(language); + session.getCompletionItems = jest.fn(() => promiseItems); + + const model = { uri: { path: 'test' }, getVersionId: jest.fn(() => 1) }; + makeProviders(language, session, model); + const position = { lineNumber: 1, column: 1 }; + expect(myRegister).toHaveBeenCalledTimes(1); + expect.assertions(5); + + const calls: { + provideCompletionItems: ( + model: unknown, + position: unknown + ) => Promise<{ suggestions: unknown[] }>; + }[] = myRegister.mock.calls[0]; + const fn = calls[1]; + + const resultItems = await fn.provideCompletionItems(model, position); + expect(session.getCompletionItems).toHaveBeenCalled(); + const { suggestions } = resultItems; + expect(suggestions.length).toBe(items.length); + expect(suggestions[0]).toMatchObject({ + insertText: newText, + label: newText, + documentation: { + value: 'markdown', + }, + }); + expect(suggestions[1]).toMatchObject({ + insertText: `${newText}2`, + label: `${newText}2`, + documentation: 'plaintext', }); }); @@ -113,6 +190,24 @@ it('registers/deregisters signature help provider properly', () => { expect(disposable.dispose).toHaveBeenCalledTimes(1); }); +it('does not register signature help if it is not available', () => { + const disposable = { dispose: jest.fn() }; + + monaco.languages.registerSignatureHelpProvider = jest.fn(() => disposable); + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const session = new (dh as any).IdeSession(DEFAULT_LANGUAGE); + session.getSignatureHelp = undefined; + + const wrapper = makeProviders(DEFAULT_LANGUAGE, session); + + expect(monaco.languages.registerSignatureHelpProvider).not.toHaveBeenCalled(); + + wrapper.unmount(); + + expect(disposable.dispose).not.toHaveBeenCalled(); +}); + it('provides signature help properly', async () => { const disposable = { dispose: jest.fn() }; const newText = 'test'; @@ -189,6 +284,24 @@ it('registers/deregisters hover provider properly', () => { expect(disposable.dispose).toHaveBeenCalledTimes(1); }); +it('does not register hover if it is not available', () => { + const disposable = { dispose: jest.fn() }; + + monaco.languages.registerHoverProvider = jest.fn(() => disposable); + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const session = new (dh as any).IdeSession(DEFAULT_LANGUAGE); + session.getHover = undefined; + + const wrapper = makeProviders(DEFAULT_LANGUAGE, session); + + expect(monaco.languages.registerHoverProvider).not.toHaveBeenCalled(); + + wrapper.unmount(); + + expect(disposable.dispose).not.toHaveBeenCalled(); +}); + it('provides hover info properly', async () => { const disposable = { dispose: jest.fn() }; const newText = 'test'; diff --git a/packages/console/src/monaco/MonacoProviders.tsx b/packages/console/src/monaco/MonacoProviders.tsx index 36061c742a..f4719cf434 100644 --- a/packages/console/src/monaco/MonacoProviders.tsx +++ b/packages/console/src/monaco/MonacoProviders.tsx @@ -4,7 +4,7 @@ import { PureComponent } from 'react'; import * as monaco from 'monaco-editor'; import Log from '@deephaven/log'; -import { IdeSession } from '@deephaven/jsapi-shim'; +import { DocumentRange, IdeSession, Position } from '@deephaven/jsapi-shim'; const log = Log.module('MonacoCompletionProvider'); @@ -23,10 +23,11 @@ class MonacoProviders extends PureComponent< > { /** * Converts LSP CompletionItemKind to Monaco CompletionItemKind + * https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#completionItemKind * @param kind The LSP kind * @returns Monaco kind */ - static lspToMonacoKind(kind: number): number { + static lspToMonacoKind(kind: number | undefined): number { const monacoKinds = monaco.languages.CompletionItemKind; switch (kind) { case 1: @@ -84,6 +85,40 @@ class MonacoProviders extends PureComponent< } } + /** + * Converts an LSP document range to a monaco range + * Accounts for LSP indexing from 0 and monaco indexing from 1 + * + * @param range The LSP document range to convert + * @returns The corresponding monaco range + */ + static lspToMonacoRange(range: DocumentRange): monaco.IRange { + const { start, end } = range; + + // Monaco expects the columns/ranges to start at 1. LSP starts at 0 + return { + startLineNumber: start.line + 1, + startColumn: start.character + 1, + endLineNumber: end.line + 1, + endColumn: end.character + 1, + }; + } + + /** + * Converts a monaco position to an LSP position + * Accounts for LSP indexing from 0 and monaco indexing from 1 + * + * @param position The monaco position + * @returns The corresponding LSP position + */ + static monacoToLspPosition(position: monaco.IPosition): Position { + // Monaco 1-indexes Position. LSP 0-indexes Position + return { + line: position.lineNumber - 1, + character: position.column - 1, + }; + } + constructor(props: MonacoProviderProps) { super(props); @@ -93,30 +128,34 @@ class MonacoProviders extends PureComponent< } componentDidMount(): void { - const { language } = this.props; + const { language, session } = this.props; this.registeredCompletionProvider = monaco.languages.registerCompletionItemProvider( - language ?? '', + language, { provideCompletionItems: this.handleCompletionRequest, - triggerCharacters: ['.'], + triggerCharacters: ['.', '"', "'", '/'], } ); - this.registeredSignatureProvider = monaco.languages.registerSignatureHelpProvider( - language ?? '', - { - provideSignatureHelp: this.handleSignatureRequest, - signatureHelpTriggerCharacters: ['(', ','], - } - ); + if (session.getSignatureHelp) { + this.registeredSignatureProvider = monaco.languages.registerSignatureHelpProvider( + language, + { + provideSignatureHelp: this.handleSignatureRequest, + signatureHelpTriggerCharacters: ['(', ','], + } + ); + } - this.registeredHoverProvider = monaco.languages.registerHoverProvider( - language ?? '', - { - provideHover: this.handleHoverRequest, - } - ); + if (session.getHover) { + this.registeredHoverProvider = monaco.languages.registerHoverProvider( + language, + { + provideHover: this.handleHoverRequest, + } + ); + } } componentWillUnmount(): void { @@ -146,22 +185,17 @@ class MonacoProviders extends PureComponent< uri: `${model.uri}`, version: model.getVersionId(), }, - // Monaco 1-indexes Position. LSP 0-indexes Position - position: { - line: position.lineNumber - 1, - character: position.column - 1, - }, + position: MonacoProviders.monacoToLspPosition(position), context, }; const completionItems = session.getCompletionItems(params); + log.debug('Requested completion items', params); const monacoCompletionItems = completionItems .then(items => { - log.debug('Completion items received: ', params, completionItems); + log.debug('Completion items received: ', params, items); - // Annoying that the LSP protocol returns completion items with a range that's slightly different than what Monaco expects - // Need to remap the items here const suggestions = items.map(item => { const { label, @@ -174,16 +208,6 @@ class MonacoProviders extends PureComponent< insertTextFormat, } = item; - const { start, end } = textEdit.range; - - // Monaco expects the columns/ranges to start at 1. - const range = { - startLineNumber: start.line + 1, - startColumn: start.character + 1, - endLineNumber: end.line + 1, - endColumn: end.character + 1, - }; - return { label, kind: MonacoProviders.lspToMonacoKind(kind), @@ -199,7 +223,7 @@ class MonacoProviders extends PureComponent< // semantically equivalent to monaco's insertTextRules===4. // Why microsoft is using almost-but-not-LSP apis is beyond me.... insertTextRules: insertTextFormat === 2 ? 4 : insertTextFormat, - range, + range: MonacoProviders.lspToMonacoRange(textEdit.range), }; }); @@ -216,7 +240,6 @@ class MonacoProviders extends PureComponent< return monacoCompletionItems; } - // eslint-disable-next-line class-methods-use-this handleSignatureRequest( model: monaco.editor.ITextModel, position: monaco.Position, @@ -244,15 +267,12 @@ class MonacoProviders extends PureComponent< uri: `${model.uri}`, version: model.getVersionId(), }, - // Monaco 1-indexes Position. LSP 0-indexes Position - position: { - line: position.lineNumber - 1, - character: position.column - 1, - }, + position: MonacoProviders.monacoToLspPosition(position), context, }; const signatureItems = session.getSignatureHelp(params); + log.debug('Requested signature help', params); const monacoSignatures = signatureItems .then(items => { @@ -297,7 +317,6 @@ class MonacoProviders extends PureComponent< return monacoSignatures; } - // eslint-disable-next-line class-methods-use-this handleHoverRequest( model: monaco.editor.ITextModel, position: monaco.Position @@ -312,14 +331,11 @@ class MonacoProviders extends PureComponent< uri: `${model.uri}`, version: model.getVersionId(), }, - // Monaco 1-indexes Position. LSP 0-indexes Position - position: { - line: position.lineNumber - 1, - character: position.column - 1, - }, + position: MonacoProviders.monacoToLspPosition(position), }; const hover = session.getHover(params); + log.debug('Requested hover', params); const monacoHover = hover .then(hoverItem => { From 7f0be174bc6b0c0450311ef09bd8e26f33ebd953 Mon Sep 17 00:00:00 2001 From: Matthew Runyon Date: Wed, 29 Mar 2023 13:05:01 -0500 Subject: [PATCH 4/6] Remove trigger character --- packages/console/src/monaco/MonacoProviders.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/console/src/monaco/MonacoProviders.tsx b/packages/console/src/monaco/MonacoProviders.tsx index f4719cf434..897b7257c9 100644 --- a/packages/console/src/monaco/MonacoProviders.tsx +++ b/packages/console/src/monaco/MonacoProviders.tsx @@ -134,7 +134,7 @@ class MonacoProviders extends PureComponent< language, { provideCompletionItems: this.handleCompletionRequest, - triggerCharacters: ['.', '"', "'", '/'], + triggerCharacters: ['.', '"', "'"], } ); From 615f275a9c17e3d678d197382f30912779dbfb21 Mon Sep 17 00:00:00 2001 From: Matthew Runyon Date: Wed, 29 Mar 2023 13:06:08 -0500 Subject: [PATCH 5/6] Update comment --- packages/console/src/monaco/MonacoProviders.tsx | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/console/src/monaco/MonacoProviders.tsx b/packages/console/src/monaco/MonacoProviders.tsx index 897b7257c9..690783c22e 100644 --- a/packages/console/src/monaco/MonacoProviders.tsx +++ b/packages/console/src/monaco/MonacoProviders.tsx @@ -23,7 +23,9 @@ class MonacoProviders extends PureComponent< > { /** * Converts LSP CompletionItemKind to Monaco CompletionItemKind + * Defaults to Variable if no LSP kind was provided * https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#completionItemKind + * * @param kind The LSP kind * @returns Monaco kind */ From 9794715ede6924b2218d37cbb7f6c3022285e9af Mon Sep 17 00:00:00 2001 From: Matthew Runyon Date: Wed, 29 Mar 2023 14:57:01 -0500 Subject: [PATCH 6/6] Add more unit tests --- .../src/monaco/MonacoProviders.test.tsx | 34 ++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/packages/console/src/monaco/MonacoProviders.test.tsx b/packages/console/src/monaco/MonacoProviders.test.tsx index 71ec1c7f70..6bb4c71e03 100644 --- a/packages/console/src/monaco/MonacoProviders.test.tsx +++ b/packages/console/src/monaco/MonacoProviders.test.tsx @@ -1,7 +1,7 @@ import React from 'react'; import { render } from '@testing-library/react'; import * as monaco from 'monaco-editor'; -import dh from '@deephaven/jsapi-shim'; +import dh, { DocumentRange, Position } from '@deephaven/jsapi-shim'; import MonacoProviders from './MonacoProviders'; const DEFAULT_LANGUAGE = 'test'; @@ -342,3 +342,35 @@ it('provides hover info properly', async () => { value: newText, }); }); + +it('converts lsp to monaco range', () => { + const lspRange: DocumentRange = { + start: { line: 0, character: 0 }, + end: { line: 1, character: 1 }, + }; + + const expectedMonacoRange: monaco.IRange = { + startLineNumber: 1, + startColumn: 1, + endLineNumber: 2, + endColumn: 2, + }; + + const monacoRange = MonacoProviders.lspToMonacoRange(lspRange); + expect(monacoRange).toMatchObject(expectedMonacoRange); +}); + +it('converts monaco to lsp position', () => { + const monacoPosition: monaco.IPosition = { + lineNumber: 1, + column: 1, + }; + + const expectedLspPosition: Position = { + line: 0, + character: 0, + }; + + const lspPosition = MonacoProviders.monacoToLspPosition(monacoPosition); + expect(lspPosition).toMatchObject(expectedLspPosition); +});