[google_maps_flutter_ios] Fixed map objects being added to the map before all properties are set #7001
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
…/github.com/Colman/packages into ios-map-objects-added-before-properties-set
| ## 2.8.1 | ||
|
|
||
| * Improves Objective-C type handling. | ||
| - Improves Objective-C type handling. |
There was a problem hiding this comment.
These style changes don't seem to be related to your change. Please follow existing style in the changelog, consistent with the others.
| NSNumber *visible = FGMGetValueOrNilFromDict(data, @"visible"); | ||
| if (visible) { | ||
| [self setVisible:[visible boolValue]]; | ||
| } |
There was a problem hiding this comment.
Moving this code doesn't seem necessary to your change. Please revert to minimise diffs.
| NSNumber *visible = FGMGetValueOrNilFromDict(data, @"visible"); | ||
| if (visible) { | ||
| [self setVisible:[visible boolValue]]; | ||
| } |
There was a problem hiding this comment.
Moving this code doesn't seem relevant to your change. Please revert to minimise diffs.
There was a problem hiding this comment.
It is not necessary to fix the polygon issue, but the root cause of the issue is that the object isn't added to the map as the last step. The polygon fill color issue is just one symptom of this problem, but there are many more. The same problem happens with the fill color of a circle since it's set after it's assigned to the map. In my opinion, it's wasteful to create a separate PR for each map object (marker, circle, polygon) to fix this issue since they all have the same root cause. That is that the properties are set after it's added to the map.
If you still insist on your comment, could I at least edit my issue to include other map objects like circles, markers, and tiles instead of creating 3 more PRs?
There was a problem hiding this comment.
If the order of this call relative to everything else in the method is critical, please add a comment to that effect on this part of the code, otherwise it's not going to be clear to people editing (or reviewing changes to) these files in the future that adding new properties after this block is incorrect.
There was a problem hiding this comment.
Okay I added a comment to each of the changes
| }; | ||
| rootObject = 97C146E61CF9000F007C117D /* Project object */; | ||
| } | ||
| } |
There was a problem hiding this comment.
This file should be reverted since there's no relevant diff.
.../google_maps_flutter/google_maps_flutter_ios/ios/Classes/FLTGoogleMapTileOverlayController.m
Show resolved
Hide resolved
|
@Colman Are you still planning on updating this PR to address the review comments above? |
Hi @stuartmorgan, I'm waiting on a reply from @cbracken in one of the comments above. |
|
From triage: @Colman did my reply in that comment thread address the blocking issue? |
This comment was marked as off-topic.
This comment was marked as off-topic.
# Conflicts: # packages/google_maps_flutter/google_maps_flutter_ios/CHANGELOG.md # packages/google_maps_flutter/google_maps_flutter_ios/example/ios14/ios/Runner.xcodeproj/project.pbxproj # packages/google_maps_flutter/google_maps_flutter_ios/ios/Classes/GoogleMapMarkerController.h # packages/google_maps_flutter/google_maps_flutter_ios/pubspec.yaml
|
Hi, I've been very busy lately and haven't had time to look at this PR. Sorry about that. I will most likely get to it next week. |
# Conflicts: # packages/google_maps_flutter/google_maps_flutter_ios/CHANGELOG.md
|
Okay @stuartmorgan it should be ready |
| @@ -1,3 +1,7 @@ | |||
| ## 2.13.2 | |||
|
|
|||
| * Fixes map objects (markers, polygons, polylines, circles, and tiles) being added with the wrong properties | |||
There was a problem hiding this comment.
This is missing a period; please see the style guide linked from the PR checklist.
| - (void)setMap:(GMSMapView *)map { | ||
| super.map = map; | ||
|
|
||
| if (self.didSetMap || map == nil || map == (id)[NSNull null]) { |
There was a problem hiding this comment.
Why are these NSNull checks here in all of these files? Is there production code passing NSNull as a GMSMapView*? If so that's a very serious bug.
There was a problem hiding this comment.
I don't think there was a reason other than me not understanding what the difference between nil and [NSNull null] was. So I think I just checked for both. Should be fixed now.
| circleId:(NSString *)circleIdentifier | ||
| mapView:(GMSMapView *)mapView | ||
| options:(NSDictionary *)options; | ||
| - (instancetype)initWithCircle:(GMSCircle *)circle |
There was a problem hiding this comment.
Are these new initializers only here for tests? If so, they should be declared in ..._Test.h files instead of here.
| [self interpretCircleOptions:options]; | ||
| } | ||
| return self; | ||
| } |
There was a problem hiding this comment.
These should not be two completely unrelated constructors; this is a significant maintenance problem, and violates Obj-C convention. The existing constructor should delegate to this new constructor with the Circle it creates.
(Same for all the other files.)
There was a problem hiding this comment.
Each implementation file should import its new test header right after its public header.
| #import "GoogleMapCircleController.h" | ||
|
|
||
| @interface FLTGoogleMapCircleController (Test) | ||
| - (instancetype)initWithCircle:(GMSCircle *)circle |
There was a problem hiding this comment.
Per the style guide, all of these new methods need declaration comments.
There was a problem hiding this comment.
Yeah sorry I don't have time for this. This should have been a simple one line change in each of the files. Now it's turned into hours of work just to add useless tests and documentation. All just to "check the boxes". If someone wants to pickup where I left off, feel free.
By the way, this is why I think Flutter has issues that stay open for years at a time. It's way too much of a hassle to contribute. These changes should be very quick and simple. Yet I have to write a test for each one, then document each method in the test. Either that, or ask for a test exemption. All of that on top of having to document why the set visible method is at the end of the method. It's very silly.
There was a problem hiding this comment.
Now it's turned into hours of work just to add useless tests and documentation. All just to "check the boxes".
The boxes exist because we have found that it's very easy for people to dismiss tests and documentation as useless in the context of the PR that they want to land, and very hard to maintain code where everyone makes that short-term tradeoff for their own convenience.
It's not that the policy exists because of the boxes. The boxes exist because of an intentional policy created to ensure the long-term health of the Flutter project.
By the way, this is why I think Flutter has issues that stay open for years at a time.
Flutter has issues that stay open for years at a time because it's a large project with millions of users, and our policy is to leave valid issues open. All software projects on a scale anywhere in the ballpark of Flutter's and with an open issue tracker receives bug reports and feature requests faster than they can be fixed; any project where the issue count and issue age doesn't reflect that reality has some other policy for issue management (e.g., there are large software projects that treat the issue tracker solely as a work-list for the core team, and close any issue that they don't plan on addressing themselves in the near future; others auto-close issues that aren't commented on every X months).
All of that on top of having to document why the set visible method is at the end of the method. It's very silly.
Does that mean your view is that it's self-evident that the set-visible method has to be at the end of the method? If so, that's a surprising position to take in light of the fact that the code previously didn't do that, meaning it demonstrably and objectively was not self-evident to other people changing and reviewing the code in the past (and thus, very likely, other people changing and reviewing the code in the future).
There was a problem hiding this comment.
The boxes exist because we have found that it's very easy for people to dismiss tests and documentation as useless in the context of the PR that they want to land
I understand that there has to be boxes there to align everyone on what the procedure and standards are. What I meant is that the automated tests on top of the comments is over kill in my opinion. In the context of this task, it felt like I was adding automated tests because the docs said so, rather than for a practical reason. That's not to say they should be thrown out altogether, I just think it's important to think critically about the value added vs. time spent depending on the situation.
I agree, the reason for the change may not be self evident. But I have a comment explaining why the line of code is at the bottom. Do I really need automated tests for it too? And on top of that document the automated tests?
I understand that with a large scale project, stability is important. However, when 20 minute task takes tens of hours and most of that time is writing comments and tests for a trivial change, it becomes a problem in my opinion. That time could have been spent fixing other issues which would improve stability quicker.
In my opinion, automated tests are there to prevent regression. I think a question that should be asked is: "If I don't write a test, is it somewhat likely that this change will regress?" In this case, since I have comment there explaining the change, I don't think it's worth it.
There was a problem hiding this comment.
But I have a comment explaining why the line of code is at the bottom. Do I really need automated tests for it too?
My experience, and the experience of a number of other leads in the Flutter project, is that comments are excellent for helping people understand code, but not very effective at preventing regressions, so our view is that yes, we need automated tests if we actually care about preventing regressions.
And on top of that document the automated tests?
The review comment that started this thread was not asking you to document automated tests. It was asking you to document production code. It happens to be declared in a _Test header because it's exposed for testing, but that doesn't make it test code.
That time could have been spent fixing other issues which would improve stability quicker.
Our collective experience within the Flutter team is that prioritizing landing changes faster at the expense of automated tests does not, in practice, improve stability over time.
The policy we have was designed with a great deal of thought, and based on extensive real-world experience. Obviously not everyone will agree with the tradeoffs, but the view that my review comments were made "because the docs said so, rather than for a practical reason" is simply not accurate.
I think a question that should be asked is: "If I don't write a test, is it somewhat likely that this change will regress?" In this case, since I have comment there explaining the change, I don't think it's worth it.
I understand that. And I, based on years of experience with this repository and decades of experience with open source in general, disagree.
In the future I would encourage you to consider the fact that just because someone reaches a different conclusion than you on this topic does not mean that they are "silly", or that they are doing something only "because the docs said so", or that it has never occurred to them to think about cost/benefit analysis of testing.
|
This will need to be resolved for the recent conversion to Pigeon data structures; sorry about that conflict. The change should be easy to resolve though since you can just move the relevant line to the end in the new version. |
|
@Colman Are you planning on updating this for the Pigeon changes? |
|
Closing per discussion above. |
4 participants
This changes the iOS maps package to ensure that each map object (Marker, Polygon, Polyline, Circle, and Tile) is added to the map after all of their properties (fillColor, strokeColor, etc.) are set. This fixes this issue:
flutter/flutter#143570
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style, or this PR is exempt from CHANGELOG changes.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.