Skip to content

Conversation

@abhishek1508
Copy link
Contributor

@abhishek1508 abhishek1508 commented Oct 11, 2022

Description

The implementation covers

  • removing @ExperimentalPreviewMapboxNavigationAPI annotation from all internal and public files related to libnavui-dropin module.
  • removing @ExperimentalPreviewMapboxNavigationAPI from all new components that are exposed outside libnavui-dropin module. For ex: RouteLineComponent, RouteLineComponentContract, RecenterButtonComponent etc. It shouldn't be an issue removing the experimental annotation, since these components and their respective contracts are hosted inside internal package.
  • adding@ExperimentalPreviewMapboxNavigationAPI to MapboxExtendableButton.updateStyles and it's usage across the SDK.
  • not removing @ExperimentalPreviewMapboxNavigationAPI from any files related to ComponentInstaller and it's usage. ComponentInstaller could be subject to modifications and we need to play more with them before declaring them stable.

@abhishek1508 abhishek1508 requested a review from a team as a code owner October 11, 2022 16:20
@codecov
Copy link

codecov bot commented Oct 11, 2022

Codecov Report

Merging #6471 (c8ca9a4) into main (a45f7d4) will increase coverage by 0.07%.
The diff coverage is n/a.

❗ Current head c8ca9a4 differs from pull request most recent head 95f2c30. Consider uploading reports for the commit 95f2c30 to get more accurate results

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #6471      +/-   ##
============================================
+ Coverage     70.26%   70.33%   +0.07%     
+ Complexity     4918     4907      -11     
============================================
  Files           722      717       -5     
  Lines         28044    27947      -97     
  Branches       3305     3295      -10     
============================================
- Hits          19705    19657      -48     
+ Misses         7064     7012      -52     
- Partials       1275     1278       +3     
Impacted Files Coverage Δ
...mapbox/navigation/navigator/internal/TripStatus.kt 0.00% <0.00%> (-100.00%) ⬇️
...com/mapbox/navigation/core/history/HistoryFiles.kt 60.00% <0.00%> (-8.43%) ⬇️
...x/navigation/core/history/MapboxHistoryRecorder.kt 42.85% <0.00%> (-4.97%) ⬇️
...x/navigation/ui/maps/building/BuildingProcessor.kt 65.62% <0.00%> (-2.38%) ⬇️
...ava/com/mapbox/navigation/core/MapboxNavigation.kt 67.85% <0.00%> (-1.81%) ⬇️
...box/navigation/core/NavigationComponentProvider.kt 51.35% <0.00%> (-1.43%) ⬇️
...box/navigation/ui/voice/api/MapboxAudioGuidance.kt 79.68% <0.00%> (-0.32%) ⬇️
...om/mapbox/navigation/base/route/NavigationRoute.kt 51.92% <0.00%> (-0.24%) ⬇️
.../mapbox/navigation/route/internal/RouterWrapper.kt 80.35% <0.00%> (-0.12%) ⬇️
...n/dropin/navigationview/MapboxNavigationViewApi.kt 92.59% <0.00%> (-0.10%) ⬇️
... and 19 more

@abhishek1508 abhishek1508 added the ⚠️ DO NOT MERGE PR should not be merged! label Oct 11, 2022
@abhishek1508 abhishek1508 changed the title [Drop-In] Remove @Experimantal annotation from drop in related files [Drop-In] Remove @Experimental annotation from drop in related files Oct 11, 2022
@abhishek1508 abhishek1508 force-pushed the ak-remove-experimental-annotation branch 3 times, most recently from a7b89a8 to 553a038 Compare October 11, 2022 22:51
@abhishek1508 abhishek1508 self-assigned this Oct 13, 2022
@abhishek1508 abhishek1508 force-pushed the ak-remove-experimental-annotation branch 4 times, most recently from 45f3e9a to 31daf52 Compare October 20, 2022 16:28
@abhishek1508 abhishek1508 force-pushed the ak-remove-experimental-annotation branch 7 times, most recently from d1aa26f to f830121 Compare November 3, 2022 19:30
@abhishek1508 abhishek1508 force-pushed the ak-remove-experimental-annotation branch 3 times, most recently from 9393692 to a17b96f Compare November 7, 2022 20:24

/**
* Factory class for creating all drop-in UI map point annotations.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

@abhishek1508 Why did you remove this comment?

Copy link
Contributor Author

@abhishek1508 abhishek1508 Nov 9, 2022

Choose a reason for hiding this comment

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

In the beginning all our classes were public and hence the need for documentation for all. Since we changed the relevant ones to internal, we did not remove the comments. Our code base has now become quite inconsistent as to some internal classes have descriptions vs the others don't. This was an attempt to bring consistency.

However I have put them back for the time being.

@tomaszrybakiewicz
Copy link
Contributor

@abhishek1508 The changes that remove @Experimental annotation are great.
However, I have a couple of questions about some other changes:

  • Why are we moving and modifying ComponentInstaller classes? I think these changes are outside the scope of this PR and should be done separately.
  • Why are we removing class description comments?

@abhishek1508
Copy link
Contributor Author

@abhishek1508 The changes that remove @Experimental annotation are great. However, I have a couple of questions about some other changes:

  • Why are we moving and modifying ComponentInstaller classes? I think these changes are outside the scope of this PR and should be done separately.

Answered here

  • Why are we removing class description comments?

Answered here

@tomaszrybakiewicz

@abhishek1508 abhishek1508 force-pushed the ak-remove-experimental-annotation branch 4 times, most recently from 1750dea to 442c08f Compare November 9, 2022 20:19
@abhishek1508
Copy link
Contributor Author

@tomaszrybakiewicz All suggestions have been applied. PTAL and let me know if any other changes are needed.

@abhishek1508 abhishek1508 force-pushed the ak-remove-experimental-annotation branch 2 times, most recently from 8c48ef4 to 731fa05 Compare November 9, 2022 23:53
@abhishek1508 abhishek1508 force-pushed the ak-remove-experimental-annotation branch from 731fa05 to a250af4 Compare November 10, 2022 12:08
Copy link
Contributor

@tomaszrybakiewicz tomaszrybakiewicz left a comment

Choose a reason for hiding this comment

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

LGTM

* private fun onDetached(mapboxNavigation: MapboxNavigation)
* ```
*/
fun forwardMapboxNavigation(
Copy link
Contributor

Choose a reason for hiding this comment

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

It's weird that we now have mapboxNavigationForward and forwardMapboxNavigation with identical implementations. cc @kmadsen

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I had some direct messages with abhishek1508 about this. I really like the mapboxNavigationForward function in android auto. It is a little similar to requireMapboxNavigation

So the plan will be to delete the mapboxNavigationForward function in androidauto, in favor of forwardMapboxNavigation in the core-sdk. Sorry it's confusing to have them renamed, consequence of iteration.

But this is all intentional

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

⚠️ DO NOT MERGE PR should not be merged!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants