Skip to content

Conversation

@jacob314
Copy link
Contributor

This should fix
#3173

@jacob314 jacob314 requested a review from pq February 11, 2019 21:37
Copy link
Contributor Author

@jacob314 jacob314 left a comment

Choose a reason for hiding this comment

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

The NPE appears to be due to a case where the tool window was created before the manager had created a currentStats() object. Changed it so that in this case we add the listener when a stats object is created a little bit later.

@jacob314 jacob314 removed the request for review from pq February 11, 2019 21:43
@jacob314
Copy link
Contributor Author

Hold off on reviewing. I think I found one more edge case.

perfManager.getCurrentStats()

Add removePerfListener call so we don't have to guess about lifecycles for
perf listeners.
@jacob314
Copy link
Contributor Author

Implemented a more robust fix. Now listeners are disposed when the view is destroyed but not when the app is just rerun.

Copy link
Member

@devoncarew devoncarew left a comment

Choose a reason for hiding this comment

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

One question in the PR -

if (currentStats != null) {
currentStats.dispose();
currentStats = null;
listeners.clear();
Copy link
Member

Choose a reason for hiding this comment

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

Should we instead call removePerfListener for each listener?

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 don't think we need to as the stats object the listeners are added to is disposed a couple lines earlier.

@jacob314 jacob314 merged commit a8065aa into flutter:master Feb 12, 2019
@jacob314 jacob314 deleted the fix_npe branch February 12, 2019 03:16
alexander-doroshko pushed a commit to alexander-doroshko/flutter-intellij that referenced this pull request Jan 24, 2020
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.

3 participants