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

Add map padding to the MapboxMap object #3761

Closed
zugaldia opened this issue Jan 29, 2016 · 27 comments · Fixed by #3942
Closed

Add map padding to the MapboxMap object #3761

zugaldia opened this issue Jan 29, 2016 · 27 comments · Fixed by #3942
Assignees
Labels
Android Mapbox Maps SDK for Android

Comments

@zugaldia
Copy link
Member

When padding is added to a map, the map continues to fill the entire container, but text and control positioning, map gestures, and camera movements will behave as if it has been placed in a smaller space. Android's corresponding implementation is described here.

Related (MapboxMap class): #3145

/cc: @tobrun @bleege

@zugaldia zugaldia added the Android Mapbox Maps SDK for Android label Jan 29, 2016
@tobrun
Copy link
Member

tobrun commented Jan 29, 2016

Will be added as:

public final void setPadding (int left, int top, int right, int bottom)

Other methods mentioned in that section are here:

@tobrun tobrun added this to the android-v4.0.0 milestone Jan 29, 2016
@bleege
Copy link
Contributor

bleege commented Jan 29, 2016

Will this be related to Content Insets in #3627?

@tobrun
Copy link
Member

tobrun commented Feb 2, 2016

@bleege they seem related,
I need to do some testing with Google Maps before giving a definite answer on this.

@tobrun
Copy link
Member

tobrun commented Feb 10, 2016

Been looking into the setPadding by playing around with Google maps SDK in a sample app,
and it seems that setPadding is the actual method for content insets on Google Maps!

Update:
Here is a short Youtube video showcasing this.

@tobrun tobrun mentioned this issue Feb 10, 2016
@tobrun
Copy link
Member

tobrun commented Feb 10, 2016

This is an example of the feature activity for without calling setPadding:
device-2016-02-10-100109\

What needs to happen:

  • expose setPadding in MapboxMap
  • Map itself should be offset (the marker must be in the center of the non blue area)
  • Overlayn items as MapboxLogo should take this padding into account

@tobrun
Copy link
Member

tobrun commented Feb 10, 2016

After hooking up the setPadding method. I'm not seeing any changes with prior screenshot..
Also looking into screenshots posted in #3627, I'm not seeing any differences as well..
Not sure if this has actually has worked before..

Update:
now seeing @bleege comment #3687 (comment). This apparently doesn't work with camera API yet..

Update:
apparently it does work with the camera API.. you just need to call setPadding before using the Camera.

device-2016-02-10-104045

@tobrun
Copy link
Member

tobrun commented Feb 10, 2016

Now that I have a better understanding about the missing pieces required to have this on Android:

  • fix API that we can always call setPadding without requirement of calling camera api
  • Overlain views should take in account the map padding
  • Expose a setPadding on UserLocationView separately (for navigation)

@lgeromegnace
Copy link

Hi @tobrun, it's good to see this subject come back to front.

About your comment, I was thinking may be setPadding should update camera once it has been set ?
This should avoid the feeling the insets are not taken into account immediately.

@lgeromegnace
Copy link

Ha I see your last comment too late :).

@tobrun
Copy link
Member

tobrun commented Feb 10, 2016

@lgeromegnace, awesome and thanks again for the contribution!

@tobrun
Copy link
Member

tobrun commented Feb 10, 2016

For the overlain views I'm going to introduce a new class for general ui management of an overlain view.
This will be managed by UiSettings class and will expose 3 attributes:

  • enabled
  • gravity
  • margins

The margins from above must be updated if we set a certain padding, but most importantly it must only update margins in a certain direction (based on gravity attribute).

update:
I'm going to call the class ViewSettings since this is used in UiSettings, this is definitly not the final name, but will leave if for now until the whole overlain view architecture becomes more clear for #3276.

update:
Apparently we don't have any tests for UiSettings! Going to add this as a part of this ticket.

update:
Ok we actually do already have unit tests for UiSettings. My build target was not correctly set :trollface:

@tobrun
Copy link
Member

tobrun commented Feb 10, 2016

We will need to provide a way to invalidate the margins/gravity associated with a ViewSettings. After calling setPadding on MapboxMap

@tobrun
Copy link
Member

tobrun commented Feb 10, 2016

This is the result with invalidate in place:

device-2016-02-10-131146

I'm going to try to find some edge cases that might need some work before continuing with other items.

