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

Remove deprecated MarkerView api #13194

Merged
merged 1 commit into from
Nov 14, 2018
Merged

Remove deprecated MarkerView api #13194

merged 1 commit into from
Nov 14, 2018

Conversation

tobrun
Copy link
Member

@tobrun tobrun commented Oct 25, 2018

Closes #13169

@tobrun tobrun added the Android Mapbox Maps SDK for Android label Oct 25, 2018
@tobrun tobrun added this to the android-v7.0.0 milestone Oct 25, 2018
@tobrun tobrun self-assigned this Oct 25, 2018
@tobrun tobrun requested a review from LukasPaczos October 25, 2018 12:52
Copy link
Contributor

@LukasPaczos LukasPaczos left a comment

Choose a reason for hiding this comment

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

Looks great! There are some leftover drawables that were used with MarkerViews, or even MyLocationView, maybe we can clean those up as a part of this PR as well?

@Override
protected void onCreate(Bundle savedInstanceState) {
super.onCreate(savedInstanceState);
setContentView(R.layout.activity_marker_view);
Copy link
Contributor

Choose a reason for hiding this comment

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

We can also remove this layout.

@Override
protected void onCreate(Bundle savedInstanceState) {
super.onCreate(savedInstanceState);
setContentView(R.layout.activity_marker_view_in_rect);
Copy link
Contributor

Choose a reason for hiding this comment

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

We can also remove this layout.

@@ -169,22 +164,10 @@ void onDestroy() {
* Called before the OnMapReadyCallback is invoked.
*/
void onPreMapReady() {
invalidateCameraPosition();
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, it might be beneficial to keep transform.invalidateCameraPosition() call here to always have a non-null camera position once the control is handled to the user. I believe we refactored Transform in the past to always, even internally, use Transform#getCameraPostion instead of the cameraPosition field and avoid NPEs (unless core returns a null camera position), but just to future proof, we can invalidate that position here as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

The use-cases are handled correctly by transform.


  @UiThread
  public final CameraPosition getCameraPosition() {
    if (cameraPosition == null) {
      cameraPosition = invalidateCameraPosition();
    }
    return cameraPosition;
  }
but just to future proof, we can invalidate that position here as well.

hardening and thus future proofing is handled in Transform.java, if there is no concrete indication to keep this use-case around. I would prefer not doing any obsolete work.

Copy link
Member Author

Choose a reason for hiding this comment

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

alternatively, I'm ok with keeping that onPreMapReady hook but let's remove the hardened check in Transform#getCameraPosition() instead

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm referring to a scenario when we would use Transform#cameraPosition field instead of the Transform#getCameraPosition method to perform operations on the camera object inside the Transform class. All of those usages have been refactored to use Transform#getCameraPosition, but I'm worried they might pop up again.

However, on the second thought, I'm not really sure whether we should perform obsolete calls to harden that, tests should be the way to go. Let's remove the call now, as it's not needed, and try to expnad our test suite to cover for that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think you concerns are valid. I did a find usages on the Transfrom#cameraPosition: 3 usages were found accessing this field directly and all where handling the null case. The more I think about it, the more I would prefer rewriting this part of the SDK..

@tobrun tobrun force-pushed the tvn-remove-markerview-api branch from 1795a22 to 951b9ed Compare October 26, 2018 08:31
if (cameraPosition == null) {
cameraPosition = invalidateCameraPosition();
if(cameraPosition == null){
throw new IllegalStateException();
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this should be the way to go. Since the initial place where we fetch the camera position from the core is on frame fully rendered, the cameraPosition is still null when we deliver the onMapReady, as the frame is typically not fully render yet. I can reproduce this by calling mapboxMap#getCameraPosition inside of the onMapReady.

We should either keep this guard, or invalidate the camera before delivering the onMapReady.

Copy link
Contributor

Choose a reason for hiding this comment

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

Capturing from the chat that this is only a test setup, to let the CI catch possible regressions.

@tobrun tobrun force-pushed the tvn-remove-markerview-api branch 2 times, most recently from f85403e to 7199d8e Compare October 30, 2018 14:50
@tobrun
Copy link
Member Author

tobrun commented Oct 31, 2018

This PR needs to be retargeted at master

@tobrun tobrun force-pushed the android-semver branch 2 times, most recently from 020966a to 30e3393 Compare October 31, 2018 12:16
@tobrun tobrun changed the base branch from android-semver to master November 1, 2018 21:17
@tobrun tobrun force-pushed the tvn-remove-markerview-api branch 2 times, most recently from a53414d to 9f69c30 Compare November 1, 2018 21:59
@tobrun
Copy link
Member Author

tobrun commented Nov 1, 2018

PR has been retargeted at master and is ready for review.

Copy link
Contributor

@LukasPaczos LukasPaczos left a comment

Choose a reason for hiding this comment

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

🚀

@tobrun tobrun force-pushed the tvn-remove-markerview-api branch 7 times, most recently from d77fc14 to 1b44c18 Compare November 7, 2018 17:44
@tobrun tobrun force-pushed the tvn-remove-markerview-api branch from 1b44c18 to cc9cb9d Compare November 13, 2018 10:55
@tobrun tobrun force-pushed the tvn-remove-markerview-api branch from cc9cb9d to 6be221d Compare November 14, 2018 10:02
@tobrun tobrun merged commit 1ceea72 into master Nov 14, 2018
@tobrun tobrun deleted the tvn-remove-markerview-api branch November 14, 2018 15:25
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.

2 participants