-
Notifications
You must be signed in to change notification settings - Fork 6k
FlutterViewController notify will dealloc #12232
Changes from all commits
4610d7e
4dc44a6
f1dff6b
058d0e6
b0ce5d6
ca3d5d5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,7 @@ @interface FlutterEngine () <FlutterTextInputDelegate, FlutterBinaryMessenger> | |
| @property(nonatomic, readonly) NSMutableDictionary* pluginPublications; | ||
|
|
||
| @property(nonatomic, readwrite, copy) NSString* isolateId; | ||
| @property(nonatomic, retain) id<NSObject> flutterViewControllerWillDeallocObserver; | ||
| @end | ||
|
|
||
| @interface FlutterEngineRegistrar : NSObject <FlutterPluginRegistrar> | ||
|
|
@@ -111,6 +112,7 @@ - (void)dealloc { | |
|
|
||
| NSNotificationCenter* center = [NSNotificationCenter defaultCenter]; | ||
| [center removeObserver:self name:UIApplicationDidReceiveMemoryWarningNotification object:nil]; | ||
| [_flutterViewControllerWillDeallocObserver release]; | ||
|
|
||
| [super dealloc]; | ||
| } | ||
|
|
@@ -167,12 +169,21 @@ - (void)setViewController:(FlutterViewController*)viewController { | |
| _viewController = [viewController getWeakPtr]; | ||
| self.iosPlatformView->SetOwnerViewController(_viewController); | ||
| [self maybeSetupPlatformViewChannels]; | ||
|
|
||
| self.flutterViewControllerWillDeallocObserver = | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is ok but consistency-wise seems a bit unnecessarily different. We already have _shell->GetPlatformView() all over the place in this file. Can't we just call
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not doing that because that would be changing behavior outside the bounds of the problem I was fixing. I'm just moving this file over to the new mechanism I created so we don't have multiple ways to do the same thing. The only logical change I made here was calling
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Discussed offline. Moving the direct plumbing to NSNotificationCenter is ok. If we're implicitly saying the precedent is also to promote moving other event plumbing to notification center. |
||
| [[NSNotificationCenter defaultCenter] addObserverForName:FlutterViewControllerWillDealloc | ||
| object:viewController | ||
| queue:[NSOperationQueue mainQueue] | ||
| usingBlock:^(NSNotification* note) { | ||
| [self notifyViewControllerDeallocated]; | ||
| }]; | ||
| } | ||
|
|
||
| - (void)notifyViewControllerDeallocated { | ||
| if (!_allowHeadlessExecution) { | ||
| [self destroyContext]; | ||
| } | ||
| _viewController.reset(); | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should only happen once right? Should we get rid of the observer here too?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could. I wouldn't recommend it. It would make more sense inside of the observer's block but it isn't necessary there either since the semantics of the message are that the object is dying, it shouldn't be dying twice. |
||
|
|
||
| - (void)destroyContext { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,6 +49,19 @@ | |
| accessibility_bridge_.reset(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we get rid of that observer here too?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are getting rid of the observer a couple lines down, line 55. I don't believe there is any reason why it should be happening higher in the method. |
||
| } | ||
| owner_controller_ = owner_controller; | ||
|
|
||
| // Add an observer that will clear out the owner_controller_ ivar and | ||
| // the accessibility_bridge_ in case the view controller is deleted. | ||
| dealloc_view_controller_observer_.reset([[NSNotificationCenter defaultCenter] | ||
| addObserverForName:FlutterViewControllerWillDealloc | ||
| object:owner_controller_.get() | ||
| queue:[NSOperationQueue mainQueue] | ||
| usingBlock:^(NSNotification* note) { | ||
| // Implicit copy of 'this' is fine. | ||
| accessibility_bridge_.reset(); | ||
| owner_controller_.reset(); | ||
| }]); | ||
|
|
||
| if (owner_controller_) { | ||
| ios_surface_ = | ||
| [static_cast<FlutterView*>(owner_controller.get().view) createSurface:gl_context_]; | ||
|
|
||
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 have a feeling if this happened before the view controller got dealloc'ed, it's probably a bug right? I don't think this can happen legitimately. Maybe we should print a message here.
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.
Nope, that wouldn't be a bug. Releasing the observer detaches this object from the notification center for that message, so after that releasing the FlutterViewController will just not notify a FlutterEngine that he's being dealloc'd.