[google_maps_flutter_ios] Fix iOS regression where updating a marker hides its info window #9127
Conversation
jokerttu
changed the title
| @@ -837,6 +846,26 @@ void main() { | |||
| iwVisibleStatus = await controller.isMarkerInfoWindowShown(marker.markerId); | |||
| expect(iwVisibleStatus, true); | |||
|
|
|||
| // Update marker and check if the info window is still visible. | |||
There was a problem hiding this comment.
I think it would be better to make this a separate test ('updating a marker does not hide its info window') rather than adding it to this test. Yes, there will be some duplicate boilerplate, but the fewer distinct things a test is testing, the easier each test is to maintain, debug, and understand failure in.
There was a problem hiding this comment.
Oh, had this as separate test first, but desided to reduce test execution time as I saw this as part of info window toggle test.
Separated to own test as suggested.
| @@ -127,6 +129,10 @@ - (void)updateFromPlatformMarker:(FGMPlatformMarker *)platformMarker | |||
| } | |||
|
|
|||
| [self setVisible:platformMarker.visible]; | |||
There was a problem hiding this comment.
Per investigation in #7001, this should always be the last thing in these methods. We don't yet have a complete PR that tests and enforces that, but we should still try to not make it worse in the meantime.
There was a problem hiding this comment.
Moved the method call to last with comment (so that it is kept there in future changes as well).
|
|
||
| if ([controller.marker conformsToProtocol:@protocol(GMUClusterItem)]) { | ||
| if (previousClusterManagerIdentifier && | ||
| ![previousClusterManagerIdentifier isEqualToString:clusterManagerIdentifier]) { |
There was a problem hiding this comment.
if (previousClusterManagerIdentifier && ![clusterManagerIdentifier isEqualToString:previousClusterManagerIdentifier])
In general, a lot of these kind of framework methods don't handle nil arguments, so we shouldn't pass in a potentially nil clusterManagerIdentifier). This ordering will end up with [nil isEqualToString:<a value we've established as non-nil>], which gives us the intended result of NO without passing nil as an argument.
|
Thanks for investigating my speculative theory :) I didn't have time to context switch to testing maps locally. |
jokerttu
force-pushed
the
fix/regression-in-displaying-info-windows-over-marker-updates
branch
from
cfd759a to
8f417df
Compare
jokerttu
added
the
autosubmit
… a marker hides its info window (flutter/packages#9127)
flutter/packages@41211cf...ae4f17d 2025-04-23 [email protected] [camerax] Ensure DeviceOrientationManager stops on dispose (flutter/packages#9141) 2025-04-23 [email protected] [google_maps_flutter_ios] Fix iOS regression where updating a marker hides its info window (flutter/packages#9127) 2025-04-23 [email protected] Redistribute iOS CODEOWNERS (flutter/packages#9129) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…hides its info window (flutter#9127) Fixes a regression introduced on version 2.12.0: _Adds support for marker clustering_ (flutter#6186), where visible info windows are not visible after marker updates. Adds tests accordingly. Fixes flutter/flutter#167459 Related to flutter/flutter#159471 ## Pre-Review Checklist [^1]: Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling.
Fixes a regression introduced on version 2.12.0: Adds support for marker clustering (#6186), where visible info windows are not visible after marker updates.
Adds tests accordingly.
Fixes flutter/flutter#167459
Related to flutter/flutter#159471
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.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
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