-
Notifications
You must be signed in to change notification settings - Fork 9.8k
Adding My Location parameter to the Google Maps plugin #898
Conversation
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.
Thanks for the contribution and the bug fix!
I left a few comments, we will also need to make the formatter presubmit happy before landing this.
Interesting that location can't be turned on again on iOS, what happens when you enabled it the second time? does it fail or just updates the property and nothing happens?
I'm also wondering about the "My location button", if I understand the documentation correctly it seems that for Google Maps on Android if you turn on "my location" you get the "my location" button as well unless you explicitly set it to off, and that on iOS the default is to not show the button.
At a minimum we should make sure the behavior is the same on both platforms (e.g explicitly set the button to true/false). Would be even better to add a "myLocationButton" option to the map options (though not a must as part of this PR).
Also note that I my availability is limited this week so I might be slow to follow up.
packages/google_maps_flutter/android/src/main/AndroidManifest.xml
Outdated
Show resolved
Hide resolved
packages/google_maps_flutter/example/ios/Runner.xcodeproj/project.pbxproj
Outdated
Show resolved
Hide resolved
...le_maps_flutter/android/src/main/java/io/flutter/plugins/googlemaps/GoogleMapController.java
Show resolved
Hide resolved
...le_maps_flutter/android/src/main/java/io/flutter/plugins/googlemaps/GoogleMapController.java
Outdated
Show resolved
Hide resolved
Thanks for the review! I modified the code so that both platforms show the My Location button along with the indicator, but I agree that eventually it would be good to build that out into a separate option. I'm seeing a bug on iOS however where the button doesn't animate the camera to their location. Other change: passing boolean from On the iOS on/off behavior, I found this open issue: https://issuetracker.google.com/issues/74362141. No notice on when it will be fixed. Also: I'm running the flutter plugins formatter but my iOS files are still failing. Do I have to fix them by hand? Plus it looks like a bunch of files outside this plugin are being flagged by the test. I'm going to do another commit to clean up formatting. |
The formatting mismatch was probably because you are using a newer clang-format than the one the CI system uses. |
Removed API keys Fixed typo Nitpicks, standard location button behavior, dart doc comments. Ran formatter, updated GoogleMapController initializer Remove keys
Ok I'm now using version 2018-10-04 of clang-format from Homebrew. Does that match with yours? |
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.
Looks like the CI formatter is green now.
Overall looks good, left a few comments and a question.
...le_maps_flutter/android/src/main/java/io/flutter/plugins/googlemaps/GoogleMapController.java
Outdated
Show resolved
Hide resolved
packages/google_maps_flutter/example/ios/Runner.xcodeproj/project.pbxproj
Outdated
Show resolved
Hide resolved
...ges/google_maps_flutter/example/ios/Runner.xcworkspace/xcshareddata/IDEWorkspaceChecks.plist
Outdated
Show resolved
Hide resolved
...google_maps_flutter/example/ios/Runner.xcworkspace/xcshareddata/WorkspaceSettings.xcsettings
Outdated
Show resolved
Hide resolved
|
||
GoogleMapController build( | ||
int id, Context context, AtomicInteger state, PluginRegistry.Registrar registrar) { | ||
final GoogleMapController controller = | ||
new GoogleMapController(id, context, state, registrar, options); | ||
new GoogleMapController(id, context, state, registrar, options, myLocationEnabled); |
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.
Can you explain again why can't we follow the same pattern as setTrackCameraPosition...
e.g after controller.init
call controller.setMyLocationEnabled
?
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.
From debugging, it appears that setMyLocationEnabled
in GMController is called before onMapReady
. So if it that function is called there in GMBuilder, it will in turn call googleMap.setMyLocationEnabled
before googleMap is initiated by onMapReady
.
If we keep the same pattern, setMyLocationEnabled
has to do two different things. When it is being called from the Builder, googleMap
is null, so it can't call googleMap
's functions. But when the user updates map options it does have to access googleMap
directly. The trackCamera
option doesn't need to update googleMap
directly.
In my new commit, I split the setLocationEnabled
function into two pieces, one that is called by the builder and one that is called when the map options are updated, to illustrate this problem.
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.
Thanks for the explanation, now I get it 😄
What do you think about calling an updateMyLocationEnabled()
from onMapReady
which will set the map's my location enabled field to whatever the value of the member myLocationEnabled
is.
Then have setMyLocationEnabled
to do nothing if the new value is the same as the current one, or if we're not yet initialized, otherwise update myLocationEnabled
(the member) and call updateMyLocationEnabled()
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.
Okay I did something like that. setMyLocationEnabled
skips updateMyLocation
if the map is null, and during map initiation updateMyLocation
is called.
From the docs its unclear to me if you need to check permissions to setMyLocationEnabled
to false
, so I check them no matter if you are turning it on or off, that might not be right.
…, changed setLocationEnabled function
...le_maps_flutter/android/src/main/java/io/flutter/plugins/googlemaps/GoogleMapController.java
Outdated
Show resolved
Hide resolved
...le_maps_flutter/android/src/main/java/io/flutter/plugins/googlemaps/GoogleMapController.java
Outdated
Show resolved
Hide resolved
...le_maps_flutter/android/src/main/java/io/flutter/plugins/googlemaps/GoogleMapController.java
Outdated
Show resolved
Hide resolved
packages/google_maps_flutter/example/android/app/src/main/AndroidManifest.xml
Outdated
Show resolved
Hide resolved
|
||
GoogleMapController build( | ||
int id, Context context, AtomicInteger state, PluginRegistry.Registrar registrar) { | ||
final GoogleMapController controller = | ||
new GoogleMapController(id, context, state, registrar, options); | ||
new GoogleMapController(id, context, state, registrar, options, myLocationEnabled); |
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.
Thanks for the explanation, now I get it 😄
What do you think about calling an updateMyLocationEnabled()
from onMapReady
which will set the map's my location enabled field to whatever the value of the member myLocationEnabled
is.
Then have setMyLocationEnabled
to do nothing if the new value is the same as the current one, or if we're not yet initialized, otherwise update myLocationEnabled
(the member) and call updateMyLocationEnabled()
…initiation and later.
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.
Looks good, just revert the Runner.xcscheme rename.
I reverted the deletion of |
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.
LGTM, thanks for the contribution!
Will land on green.
Introduces a My Location parameter to the Google Maps plugin. This is a feature that Google Maps offers that puts a blue location dot on the user's location, and provides a clickable compass. This parameter works the same way as the zoom, or scale options, and can be enabled or disabled by updating the map option.
This pull request introduces a My Location parameter to the Google Maps plugin. This is a feature that Google Maps offers that puts a blue location dot on the user's location, and provides a clickable compass. This parameter works the same way as the zoom, or scale options, and can be enabled or disabled by updating the map options. I've added:
I also fixed a bug in the iOS code which was improperly loading the map options when the map was first initiated.
Things I am uncertain about: