Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

Conversation

@stefalda
Copy link

The method isMarkerInfoWindowShown in file method_channel_google_maps_flutter.dart isn't working with null safety because the channel(mapId).invokeMethod has an optional return type (<T?>), so forcing it to bool doesn't work.

In the original code a cast has been added to silence the compiler warnings but there is a runtime error when the method is invoked because the signature invokeMethod should be invokeMethod<bool?>.

This is a fix for issue 78426 where there's the original code.

The method isMarkerInfoWindowShown in file method_channel_google_maps_flutter.dart isn't working with null safety because the channel(mapId).invokeMethod has an optional return type, so forcing it to bool doesn't work.

In the original code a cast has been added to silence the compiler warnings but there is a runtime error when the method is invoked because the signature invokeMethod should be invokeMethod<bool?>.
@stefalda stefalda requested a review from cyanglaz as a code owner March 17, 2021 17:13
@google-cla
Copy link

google-cla bot commented Mar 17, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@stefalda
Copy link
Author

@googlebot I signed it!

@google-cla google-cla bot added cla: yes and removed cla: no labels Mar 17, 2021
Copy link
Contributor

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, this was generated by the migration tool; I wonder why it generated that. Thanks for the contribution!

Please add a test that exercises this codepath (i.e., that fails without your fix and passes with it) per Flutter PR policy and the PR template checklist. This will also need a version and CHANGELOG update per the checklist (in the future, please don't delete the checklist when opening a PR; it's there to help smooth the process).

@google-cla
Copy link

google-cla bot commented Mar 22, 2021

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes labels Mar 22, 2021
@stefalda
Copy link
Author

@googlebot I fixed it

@google-cla
Copy link

google-cla bot commented Mar 22, 2021

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@stuartmorgan-g
Copy link
Contributor

Thanks for the version update! Are you planning on adding the test coverage as well, or do you need feedback on something first?

@ditman
Copy link
Member

ditman commented Mar 23, 2021

Missing CLA for M@tilde07, not sure what commit triggered that.

@stefalda
Copy link
Author

Missing CLA for M@tilde07, not sure what commit triggered that.

Sincerely I don't understand either, I was surprised it asked me to sign the CLA again...

@stefalda
Copy link
Author

Thanks for the version update! Are you planning on adding the test coverage as well, or do you need feedback on something first?

I've tried but I've not been able to find the right test where to put the logic, because most of the tests use a fakeGoogleMap instead of the real one, so I'm unable to reproduce the problem that I've discovered while building a real app...

@stuartmorgan-g
Copy link
Contributor

Missing CLA for M@tilde07, not sure what commit triggered that.

Sincerely I don't understand either, I was surprised it asked me to sign the CLA again...

The "Updated version and changelog" commit appears to be using a different email address; notice that it doesn't have your profile image. You probably need to ammend that commit locally to use this account's email address, and then force push.

description: A Flutter plugin for integrating Google Maps in iOS and Android applications.
homepage: https://github.com/flutter/plugins/tree/master/packages/google_maps_flutter/google_maps_flutter
version: 2.0.1
version: 2.0.2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I hadn't looked at this closely enough; you've updated the version and changelog of the app-facing package, rather than the platform interface package where you made the change. You need to update the version of the package that your changes are in.

@stuartmorgan-g
Copy link
Contributor

Thanks for the version update! Are you planning on adding the test coverage as well, or do you need feedback on something first?

I've tried but I've not been able to find the right test where to put the logic, because most of the tests use a fakeGoogleMap instead of the real one, so I'm unable to reproduce the problem that I've discovered while building a real app...

The test should go in this group. Currently there are no realy tests there it looks like (just a placeholder that should be removed), but you can look at, e.g., this to see what a test that sets mock method channel data looks like.

@hlavki
Copy link

hlavki commented Mar 24, 2021

@stefalda can be this problem related also to issue #78856? thanks

@stefalda
Copy link
Author

@stefalda can be this problem related also to issue #78856? thanks

Yes, I guess that it could be the same problem just in another method call

@stuartmorgan-g
Copy link
Contributor

Since this is a more general problem, I'll follow up shortly with a PR that tests all of the APIs that could have this issue, and fixes all of them, which will replace this one.

@stuartmorgan-g
Copy link
Contributor

Closing in favor of #3754. Thanks for the clear issue report, debugging, and PR, which definitely accelerated the fix here!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants