Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove dynamic padding way name adjustment for MapWayname #1473

Merged
merged 1 commit into from
Oct 31, 2018

Conversation

danesfeder
Copy link
Contributor

@danesfeder danesfeder commented Oct 29, 2018

Closes #1465

As reported by @hurrba, we are dynamically adjusting the map top padding based on the visibility of the way name pill. If a developer want's to adjust the top padding themselves, or just wants to center the icon, we should make this easier and not override with unexpected behavior.

ezgif com-video-to-gif 1

This PR also adds a more straightforward API to center the location icon, which also let's us account for centering when rotating the device:

// To center the icon
int[] customPadding = {0, 0, 0, 0};
navigationView.retrieveNavigationMapboxMap().updateLocationIconWith(customPadding);

@danesfeder danesfeder added bug Defect to be fixed. ✓ ready for review backwards incompatible Requires a SEMVER major version change. labels Oct 29, 2018
@danesfeder danesfeder added this to the v0.23.0 milestone Oct 29, 2018
@danesfeder danesfeder self-assigned this Oct 29, 2018
@danesfeder danesfeder changed the title [WIP] Remove dynamic padding way name adjustment for MapWayname Remove dynamic padding way name adjustment for MapWayname Oct 29, 2018
Copy link

@hurrba hurrba left a comment

Choose a reason for hiding this comment

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

looks fine, I've added a single comment in the code.

Also, while this solves the issue with centering the puck (which is what we need), I can't help but think that it could(should?) be a bit more generalized. if you want the puck to be in the top of the screen (e.g. in order to get a better overview when travelling south-bound with NavigationCamera.NAVIGATION_TRACKING_MODE_NORTH), the puck would still jump between top of screen and either center of screen (if updateLocationIconPosition(true)) or bottom of screen.

@@ -324,8 +325,8 @@ public void resumeCamera(@NonNull Location location) {
* @param trackingCameraMode the tracking mode
*/
public void resetCameraPositionWith(@NavigationCamera.TrackingMode int trackingCameraMode) {
updateLocationIconPosition(false);
Copy link

Choose a reason for hiding this comment

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

should probably use mapPaddingAdjustor.isCentered() instead of false? Otherwise, clicking the recenterButton will reset the padding.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hurrba yeah great catch, thanks!

@danesfeder
Copy link
Contributor Author

I can't help but think that it could(should?) be a bit more generalized.

@hurrba you're saying the API should be more specific rather than just a toggle?

@hurrba
Copy link

hurrba commented Oct 30, 2018

you're saying the API should be more specific rather than just a toggle?

my point was that instead of keeping track of isCentered, we could have an array, say customMapPadding, that keeps the custom padding set by the developer (if any).

the logic would be similar, but instead of checking for isCentered here, we check if customMapPadding is set and use that value if it is.

this would account for people setting the padding to anything other than center or default (bottom of screen). again, the current solution solves our problem, but others might have other needs.

edit: this is only an issue when rotating the screen.

@danesfeder
Copy link
Contributor Author

@hurrba yeah I see what you mean - let me work a bit more and see if I run into any snags implementing. If I do, we can ticket as a future improvement. I don't want this PR to sit / continue to block you all for too long.

@danesfeder danesfeder force-pushed the dan-padding-adjust branch 2 times, most recently from 192739b to 9c9706a Compare October 30, 2018 18:23
@danesfeder
Copy link
Contributor Author

@hurrba I update the APIs here more aligned with what we discussed. A custom int[] can now be provided and we will consider this padding for the life of the view thereafter. With this current setup, to return to the default, you'd need to cache [the default] on your side and set again.

This PR also considers the custom padding during rotation / when clicking the re-center button. We will always zero out the padding when clicking the overview button.

@hurrba
Copy link

hurrba commented Oct 31, 2018

@danesfeder I've tested the latest changes and it seems to be working as it should! :)
not sure if I can/should approve, but you have my thumbs up 👍

Copy link
Contributor

@Guardiola31337 Guardiola31337 left a comment

Choose a reason for hiding this comment

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

I've left a couple of minor comments.

Also, I'm wondering how's this SEMVER. I think we're not breaking any APIs here 🤔 Could you clarify? If it's because of NavigationMapboxMapInstanceState#isCameraTracking, NavigationMapboxMapInstanceState is already package private so without an instance you cannot access to it so I believe that wouldn't break SemVer. Actually, the "public" methods included in NavigationMapboxMapInstanceState should be package private - see comment below re Access can be package-private.

updatePaddingWith(defaultPadding);
}

void updatePaddingWithZero() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor comment - What about "exposing" (package private) updatePaddingWith instead and removing this method?

@@ -30,8 +33,8 @@ public String retrieveWayname() {
return waynameText;
}

public boolean isCameraTracking() {
return isCameraTracking;
public MapPaddingInstanceState retrieveMapPadding() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Access can be package-private

The same thing happens with isWaynameVisible, retrieveWayname and retrieveMapPadding methods in NavigationMapboxMapInstanceState.

@danesfeder
Copy link
Contributor Author

Glad to hear it @hurrba, thanks for the great feedback.

@Guardiola31337 this is ready for another round 👀

@danesfeder danesfeder removed the backwards incompatible Requires a SEMVER major version change. label Oct 31, 2018
Copy link
Contributor

@Guardiola31337 Guardiola31337 left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the feedback @danesfeder 🚢

@danesfeder danesfeder merged commit 439c501 into master Oct 31, 2018
@danesfeder danesfeder deleted the dan-padding-adjust branch October 31, 2018 15:47
@Guardiola31337
Copy link
Contributor

Hey @hurrba 👋 friendly reminder that OP landed and as you mentioned fixed your issue. You can retest with 0.23.0-SNAPSHOT to re-reconfirm 😂 This will be in the next release. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Defect to be fixed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants