Skip to content

Signature help: restructure async work #1198

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Dec 9, 2024
Merged

Signature help: restructure async work #1198

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1f3d6f6
Signature help: restructure async work
microbit-matt-hillsdon Dec 5, 2024
b7d5667
Calculate line/column immediately
microbit-matt-hillsdon Dec 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
162 changes: 104 additions & 58 deletions src/editor/codemirror/language-server/signatureHelp.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
*
* SPDX-License-Identifier: MIT
*/
import { EditorState, StateEffect, StateField } from "@codemirror/state";
import { Facet, StateEffect, StateField } from "@codemirror/state";
import {
Command,
EditorView,
Expand All @@ -22,8 +22,8 @@ import {
import { IntlShape } from "react-intl";
import {
MarkupContent,
Position,
SignatureHelp,
SignatureHelpParams,
SignatureHelpRequest,
} from "vscode-languageserver-protocol";
import { ApiReferenceMap } from "../../../documentation/mapping/content";
Expand All @@ -38,6 +38,10 @@ import {
import { nameFromSignature, removeFullyQualifiedName } from "./names";
import { offsetToPosition } from "./positions";

export const automaticFacet = Facet.define<boolean, boolean>({
combine: (values) => values[values.length - 1] ?? true,
});

export const setSignatureHelpRequestPosition = StateEffect.define<number>({});

export const setSignatureHelpResult = StateEffect.define<SignatureHelp | null>(
Expand All @@ -49,6 +53,12 @@ class SignatureHelpState {
* -1 for no signature help requested.
*/
pos: number;

/**
* The LSP position for pos.
*/
position: Position | null;

/**
* The latest result we want to display.
*
Expand All @@ -57,11 +67,16 @@ class SignatureHelpState {
*/
result: SignatureHelp | null;

constructor(pos: number, result: SignatureHelp | null) {
constructor(
pos: number,
position: Position | null,
result: SignatureHelp | null
) {
if (result && pos === -1) {
throw new Error("Invalid state");
}
this.pos = pos;
this.position = position;
this.result = result;
}
}
Expand All @@ -77,44 +92,22 @@ const signatureHelpToolTipBaseTheme = EditorView.baseTheme({
},
});

const triggerSignatureHelpRequest = async (
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I found this hard to pick up again & understand because this function is called from two very different contexts: a CM command (where it's totally fine / expected to view.dispatch) and and the ViewPlugin update call. Doubtless my doing!

view: EditorView,
state: EditorState
): Promise<void> => {
const uri = state.facet(uriFacet)!;
const client = state.facet(clientFacet)!;
const pos = state.selection.main.from;
const params: SignatureHelpParams = {
textDocument: { uri },
position: offsetToPosition(state.doc, pos),
};
try {
// Must happen before other event handling that might dispatch more
// changes that invalidate our position.
queueMicrotask(() => {
view.dispatch({
effects: [setSignatureHelpRequestPosition.of(pos)],
});
});
const result = await client.connection.sendRequest(
SignatureHelpRequest.type,
params
);
view.dispatch({
effects: [setSignatureHelpResult.of(result)],
});
} catch (e) {
if (!isErrorDueToDispose(e)) {
logException(state, e, "signature-help");
}
view.dispatch({
effects: [setSignatureHelpResult.of(null)],
});
const positionEq = (a: Position | null, b: Position | null): boolean => {
if (a === null) {
return b === null;
}
if (b === null) {
return a === null;
}
return a.character === b.character && a.line === b.line;
};

const openSignatureHelp: Command = (view: EditorView) => {
triggerSignatureHelpRequest(view, view.state);
view.dispatch({
effects: [
setSignatureHelpRequestPosition.of(view.state.selection.main.from),
],
});
return true;
};

Expand All @@ -124,7 +117,7 @@ export const signatureHelp = (
apiReferenceMap: ApiReferenceMap
) => {
const signatureHelpTooltipField = StateField.define<SignatureHelpState>({
create: () => new SignatureHelpState(-1, null),
create: () => new SignatureHelpState(-1, null, null),
update(state, tr) {
let { pos, result } = state;
for (const effect of tr.effects) {
Expand All @@ -138,17 +131,45 @@ export const signatureHelp = (
}
}
}

// Even if we just got a result, if the position has been cleared we don't want it.
if (pos === -1) {
result = null;
}

// By default map the previous position forward
pos = pos === -1 ? -1 : tr.changes.mapPos(pos);
if (state.pos === pos && state.result === result) {

// Did the selection moved while open? We'll re-request but keep the old result for now.
if (pos !== -1 && tr.selection) {
pos = tr.selection.main.from;
}

// Automatic triggering cases
const automatic = tr.state.facet(automaticFacet).valueOf();
if (
automatic &&
((tr.docChanged && tr.isUserEvent("input")) ||
tr.isUserEvent("dnd.drop.call"))
) {
tr.changes.iterChanges((_fromA, _toA, _fromB, _toB, inserted) => {
if (inserted.sliceString(0).trim().endsWith("()")) {
// Triggered
pos = tr.newSelection.main.from;
}
});
}

const position = pos === -1 ? null : offsetToPosition(tr.state.doc, pos);
if (
state.pos === pos &&
state.result === result &&
positionEq(state.position, position)
) {
// Avoid pointless tooltip updates. If nothing else it makes e2e tests hard.
return state;
}
return new SignatureHelpState(pos, result);
return new SignatureHelpState(pos, position, result);
},
provide: (f) =>
showTooltip.from(f, (val) => {
Expand Down Expand Up @@ -191,30 +212,54 @@ export const signatureHelp = (
extends BaseLanguageServerView
implements PluginValue
{
constructor(view: EditorView, private automatic: boolean) {
private destroyed = false;
private lastPosition: Position | null = null;

constructor(view: EditorView) {
super(view);
}
update(update: ViewUpdate) {
if (
(update.docChanged || update.selectionSet) &&
this.view.state.field(signatureHelpTooltipField).pos !== -1
) {
triggerSignatureHelpRequest(this.view, update.state);
} else if (this.automatic && update.docChanged) {
const last = update.transactions[update.transactions.length - 1];

// This needs to trigger for autocomplete adding function parens
// as well as normal user input with `closebrackets` inserting
// the closing bracket.
if (last.isUserEvent("input") || last.isUserEvent("dnd.drop.call")) {
last.changes.iterChanges((_fromA, _toA, _fromB, _toB, inserted) => {
if (inserted.sliceString(0).trim().endsWith("()")) {
triggerSignatureHelpRequest(this.view, update.state);
const { view, state } = update;
const uri = state.facet(uriFacet)!;
const client = state.facet(clientFacet)!;
const { position } = update.state.field(signatureHelpTooltipField);
if (!positionEq(this.lastPosition, position)) {
this.lastPosition = position;
if (position !== null) {
(async () => {
try {
const result = await client.connection.sendRequest(
SignatureHelpRequest.type,
{
textDocument: { uri },
position,
}
);
if (!this.destroyed) {
view.dispatch({
effects: [setSignatureHelpResult.of(result)],
});
}
} catch (e) {
if (!isErrorDueToDispose(e)) {
logException(state, e, "signature-help");
}
// The sendRequest call can fail synchronously when disposed so we need to ensure our clean-up doesn't happen inside the CM update call.
queueMicrotask(() => {
if (!this.destroyed) {
view.dispatch({
effects: [setSignatureHelpResult.of(null)],
});
}
});
}
});
})();
}
}
}
destroy(): void {
this.destroyed = true;
}
}

const formatSignatureHelp = (
Expand Down Expand Up @@ -306,10 +351,11 @@ export const signatureHelp = (

return [
// View only handles automatic triggering.
ViewPlugin.define((view) => new SignatureHelpView(view, automatic)),
ViewPlugin.define((view) => new SignatureHelpView(view)),
signatureHelpTooltipField,
signatureHelpToolTipBaseTheme,
keymap.of(signatureHelpKeymap),
automaticFacet.of(automatic),
EditorView.domEventHandlers({
blur(event, view) {
// Close signature help as it interacts badly with drag and drop if
Expand Down
Loading