[google_maps_flutter] Set properties before adding maps objects #10347
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the update logic for various map objects on iOS to fix a potential flickering issue. The changes ensure all properties of an object are set before it's added to the map, preventing a flicker of default values. This is achieved by introducing static helper methods for each map object type (markers, circles, polygons, etc.) that apply all properties first and then set the map property to make the object visible. This refactoring also resolves some init methods that were incorrectly calling instance methods. The changes are consistently applied across all relevant controllers and are well-supported by new unit tests that verify the correct property update order.
There was a problem hiding this comment.
This isn't actually deleted, I just renamed it (Polylines->Polyline) for consistency, and since I also add a lot of lines, git didn't see it as a rename.
| mapViewOptions.frame = CGRectMake(0, 0, 100, 100); | ||
| mapViewOptions.camera = [[GMSCameraPosition alloc] initWithLatitude:0 longitude:0 zoom:0]; | ||
| return [[PartiallyMockedMapView alloc] initWithOptions:mapViewOptions]; | ||
| } |
There was a problem hiding this comment.
We could extract this as a convenience constructor, but it was simple enough that I just copied it from the tests that already did it into each other test file.
|
|
||
| @end | ||
|
|
||
| @implementation PropertyOrderValidatingGroundOverlay |
There was a problem hiding this comment.
I considered doing something sketchy with overriding core Obj-C selector handling methods to catch all property setters at once, instead of having boilerplate like this for every map object, but since that would have had to be rewritten to something like this to convert the tests to Swift I decided doing this was the better option.
| self.groundOverlay.map = nil; | ||
| } | ||
|
|
||
| - (void)setConsumeTapEvents:(BOOL)consumes { |
There was a problem hiding this comment.
I did this refactoring in all of the object files: every object had a bunch of these passthroughs, almost all of which were a single line, and they are all private and only called by the updateFrom* method, so it seemed like a lot of boilerplate for nothing, and it also prevented me from making the update method a class method. It's not necessary to fix the issue, but it makes the test setup much simpler.
| usingBounds:self.createdWithBounds]; | ||
| } | ||
|
|
||
| + (void)updateGroundOverlay:(GMSGroundOverlay *)groundOverlay |
There was a problem hiding this comment.
This is the other part of the refactoring I made in every object. Having the class method helps with testing because I can expose this and not need to make a full controller (which for some objects has side effects on the map, which we aren't fully mocking, and it can get complicated). We also had a case where the controller's init* method called updateFrom*, which violates style, and making the class method allowed trivially fixing that.
| [self setFadeIn:overlay.fadeIn]; | ||
| [self setTileSize:overlay.tileSize]; | ||
| // This must be done last, to avoid visual flickers of default property values. | ||
| tileLayer.map = platformOverlay.visible ? mapView : nil; |
There was a problem hiding this comment.
This is the actual fix; about half the objects were setting visibility somewhere other than the end.
|
|
||
| #import "messages.g.h" | ||
|
|
||
| NS_ASSUME_NONNULL_BEGIN |
There was a problem hiding this comment.
Drive-by fix; most of the headers had it, and I noticed this one was missing it.
| #import "FLTGoogleMapJSONConversions.h" | ||
|
|
||
| /// Converts a list of holes represented as CLLocation lists to GMSMutablePath lists. | ||
| static NSArray<GMSMutablePath *> *FMGPathHolesFromLocationHoles( |
There was a problem hiding this comment.
I extracted this helper rather than inlining all of setHoles:. It's shorter than the original because the inner loop was doing exactly what the existing FGMGetPathFromPoints utility method (in the utils file) already does.
| identifier:identifier | ||
| mapView:self.mapView]; | ||
| [controller updateFromPlatformPolygon:polygon registrar:self.registrar]; | ||
| [controller updateFromPlatformPolygon:polygon]; |
There was a problem hiding this comment.
The registrar was either copypasta from another object, or it used to be needed and now isn't; it was unused, so I removed it in the refactoring.
| /// | ||
| /// @param styles The styles for repeating pattern sections. | ||
| /// @param lengths The lengths for repeating pattern sections. | ||
| - (void)setPattern:(NSArray<GMSStrokeStyle *> *)styles lengths:(NSArray<NSNumber *> *)lengths; |
There was a problem hiding this comment.
This was exposed for testing, but since I inlined it the test now uses updatePolygon:... instead. That also means it's testing more of the codepath.
1 participant
It appears that the Google Maps SDK on iOS renders on a different thread, and doesn't necessarily batch all updates that happen in a runloop the way standard iOS UI would, so that adding an object to the map before setting all of its properties can cause a flicker of the default property.
This ensures that updating the native maps object from the Dart version sets all properties before adding the object to the map, and refactors to make unit testing the update helper simpler (and in one case, to fix an init-calls-self violation).
Fixes flutter/flutter#143570
Pre-Review Checklist
[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under1.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under1.///).Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2 ↩3