@tobrun
Copy link
Member

tobrun commented Feb 10, 2016

device-2016-02-10-163225

I was not able to find any strange edge cases. Next up is UserLocationView.

@tobrun
Copy link
Member

tobrun commented Feb 10, 2016

UserLocationView with padding, next up is exposing an API to offset UserLocationView

device-2016-02-10-172449

@tobrun
Copy link
Member

tobrun commented Feb 10, 2016

Noticing that I also need to update some code related to gestures

eg. zoom should take in account the padding.

@tobrun
Copy link
Member

tobrun commented Feb 12, 2016

To correctly manage the change in #3899 I need to find some code on a stale branch.

@tobrun
Copy link
Member

tobrun commented Feb 12, 2016

This is the refactored stale code:

    private void validateGesturesForTrackingModes() {
        if(!dismissTrackingOnGesture) {
            int myLocationTrackingMode = getMyLocationTrackingMode();
            int myBearingTrackingMode = getMyBearingTrackingMode();

            // Enable/disable gestures based on tracking mode
            if (myLocationTrackingMode == MyLocationTracking.TRACKING_NONE) {
                uiSettings.setScrollGesturesEnabled(true);
                uiSettings.setRotateGesturesEnabled(true);
            } else {
                uiSettings.setScrollGesturesEnabled(false);
                uiSettings.setRotateGesturesEnabled((myBearingTrackingMode == MyBearingTracking.NONE));
            }
        }
    }

Code above will enable/disable certain gestures if we are not automatically dismissing tracking modes.

@tobrun
Copy link
Member

tobrun commented Feb 12, 2016

I was able to correctly calculate center of gesture in case of tracking mode 💥
See result here:
https://www.dropbox.com/s/p95k6qh7z6kdsxc/device-2016-02-12-130800.mp4?dl=0

@tobrun
Copy link
Member

tobrun commented Feb 12, 2016

All usecases of gestures are working except for tilt..
https://www.dropbox.com/s/0q0wh6asjgokfpa/device-2016-02-12-145442.mp4?dl=0

@tobrun
Copy link
Member

tobrun commented Feb 12, 2016

Apparently to fix above we first need to expose this through jni:

void Map::setPitch(double pitch, const PrecisionPoint& anchor, const Duration& duration) {
    transform->setPitch(pitch * M_PI / 180, anchor, duration);
    update(Update::Repaint);
}

currently we are using:

void Map::setPitch(double pitch, const Duration& duration) {
    setPitch(pitch, {NAN, NAN}, duration);
}

@tobrun
Copy link
Member

tobrun commented Feb 12, 2016

After 👊 with JNI I was able to compile and call the native method.
Only the anchor point used should not be the same as with the other gestures. there we are rotating/scaling around UserLocationView-center...

@tobrun
Copy link
Member

tobrun commented Feb 12, 2016

Going to punt on above and move back to fix API that we can always call setPadding without requirement of calling camera api

On iOS this is handled here 6a0b7f8#diff-20502b6bc02986a4398d0da84b2d4b94R808

We just need to reset CameraPosition to current position.

@tobrun
Copy link
Member

tobrun commented Feb 12, 2016

💥 I was able to implement above:
See result here:
https://www.dropbox.com/s/b9ea31qteyb6oii/device-2016-02-12-175452.mp4?dl=0

We now just need 2 more things to close this:

  • support map padding when doing a tilt gesture
  • add UserLocationView padding (attributes are in place but need to shuffle with calculations)

@tobrun
Copy link
Member

tobrun commented Feb 15, 2016

Going to punt on current progress and add a PR for current progress.
Will create 2 other issues for above.

tobrun added a commit that referenced this issue Feb 15, 2016
[android] #3781 #3899 - add map padding, user location view, ViewSettings, TrackingSettings

[android] #3761 - update map padding sample

[android] #3761 - add map padding
@tobrun
Copy link
Member

tobrun commented Feb 15, 2016

Follow up issues:
Tilt gesture with padding and user location tracking Android bug #3947
Add padding to UserLocationView Android feature request #3946

@bleege
Copy link
Contributor

bleege commented Feb 15, 2016

@tobrun Good call on breaking this issue up into smaller tasks. Gets the working code in quicker and brings more clarity to the remaining issues. 👍

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 a pull request may close this issue.

6 participants