-
Notifications
You must be signed in to change notification settings - Fork 6k
Fix iOS embedder memory management and other analyzer warnings #29623
Changes from all commits
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 | ||
|---|---|---|---|---|
|
|
@@ -349,15 +349,15 @@ @interface FlutterEmbedderKeyResponder () | |||
| * | ||||
| * Set by the initializer. | ||||
| */ | ||||
| @property(nonatomic) FlutterSendKeyEvent sendEvent; | ||||
| @property(nonatomic, copy, readonly) FlutterSendKeyEvent sendEvent; | ||||
|
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. engine/shell/platform/darwin/ios/framework/Source/FlutterEmbedderKeyResponder.mm Line 487 in b7459d5
|
||||
|
|
||||
| /** | ||||
| * A map of pressed keys. | ||||
| * | ||||
| * The keys of the dictionary are physical keys, while the values are the logical keys | ||||
| * of the key down event. | ||||
| */ | ||||
| @property(nonatomic) NSMutableDictionary<NSNumber*, NSNumber*>* pressingRecords; | ||||
| @property(nonatomic, retain, readonly) NSMutableDictionary<NSNumber*, NSNumber*>* pressingRecords; | ||||
|
Contributor
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. Why do we have
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 didn't write this and I don't want to start a stylistic war, but I think properties should always preferred over ivars (and the backing ivar only touched in
Can we put a pin in this question for this PR? I only did the bare minimum to make the analyzer happy, plus some tests.
Contributor
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 aware of the property vs ivar debate; I actually don't have a strong opinion on that. I was just curious about the addition of But this wasn't intended to be a PR-blocking question.
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.
Guess I'm always geared up for a religious war in these kinds of reviews 😉 Re: the readonly, you're right, that wasn't a static analyzer warning. I did it because the property wasn't being set other than in |
||||
|
|
||||
| /** | ||||
| * A constant mask for NSEvent.modifierFlags that Flutter synchronizes with. | ||||
|
|
@@ -396,7 +396,8 @@ @interface FlutterEmbedderKeyResponder () | |||
| * Its values are |responseId|s, and keys are the callback that was received | ||||
| * along with the event. | ||||
| */ | ||||
| @property(nonatomic) NSMutableDictionary<NSNumber*, FlutterAsyncKeyCallback>* pendingResponses; | ||||
| @property(nonatomic, retain, readonly) | ||||
| NSMutableDictionary<NSNumber*, FlutterAsyncKeyCallback>* pendingResponses; | ||||
|
|
||||
| /** | ||||
| * Compare the last modifier flags and the current, and dispatch synthesized | ||||
|
|
||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,12 +13,14 @@ @interface FlutterKeyboardManager () | |
| /** | ||
| * The primary responders added by addPrimaryResponder. | ||
| */ | ||
| @property(nonatomic) NSMutableArray<id<FlutterKeyPrimaryResponder>>* primaryResponders; | ||
| @property(nonatomic, retain, readonly) | ||
|
Contributor
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. Same question here. |
||
| NSMutableArray<id<FlutterKeyPrimaryResponder>>* primaryResponders; | ||
|
|
||
| /** | ||
| * The secondary responders added by addSecondaryResponder. | ||
| */ | ||
| @property(nonatomic) NSMutableArray<id<FlutterKeySecondaryResponder>>* secondaryResponders; | ||
| @property(nonatomic, retain, readonly) | ||
| NSMutableArray<id<FlutterKeySecondaryResponder>>* secondaryResponders; | ||
|
|
||
| - (void)dispatchToSecondaryResponders:(nonnull FlutterUIPressProxy*)press | ||
| complete:(nonnull KeyEventCompleteCallback)callback | ||
|
|
@@ -84,7 +86,7 @@ - (void)handlePress:(nonnull FlutterUIPressProxy*)press | |
| NSAssert([_primaryResponders count] >= 0, @"At least one primary responder must be added."); | ||
|
|
||
| __block auto weakSelf = [self getWeakPtr]; | ||
| __block int unreplied = [_primaryResponders count]; | ||
| __block NSUInteger unreplied = [self.primaryResponders count]; | ||
| __block BOOL anyHandled = false; | ||
| FlutterAsyncKeyCallback replyCallback = ^(BOOL handled) { | ||
| unreplied--; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,8 @@ | |
| // Use of this source code is governed by a BSD-style license that can be | ||
| // found in the LICENSE file. | ||
|
|
||
| // FLUTTER_NOLINT: https://github.com/flutter/flutter/issues/93360 | ||
|
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. This one is new. When |
||
|
|
||
| // The only point of this file is to ensure that the Flutter framework umbrella header can be | ||
| // cleanly imported from an Objective-C translation unit. The target that uses this file copies the | ||
| // headers to a path that simulates how users would actually import the framework outside of the | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -73,7 +73,7 @@ @interface FlutterViewController () <FlutterBinaryMessenger, UIScrollViewDelegat | |
| * Keyboard animation properties | ||
| */ | ||
| @property(nonatomic, assign) double targetViewInsetBottom; | ||
| @property(nonatomic, strong) CADisplayLink* displayLink; | ||
| @property(nonatomic, retain) CADisplayLink* displayLink; | ||
|
|
||
| /** | ||
| * Creates and registers plugins used by this view controller. | ||
|
|
@@ -671,21 +671,24 @@ - (void)viewDidLoad { | |
| } | ||
|
|
||
| - (void)addInternalPlugins { | ||
| [self.keyboardManager release]; | ||
| self.keyboardManager = [[FlutterKeyboardManager alloc] init]; | ||
| self.keyboardManager = [[[FlutterKeyboardManager alloc] init] autorelease]; | ||
| fml::WeakPtr<FlutterViewController> weakSelf = [self getWeakPtr]; | ||
| FlutterSendKeyEvent sendEvent = | ||
| ^(const FlutterKeyEvent& event, FlutterKeyEventCallback callback, void* userData) { | ||
| [_engine.get() sendKeyEvent:event callback:callback userData:userData]; | ||
| [weakSelf.get()->_engine.get() sendKeyEvent:event callback:callback userData:userData]; | ||
|
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. This didn't show up in the linter, but this retain cycle was exposed when I wrote the |
||
| }; | ||
| [self.keyboardManager | ||
| addPrimaryResponder:[[FlutterEmbedderKeyResponder alloc] initWithSendEvent:sendEvent]]; | ||
| [self.keyboardManager addPrimaryResponder:[[FlutterChannelKeyResponder alloc] | ||
| initWithChannel:self.engine.keyEventChannel]]; | ||
| [self.keyboardManager addSecondaryResponder:self.engine.textInputPlugin]; | ||
| FlutterChannelKeyResponder* responder = [[[FlutterChannelKeyResponder alloc] | ||
| initWithChannel:self.engine.keyEventChannel] autorelease]; | ||
| [self.keyboardManager addPrimaryResponder:responder]; | ||
| FlutterTextInputPlugin* textInputPlugin = self.engine.textInputPlugin; | ||
| if (textInputPlugin != nil) { | ||
|
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.
|
||
| [self.keyboardManager addSecondaryResponder:textInputPlugin]; | ||
| } | ||
| } | ||
|
|
||
| - (void)removeInternalPlugins { | ||
| [self.keyboardManager release]; | ||
| self.keyboardManager = nil; | ||
| } | ||
|
|
||
|
|
@@ -786,6 +789,8 @@ - (void)deregisterNotifications { | |
| - (void)dealloc { | ||
| [self removeInternalPlugins]; | ||
| [self deregisterNotifications]; | ||
|
|
||
| [_displayLink release]; | ||
| [super dealloc]; | ||
| } | ||
|
|
||
|
|
@@ -1729,6 +1734,7 @@ - (void)scrollEvent:(UIPanGestureRecognizer*)recognizer API_AVAILABLE(ios(13.4)) | |
| - (void)encodeRestorableStateWithCoder:(NSCoder*)coder { | ||
| NSData* restorationData = [[_engine.get() restorationPlugin] restorationData]; | ||
| [coder encodeDataObject:restorationData]; | ||
| [super encodeRestorableStateWithCoder:coder]; | ||
|
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. This was an analyzer warning, |
||
| } | ||
|
|
||
| - (void)decodeRestorableStateWithCoder:(NSCoder*)coder { | ||
|
|
||
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.
Foundation objects with mutable variants (
NSMutableString) should becopy.