Location View plugin #22
Conversation
cammace
added
enhancement
location-layer-plugin
|
Basic user location icon using the optional
Using mode
Compass bearing is also supported.
Note that the cc: @mapbox/android |
|
@cammace Love seeing this effort back in motion! Let's build an MVP that we can merge on the earlier side. We can always iterate later to make it more complete. |
cammace
force-pushed
the
cam-location-view
branch
from
41e6c12
to
b7aeca8
Compare
cammace
added
ready for review
|
I had to add an API for manually controlling the user location since in the Navigation SDK, snapping to route provides a location object which currently will need to be passed to the location layer to actually snap the location. |
|
Tried running this on a Nexus 5 running KitKat, crashes at startup: |
|
Not handling any runtime permissions |
| @BindView(R.id.button_location_mode_navigation) | ||
| Button locationModeNavigationButton; | ||
|
|
||
| private MyLocationLayerOptions options; |
There was a problem hiding this comment.
Do we really want an options object to configure a plugin? The options paradigm, that is used in the main SDK was introduced to separate setters/getters from MapboxMap and inspired by similar constructs in the Gmaps SDK. Since the Location plugin is standalone. I don't feel we need a separate options object to make it more complicated.
| @@ -27,6 +29,30 @@ | |||
| android:name="android.support.PARENT_ACTIVITY" | |||
| android:value=".activity.FeatureOverviewActivity"/> | |||
| </activity> | |||
|
|
|||
| <activity | |||
| android:name=".activity.location.MyLocationModesActivity" | |||
There was a problem hiding this comment.
We used the My prefix for API compability, with this plugin this isn't a requirement anymore. I would suggest naming all things just Location instead of MyLocation.
| @BindView(R.id.button_location_mode_tracking) | ||
| Button locationModeTrackingButton; | ||
| @BindView(R.id.button_location_mode_navigation) | ||
| Button locationModeNavigationButton; |
There was a problem hiding this comment.
Why do you bind these buttons? Just having the @OnClick is sufficient.
| private CompassListener compassListener; | ||
|
|
||
| private Sensor rotationVectorSensor; | ||
| float[] matrix = new float[9]; |
| * | ||
| * @since 0.1.0 | ||
| */ | ||
| class MyLocationLayerConstants { |
There was a problem hiding this comment.
Are these constants used all around the codebase?
If not, these should be encapsulated by the respective classes themselves.
| <resources> | ||
|
|
||
| <color name="mapbox_plugin_blue">#4882C6</color> | ||
| <color name="mapbox_plugin_gray">#263D57</color> |
There was a problem hiding this comment.
this resource is placed in a specific plugin folder, thus the name should be more unique to avoid id conflicts.
| @@ -0,0 +1,3 @@ | |||
| <resources> | |||
| <string name="app_name">Location View</string> | |||
| </resources> | |||
| * | ||
| * @since 0.1.0 | ||
| */ | ||
| @SuppressWarnings( {"MissingPermission"}) |
There was a problem hiding this comment.
Instead of ignoring, enforcing runtime permissions with the correct annotations on the public API would be cleaner.
| options.removeLayerAndSources(); | ||
| } | ||
| } | ||
| this.myLocationLayerMode = myLocationLayerMode; |
There was a problem hiding this comment.
There are a lot of if/else checking done in this method, this could re factored with using design pattern
| * @param location where you'd like the location icon to be placed on the map | ||
| * @since 0.1.0 | ||
| */ | ||
| public void setMyLocation(Location location) { |
There was a problem hiding this comment.
Is null allowed here? Either annotate public api with Nullable or NonNull to make the purpose clear for the end user
| * @since 0.1.0 | ||
| */ | ||
| @SuppressWarnings( {"MissingPermission"}) | ||
| public class MyLocationLayerPlugin implements LocationEngineListener, CompassListener { |
There was a problem hiding this comment.
This class could be decoupled into more smaller focused classes, as a result this would shrink most methods to a couple of lines.
| private MapboxMap mapboxMap; | ||
| private MapView mapView; | ||
|
|
||
| // Enabled booleans |
There was a problem hiding this comment.
Not sure if I understand what Enabled booleans means
|
Much of the changes have been implemented, the code still needs a cleanup and I could use some help debugging a native crash. I'm trying to implement onMapChange event to handle when the map style changes but I receive a crash immediately when the style changes and a new location update occurs. I've checked that all my sources and layers are getting created and added again to the new styles. @tobrun would you have any idea on resolving this? |
|
Hey!
|
|
Hey @Grsmto! Glad to hear this plugin's working for you. If you pull down the latest code, you should be able to change the location layer drawables through your app's style.xml file: For setting the custom location engine, this should be done through the maps |
|
I symbolicated (with little success) the crash log I posted above: I eventually found that an animation update listener was causing the crash and now remove it right before the style begins to get loaded which fixes the issue. I'm now running into an issue with duplicate location events causing the animation to abruptly end after ~15 ms and then interpolates the same point for the actual time since the events happen right after another. I remember this being an issue in LOST. I'd also like to add the Lifecycle callbacks to make this plugin even easier to use. |
|
I've added the lifecycle observers and had to upgrade to the 5.1.0 beta since 5.0.2 was producing duplicate locationChange events. This PR's not ready for your eyes @tobrun @Guardiola31337 @zugaldia |
I meant to say it is now ready for your eyes 😄 |
cammace
force-pushed
the
cam-location-view
branch
from
75b7341
to
85a5407
Compare
|
Bearing not correctly restored when toggling styles:
|
Makefile
Outdated
|
|
||
| javadoc: | ||
| # Android modules | ||
| # Output is ./mapbox/*/build/docs/javadoc/release | ||
| cd plugins; ./gradlew :traffic:javadocrelease | ||
| cd plugins; ./gradlew :locationlayer:javadocrelease |
| mapView.onLowMemory(); | ||
| } | ||
|
|
||
| private static class StyleCycle { |
There was a problem hiding this comment.
This is now duplicated between activities, maybe extract in an utils instead?
| public void onMapReady(MapboxMap mapboxMap) { | ||
| this.mapboxMap = mapboxMap; | ||
| locationLayerPlugin = new LocationLayerPlugin(mapView, mapboxMap); | ||
| getLifecycle().addObserver(locationLayerPlugin); |
plugins/dependencies.gradle
Outdated
| mapSdk : 'com.mapbox.mapboxsdk:mapbox-android-sdk:5.1.0', | ||
| mapboxServices : 'com.mapbox.mapboxsdk:mapbox-android-services:2.1.3', | ||
|
|
||
| // lifecycle |
plugins/dependencies.gradle
Outdated
| mapboxServices : 'com.mapbox.mapboxsdk:mapbox-android-services:2.1.3', | ||
|
|
||
| // lifecycle | ||
| lifecycleRuntime : 'android.arch.lifecycle:runtime:1.0.0-alpha3', |
| this.compassListener = compassListener; | ||
| } | ||
|
|
||
| void onStart() { |
There was a problem hiding this comment.
could we leverage constructs from the new architecture libs to avoid having methods as onStart/onPause?
There was a problem hiding this comment.
The onStart and onStop still need to be called from LocationLayerPlugin since an observer is only optional. If I added a lifecycle annotation here, it wouldn't resolve me having to call the onStart methods unless you had something else in mind and I misunderstood.
| * @since 0.1.0 | ||
| */ | ||
| @RequiresPermission(anyOf = {Manifest.permission.ACCESS_FINE_LOCATION, Manifest.permission.ACCESS_COARSE_LOCATION}) | ||
| public void setLocationLayerEnabled(@LocationLayerMode.Mode int locationLayerMode) { |
There was a problem hiding this comment.
code in this method could be refactored into smaller parts
|
|
||
| @SuppressWarnings( {"MissingPermission"}) | ||
| @Override | ||
| public void onMapChanged(int change) { |
There was a problem hiding this comment.
The code below could be refactored into smaller parts
| * @param bearingEnabled boolean true if you'd like to enable the user location bearing, otherwise, false will disable | ||
| * @since 0.1.0 | ||
| */ | ||
| private void setMyBearingEnabled(boolean bearingEnabled) { |
There was a problem hiding this comment.
code below could be simplified and lower the amounts of if/else blocks
| * @param magneticHeading the raw compass heading | ||
| * @since 0.1.0 | ||
| */ | ||
| private void bearingChangeAnimate(float magneticHeading) { |
There was a problem hiding this comment.
code below could be refactored into smaller parts
|
The compass orientation matrix now supports remapping the axes when the device is rotated past 90 degrees in the y-axis. Previously with this plugin or the original |
|
@cammace CI is complaining about some NPE on the LineLayer, could you 👀 that before merging? Thanks. |
cammace
force-pushed
the
cam-location-view
branch
from
6c811a0
to
93880b6
Compare
This PR adds a location view plugin that makes use of runtime styling instead of drawing a view on top of the map as discussed in this issue and older PR.