Skip to content
This repository has been archived by the owner on Jan 26, 2022. It is now read-only.

Display more variables during expand variable like Class #125

Merged
merged 18 commits into from
Nov 13, 2019
Merged

Display more variables during expand variable like Class #125

merged 18 commits into from
Nov 13, 2019

Conversation

KsavinN
Copy link
Collaborator

@KsavinN KsavinN commented Oct 30, 2019

It's not an appropriate method, more like a hack.

ObjectInpsector from react-inspector doesn't have props for handle expand click.

NEW:
newVar

OLD:

@KsavinN
Copy link
Collaborator Author

KsavinN commented Oct 30, 2019

@jtpio this is new PR correlated with #123 but from the rebased branch with merge current master

src/service.ts Outdated Show resolved Hide resolved
src/service.ts Outdated Show resolved Hide resolved
src/variables/body/index.tsx Outdated Show resolved Hide resolved
@jtpio
Copy link
Member

jtpio commented Oct 30, 2019

Thanks @KsavinN for investigating this.

That might look like a workaround but maybe a good compromise for now while we stick to the react-inspector.

Will have a look at it locally.

@KsavinN
Copy link
Collaborator Author

KsavinN commented Oct 30, 2019

Thanks @KsavinN for investigating this.

That might look like a workaround but maybe a good compromise for now while we stick to the react-inspector.

Will have a look at it locally.

I get a fork from react-inspector. I will try develop ObjectInspector to make additional props for set
handle onExpanded.

@jtpio
Copy link
Member

jtpio commented Nov 4, 2019

Trying it out locally, I noticed the following behavior when expanding an int variable:

variable-expand-moreinfo

@KsavinN
Copy link
Collaborator Author

KsavinN commented Nov 4, 2019

Trying it out locally, I noticed the following behavior when expanding an int variable:

![variable-expand-moreinfo](https://user-images.githubusercontent.com/591645/68114744-12f60100-fef7-11e9-9afd-a86c2405ed8a.gif

@jtpio Thanks for catching that. It probably correlated with merge master. I will fix that

KsavinN and others added 3 commits November 6, 2019 14:57
Set breakpoints via the service

Convert to source breakpoint in the service

Filter breakpoints with the same line numbers

Pass this to disconnect

Rename onNewCell to onActiveCellChanged

Ensure session is ready

Use changed signal for models

add hover method for breakpoints

hover gutter by only css

fixed issue with variablesreference 0 and foreach in service

Use source path for the breakpoints view

Rebase DisplayClassVariable branch
@KsavinN KsavinN requested a review from jtpio November 7, 2019 14:45
@KsavinN KsavinN changed the title [WIP] Display more variables during expand variable like Class Display more variables during expand variable like Class Nov 7, 2019
src/variables/index.ts Outdated Show resolved Hide resolved
src/service.ts Outdated Show resolved Hide resolved
src/service.ts Outdated Show resolved Hide resolved
@jtpio
Copy link
Member

jtpio commented Nov 12, 2019

Thanks @KsavinN for the updates, I will have another look at it.

@jtpio jtpio self-assigned this Nov 12, 2019
@jtpio
Copy link
Member

jtpio commented Nov 13, 2019

It looks like a lot of variables requests are being sent to the kernel, before expanding the variables:

variables-expand

@jtpio
Copy link
Member

jtpio commented Nov 13, 2019

The last commit changes the behavior slightly, so the variables request is only sent when the node is expanded:

expand-on-click

As a side-effect, there is a short period of time where we see the previous content, but I think it's fine for now. As a workaround we could check if it's possible to override the empty dict when the node is collapsed.

@jtpio
Copy link
Member

jtpio commented Nov 13, 2019

Looking at a few other details before merging (switching to wip).

src/variables/body/index.tsx Outdated Show resolved Hide resolved
@jtpio
Copy link
Member

jtpio commented Nov 13, 2019

OK I think we can stop here for now.

Thanks again @KsavinN, this is already a good improvement. We can open a new issue with the things to do as follow-up.

@jtpio jtpio merged commit 8192e8a into jupyterlab:master Nov 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants