Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Expose camera padding option #15444

Merged
merged 2 commits into from
Aug 29, 2019
Merged

Expose camera padding option #15444

merged 2 commits into from
Aug 29, 2019

Conversation

LukasPaczos
Copy link
Contributor

Closes #15436.

This PR exposes CameraPosition#padding and a bunch of utility methods around it. This gives a choice to set the padding lazily with MapboxMap#setPadding or immediately with MapboxMap#moveCamera(paddingTo(0, value, 0, 0)).

For example, both of the below will have the same effect:

mapboxMap.setPadding(0, 500, 0, 0);
mapboxMap.moveCamera(CameraUpdateFactory.newCameraPosition(mapboxMap.getCameraPosition()));
mapboxMap.moveCamera(CameraUpdateFactory.paddingTo(0, 500, 0, 0));

Additionally, this PR deprecates the lazy padding setters in favor of the camera property because now, the camera object can server use cases like what's mentioned in #15412 (comment):

We're navigating with top padding applied (0, 0, value, 0), the user clicks a POI and camera should travel to the POI that's centered on the screen. With the current behavior, the camera would jump when padding (0, 0, 0, 0) is applied to achieve that.

for example:

mapboxMap.moveCamera(CameraUpdateFactory.paddingTo(0, 1000, 0, 0));
mapboxMap.addOnMapClickListener(new MapboxMap.OnMapClickListener() {
  @Override
  public boolean onMapClick(@NonNull LatLng point) {
    mapboxMap.animateCamera(CameraUpdateFactory.newLatLngPadding(point, 0, 0, 0, 0));
    return false;
  }
});

/cc @astojilj

@LukasPaczos LukasPaczos added the Android Mapbox Maps SDK for Android label Aug 21, 2019
@LukasPaczos LukasPaczos requested a review from tobrun August 21, 2019 17:31
@LukasPaczos LukasPaczos force-pushed the lp-camera-padding-15436 branch from 39d816f to f26a767 Compare August 21, 2019 17:35
@LukasPaczos LukasPaczos added this to the release-ristretto milestone Aug 21, 2019
@LukasPaczos LukasPaczos force-pushed the lp-camera-padding-15436 branch 4 times, most recently from af99751 to 47a5d4e Compare August 26, 2019 13:29
@chloekraw chloekraw added the needs changelog Indicates PR needs a changelog entry prior to merging. label Aug 26, 2019
Copy link
Member

@tobrun tobrun left a comment

Choose a reason for hiding this comment

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

Looking good, can we add @Size annotations to the padding[] public API setters?

@LukasPaczos LukasPaczos force-pushed the lp-camera-padding-15436 branch from 47a5d4e to 3e29912 Compare August 28, 2019 14:07
When padding is not provided, the current one that's cached in the TransformState is going to be returned.
@LukasPaczos LukasPaczos force-pushed the lp-camera-padding-15436 branch from 3e29912 to 60c9c92 Compare August 28, 2019 14:14
@LukasPaczos LukasPaczos removed the needs changelog Indicates PR needs a changelog entry prior to merging. label Aug 28, 2019
Copy link
Member

@tobrun tobrun left a comment

Choose a reason for hiding this comment

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

Besides the removal of using Espresso in the animation tests this is ready to ship.
Let's do that as tail work of this PR.

This removes the cached insets on the Android side, making the core TransformState the source of truth. This still leaves an option to lazily set the padding which is going to be applied only when the next camera animation is started.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose camera padding option
3 participants