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

Add new data flow paths view (empty) #2172

Merged
merged 3 commits into from
Mar 17, 2023
Merged

Conversation

charisk
Copy link
Contributor

@charisk charisk commented Mar 14, 2023

Add a new webview that will be used to show the data flow paths, instead of the full screen overlay that we use currently. See linked internal issue for more details.

This is currently not in use - it will be wired up in a follow-up PR.

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 added the secexp label Mar 14, 2023
@charisk charisk requested review from a team as code owners March 14, 2023 16:02
Comment on lines 22 to 28
if (!this.isShowingPanel) {
await this.getPanel();
await this.waitForPanelLoaded();
}

// Focus on panel
this.panel?.reveal(undefined, true);
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be in a different order than the variant analysis view. Does this construct from the variant analysis view not work for this view or should the variant analysis view be changed to use this?

Suggested change
if (!this.isShowingPanel) {
await this.getPanel();
await this.waitForPanelLoaded();
}
// Focus on panel
this.panel?.reveal(undefined, true);
const panel = await this.getPanel();
panel.reveal(undefined, true);
await this.waitForPanelLoaded();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that the use case is just slightly different. With the data flow paths view, we'd want the callers to just say "show the paths" instead of having separate openView and updateView steps. We also only want to ever allow 1 data flow paths view to ever be open, rather than one for each set of data flow paths it renders.

Copy link
Member

Choose a reason for hiding this comment

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

I think that is still possible with the logic from the variant analysis view. The isShowingPanel check is implicit when you call getPanel, and waitForPanelLoaded will just resolve immediately when the panel is already loaded. So, I think the behaviour is the same. I think it would be good to keep these checks consistent between the views.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh sorry I didn't look at the diff properly. I thought you were suggesting having two functions - openView and editView. Made the change now.

ToDataFlowPathsMessage,
FromDataFlowPathsMessage
> {
public static readonly viewType = "codeQL.dataFlowPaths";
Copy link
Member

Choose a reason for hiding this comment

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

Should opening this view also be added as an activation event?

It might not be necessary to add it since it can only be opened if another CodeQL view is already open, but perhaps it would be good for consistency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm I'm not sure. This view won't persist - it will be lost if you restart VS Code and you'd have to click on "Show paths" again. So why would it go in the activation events? Or have I misunderstood what this is?

Copy link
Member

Choose a reason for hiding this comment

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

Activation events are also used for opening a non-persisted view, so it would also be an activation event if you opened the view from somewhere else. However, as I mentioned I don't believe it's necessary for this view to be an activation event because another view will always have to be loaded already before you can open this one. However, all of our current views are in the activation events, even those that currently also have that behaviour (like onWebviewPanel:resultsView), so I think it would be good to add it for consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems odd to just do it for consistency but that's perhaps a tech-debt issue. Updated.

@charisk charisk force-pushed the charisk/data-flow-paths-view branch from 239190d to 26a46df Compare March 17, 2023 08:45
@charisk charisk merged commit 8a52730 into main Mar 17, 2023
@charisk charisk deleted the charisk/data-flow-paths-view branch March 17, 2023 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants