-
Notifications
You must be signed in to change notification settings - Fork 106
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.
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
Fix strong reference cycle between RoomProxy and RoomTimelineProvider #217
Conversation
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
📱 Scan the QR code below to install the build for this PR. If you can't scan the QR code you can install the build via this link: https://i.diawi.com/kXztLH |
Codecov ReportBase: 11.79% // Head: 11.79% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## develop #217 +/- ##
========================================
Coverage 11.79% 11.79%
========================================
Files 213 213
Lines 14359 14359
Branches 6974 6974
========================================
Hits 1693 1693
Misses 12623 12623
Partials 43 43
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not quite sure I follow this one: the roomProxy is a direct dependency of the TimelineProvider and should be strongly retained by it. If there's a cycle it should be handled another way.
LE: I had a closer look at it, we need to take the timelineProvider out of the roomProxy and create it when needed in the AppCoordinatorUserSessionFlowCoordinator. The timelineProvider can register itself as a listener on its own roomProxy dependency.
Johannes tries to get weird retain cycle context from last night back into his head. 😅 I think they're mutual dependencies, right? The reason I made the reference in
That might be a better solution altogether. I'll have to return to this after hours. |
Yeah, the RoomProxy used to create the timelineProvider and retain it itself. I fixed it here if you want to have a look /pull/220 |
Discarding this over #220. |
Fixes: #216
Pull Request Checklist
UI changes have been tested with: