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.

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

Jump to bottom

get children when node expands #201909

Merged
merged 3 commits into from
Jan 5, 2024
Merged

get children when node expands #201909

merged 3 commits into from
Jan 5, 2024

Conversation

amunger
Copy link
Contributor

@amunger amunger commented Jan 5, 2024 •
edited
Loading

#165445

  • switched to an async tree so that getting children can be done lazily
  • added an ID field to the variables on vscode side so we can look up the correct variable instance to ask for children
  • It seems like we would benefit from sharing the ID with the extension as well to be able to keep using the same instances
    • Every time we ask for the root variables, we no longer know how to reliably match them up to what we currently have

@VSCodeTriageBot VSCodeTriageBot added this to the December / January 2024 milestone Jan 5, 2024
@amunger amunger requested a review from rebornix January 5, 2024 23:22
@amunger amunger merged commit f88bce8 into main Jan 5, 2024
6 checks passed
@amunger amunger deleted the aamunger/notebookVariableView branch January 5, 2024 23:44
roblourens
roblourens reviewed Jan 8, 2024
View reviewed changes
.../workbench/contrib/notebook/browser/contrib/notebookVariables/notebookVariablesDataSource.ts

async getChildren(element: INotebookScope | INotebookVariableElement): Promise<Array<INotebookVariableElement>> {
if (element.type === 'root') {
this.notebook = element.notebook;
Copy link
Member

Choose a reason for hiding this comment

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

Instead of having this state on the data source object, it might be nicer if all the state needed to look up child variables is on the variable itself.

@microsoft microsoft locked and limited conversation to collaborators Jun 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@roblourens roblourens roblourens left review comments

@rebornix rebornix rebornix approved these changes

Assignees

@amunger amunger

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@amunger @roblourens @rebornix @VSCodeTriageBot