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

ResourceLabel listens #63655

Closed
jrieken opened this issue Nov 22, 2018 · 2 comments · Fixed by #65408
Closed

ResourceLabel listens #63655

jrieken opened this issue Nov 22, 2018 · 2 comments · Fixed by #65408
Assignees
@bpasero
Labels
debt Code quality issues
Milestone
December/January ...

Comments

@jrieken
Copy link
Member

jrieken commented Nov 22, 2018

When adding the listener-leak-warning (b548ced) I had to chose a very high default limit of 500 listeners. This is mostly because of the ResourceLabel which registers 5 listeners per instance. This multiples quickly because the resource label is used everywhere. On a normal screen the explorer shows ~40 elements which results is 200 listeners already. The same is true for one page of search results and a page full of git changes, totalling then at 600 listeners, 120 listener per event.

Instead, "containers" should listen. In above sample the explorer-, the search-, and the git-viewlet. That would total in 3*5 listeners. This would be especially useful for those event-handlers that are all the same, like this one: https://github.com/Microsoft/vscode/blob/d21dbf9cc1ad020a8dc686de8d29894a2a6ea7b3/src/vs/workbench/browser/labels.ts#L79-L83

Also, listening can be made smarter. Today, a theme change updates a label even when its viewlet isn't visible.

For some events this might be more tricky, esp. those that require state form the widget, like its uri or specific config. It would still try to keep the widget stupid and to make that state accessible via getters so the outside can make a decision with that information.
@bpasero bpasero added the debt Code quality issues label Nov 22, 2018
jrieken added a commit that referenced this issue Nov 23, 2018
@jrieken
have high listener leak limit only for those emitters that are used b…
c114ccc 
…y the ResourceLabel, #63655
@jrieken jrieken mentioned this issue Dec 6, 2018
Closed
@bpasero bpasero added this to the December/January 2019 milestone Dec 19, 2018
@bpasero bpasero mentioned this issue Dec 19, 2018
Merged
@bpasero
Copy link
Member

bpasero commented Dec 19, 2018 •
edited

@jrieken I pushed a couple of things:

  • ResourceLabels is the new guy that should be used unless you really just have one label (only 2 places currently where this is true)
  • it listens to all the events and forwards them to the labels within
  • it exposes methods onVisible and onHidden (similar to the old tree) that containers on top can call to indicate if they are visible or not
  • based on that the labels decide wether they should redraw or not (and if a request comes in to redraw, they will redraw once the label becomes visible

I adopted onVisible and onHidden for every view. I did not adopt it for BreadcrumbsPicker because I did not see an obvious isVisible change, maybe you could add that.

@sandy081 I got a little bit lost for Viewlet vs View. My understanding is that setVisible is called for Viewlets, but I was not sure if a similar concept exists for a View when it gets collapsed or expanded (or actually its parent Viewlet gets shown or hidden).

I feel like a follow up item could be to have Viewlet and View have an event that the ResourceLabels could subscribe to to make this easier.

PS: the labels are still listening on events even if they are not visible. I thought about changing that, but it would have the downside that I would have to basically replay each event that happened when the label becomes visible. I thought this was not a big enough gain to have.

PPS: I guess this means we could change the event leak listener count again.

@jrieken
Copy link
Member Author

jrieken commented Dec 20, 2018

PPS: I guess this means we could change the event leak listener count again.

✅

jrieken added a commit that referenced this issue Dec 20, 2018
@jrieken
no more special listener thresholds, #63655
d3e1055
@vscodebot vscodebot bot locked and limited conversation to collaborators Feb 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@bpasero bpasero

Labels
debt Code quality issues
Projects
None yet
Milestone
December/January 2019
Development

Successfully merging a pull request may close this issue.

introduce ResourceLabels microsoft/vscode
2 participants
@bpasero @jrieken