Skip to content

Implementing jumping to location when clicking on a usage in the details panel#2679

Merged
robertbrignull merged 5 commits intomainfrom
robertbrignull/data-details-jump
Aug 9, 2023
Merged

Implementing jumping to location when clicking on a usage in the details panel#2679
robertbrignull merged 5 commits intomainfrom
robertbrignull/data-details-jump

Conversation

@robertbrignull
Copy link
Copy Markdown
Contributor

@robertbrignull robertbrignull commented Aug 8, 2023

The feature being implemented here is that you can click on an entry in the CodeQL model details panel and it'll open that code location in the main editor. This doesn't implement selecting the correct usage when clicking on the "view" button and that will be a different PR.

I've implemented this using a new codeQLDataExtensions.jumpToUsageLocation command. I did first try out using vscode.open and passing it a URI, but sadly I don't believe we can use that. Instead we can call the existing showResolvableLocation method, but making that work involves a few steps:

  • We'd like some error handling, and we currently already perform this same error handling in two places. I move the error handling to be inside showResolvableLocation so we avoid duplication.
  • We need the DatabaseItem to be able to resolve the code location to a file that can be displayed in the editor. This means that we need to pass the database item to the ModelDetailsDataProvider class along with the ExternalApiUsage[] array. I think it makes sense to set these together because you need both to be in sync in order to resolve locations correctly.

Checklist

  • 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.

@robertbrignull robertbrignull requested review from a team as code owners August 8, 2023 14:19
@robertbrignull robertbrignull force-pushed the robertbrignull/data-details-jump branch from 6779475 to 7443b35 Compare August 8, 2023 15:34
Copy link
Copy Markdown
Contributor

@charisk charisk left a comment

Choose a reason for hiding this comment

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

Thank you for splitting into sensible commits - it made reviewing much easier 🎉

I've got a couple of non-blocking comments.

},
);
},
"codeQLDataExtensions.jumpToUsageLocation": async (
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.

Should the command be codeQLDataExtensionsEditor as it's not general to data extensions? I don't have a strong opinion - just a thought really.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point. I was more trying to match the naming scheme of the files for the module, but either way codeQLDataExtensionsEditor is a better name.

externalApiUsages: ExternalApiUsage[],
databaseItem: DatabaseItem,
): void {
this.dataProvider.setState(externalApiUsages, databaseItem);
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 surprised me a little bit. I've generally thought of the "DataProvider" classes as the ones managing state and expected the data extensions editor view to use the provider rather than the panel. Having said that, I can't think of a problem with what you've done here 🤔

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I guess it depends whether you think of the data provider as the public interface to the panel or as an internal detail. Personally I think of them as more internal, but I'm also happy with either option.

Currently the data provider is constructed and owned by the panel. We could have it be constructed outside and passed into the panel. Then the panel is literally nothing more than a single call to window.createTreeView.

Currently the panel is constructed from DataExtensionsEditorModule and there isn't a separate "module" just for the detail panel. So in this case I think it's better to have the data provider be constructed inside ModelDetailsPanel as it keeps things slightly contained without introducing another "module" class which would itself be tiny.

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.

Cool! Lets go with that and see if we can identify any pros/cons about either approach. In general it feels like it would be good to standardise a bit to make the codebase a bit easier.

@robertbrignull robertbrignull merged commit bd6a7b2 into main Aug 9, 2023
@robertbrignull robertbrignull deleted the robertbrignull/data-details-jump branch August 9, 2023 12:04
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