Skip to content
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

feat: user widgets #194

Merged
merged 21 commits into from
Jul 25, 2022
Merged

feat: user widgets #194

merged 21 commits into from
Jul 25, 2022

Conversation

EdAyers
Copy link
Contributor

@EdAyers EdAyers commented Jun 14, 2022

Introduces arbitrary javascript widgets to Lean 4.

Todo

lean4-infoview/src/infoview/userWidget.tsx Outdated Show resolved Hide resolved
lean4-infoview/src/infoview/userWidget.tsx Outdated Show resolved Hide resolved
lean4-infoview/src/infoview/userWidget.tsx Show resolved Hide resolved
lean4-infoview/src/infoview/userWidget.tsx Outdated Show resolved Hide resolved
lean4-infoview/src/infoview/userWidget.tsx Outdated Show resolved Hide resolved
lean4-infoview/src/infoview/userWidget.tsx Outdated Show resolved Hide resolved
lean4-infoview/src/infoview/userWidget.tsx Outdated Show resolved Hide resolved
lean4-infoview/src/infoview/userWidget.tsx Outdated Show resolved Hide resolved
lean4-infoview/src/infoview/info.tsx Outdated Show resolved Hide resolved
@EdAyers EdAyers marked this pull request as ready for review June 26, 2022 04:07
<div style={{display: hasMessages ? 'block' : 'none'}}>
{widgets && widgets.map(widget =>
<div style={{display: hasWidget ? 'block' : 'none'}}
key={`widget::${widget.widgetSourceId}::${widget.range?.toString()}`}>
Copy link
Member

Choose a reason for hiding this comment

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

This is not a unique key, is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I was assuming multiple widgets with the same widgetid wouldn't be attached to the same point in the document which might be a bad assumption.

lean4-infoview/src/infoview/info.tsx Outdated Show resolved Hide resolved
lean4-infoview/src/infoview/userWidget.tsx Outdated Show resolved Hide resolved
lean4-infoview/src/infoview/userWidget.tsx Outdated Show resolved Hide resolved
lean4-infoview/src/infoview/userWidget.tsx Outdated Show resolved Hide resolved
lean4-infoview/src/infoview/util.ts Show resolved Hide resolved
lean4-infoview/src/infoview/rpcInterface.ts Outdated Show resolved Hide resolved
@gebner
Copy link
Member

gebner commented Jul 22, 2022

There are some merge conflicts due to #226 now. I'm happy to migrate the code here to the new API if you want me to, but maybe it's better if you check out the differences yourself.

}

/** Given a position, returns all of the user-widgets on the infotree at this position. */
export function Widget_getWidgets(rs: RpcSessions, pos: DocumentPosition): Promise<UserWidgets | undefined> {
Copy link
Member

Choose a reason for hiding this comment

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

I started on the merge, but noticed the following thing: in our RPC calls, sending DocumentPosition actually makes no sense because we always use the URI at which the RPC call was made (so now, the RpcSessionAtPos URI). getInteractiveDiagnostics just takes in a line range, whereas getInteractiveGoals takes the whole TextDocumentPositionParams but the the handler just ignores it. Therefore I think we should change the widget APIs to just take a Position which is a line+character, but no URI. The goal APIs have been around so I guess we should keep them as-is for compat.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I made this change everywhere. Can always revert if we decide otherwise.

@gebner gebner merged commit d0944a2 into leanprover:master Jul 25, 2022
@Vtec234 Vtec234 deleted the widget-pr branch July 25, 2022 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants