Skip to content

Add ability to set method for modeling in the modeling panel#2767

Merged
charisk merged 5 commits intomainfrom
charisk/method-model-basic-wire-up
Sep 1, 2023
Merged

Add ability to set method for modeling in the modeling panel#2767
charisk merged 5 commits intomainfrom
charisk/method-model-basic-wire-up

Conversation

@charisk
Copy link
Copy Markdown
Contributor

@charisk charisk commented Aug 31, 2023

This PR adds some initial wire-up for the method modeling panel. It first adds the ability to set the method that is being modeled, and then adds some basic wire up so that a dummy method is shown when the user clicks the "View" button in the editor. I've not wired up the actual method information yet because it requires some refactoring to make that available (we only have the Usage at the minute). This will be done in a following PR.

Note that I've broken this into 3 commits for ease of review.

Checklist

N/A:

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@charisk charisk requested review from a team as code owners August 31, 2023 11:04
@charisk charisk force-pushed the charisk/method-model-basic-wire-up branch from f82613e to e9bb357 Compare August 31, 2023 11:21
Copy link
Copy Markdown
Contributor

@robertbrignull robertbrignull left a comment

Choose a reason for hiding this comment

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

Code all looks ok to me, but sadly it didn't work when I tested it. I didn't see any error messages though. It just didn't show the usages panel or the new modeling panel.

Comment thread extensions/ql-vscode/src/model-editor/model-editor-module.ts
}

public async setMethod(method: ExternalApiUsage): Promise<void> {
if (this.webviewView) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This doesn't seem to be working for me. At the point it reaches this point this.webviewView is undefined. Does it work for you? 🤔

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems to be because resolveWebviewView isn't called until the user manually expands the tab. The tab was there, but it wasn't responding when I selected a method until I had manually expanded it.

Perhaps we could automatically initialise the view and not rely on the user. But that could be a separate PR from this one. Once I expanded the view it was behaving as expected 🎉

@charisk charisk force-pushed the charisk/method-model-basic-wire-up branch from e9bb357 to 56c8eb2 Compare September 1, 2023 09:58
charisk and others added 2 commits September 1, 2023 11:17
…ew.tsx

Co-authored-by: Robert <robertbrignull@github.com>
@charisk charisk enabled auto-merge (squash) September 1, 2023 10:34
Copy link
Copy Markdown
Contributor

@robertbrignull robertbrignull left a comment

Choose a reason for hiding this comment

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

LGTM

@charisk charisk merged commit ac7a37c into main Sep 1, 2023
@charisk charisk deleted the charisk/method-model-basic-wire-up branch September 1, 2023 11:16
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.

2 participants