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

Support EV route refresh #6511

Merged
merged 6 commits into from
Oct 28, 2022
Merged

Support EV route refresh #6511

merged 6 commits into from
Oct 28, 2022

Conversation

dzinad
Copy link
Contributor

@dzinad dzinad commented Oct 26, 2022

Description

The PR has the following parts (split into the corresponding commits):

  1. Get EV data updates from client and pass it as route refresh request parameters;
  2. Update state_of_charge annotations after route refresh (provided via unrecognized properties);
  3. Update waypoints after route refresh.

updatedRoutes[0].getSocAnnotationsFromLeg(1)!!.firstLastAnd(legGeometryIndex)
)
assertEquals(
listOf(null, 8097, null),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This check fails for now because I've used the real Directions API response to form our test response but it returned less waypoints than I expected (and than as described in their docs). I've asked Mandeep a corresponding question.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The fixed a bug, I've updated the mocked response appropriately. Should be working now.

@dzinad dzinad force-pushed the NAVAND-713-dd-response branch from 7eab1b6 to c0be5ad Compare October 26, 2022 11:58
@dzinad dzinad added the ⚠️ DO NOT MERGE PR should not be merged! label Oct 26, 2022
@dzinad
Copy link
Contributor Author

dzinad commented Oct 26, 2022

I've added a DO NOT MERGE label because some changes may be required after #6005 is merged.

@dzinad dzinad marked this pull request as ready for review October 26, 2022 11:59
@dzinad dzinad requested a review from a team as a code owner October 26, 2022 11:59
@dzinad dzinad force-pushed the NAVAND-713-dd-response branch from c0be5ad to 93fb66d Compare October 26, 2022 14:44
@codecov
Copy link

codecov bot commented Oct 26, 2022

Codecov Report

Merging #6511 (8af0a4c) into main (a45f7d4) will increase coverage by 0.07%.
The diff coverage is 85.45%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #6511      +/-   ##
============================================
+ Coverage     70.26%   70.33%   +0.07%     
- Complexity     4918     4941      +23     
============================================
  Files           722      724       +2     
  Lines         28044    28148     +104     
  Branches       3305     3320      +15     
============================================
+ Hits          19705    19798      +93     
- Misses         7064     7074      +10     
- Partials       1275     1276       +1     
Impacted Files Coverage Δ
...avigation/base/internal/RouteRefreshRequestData.kt 0.00% <0.00%> (ø)
...mapbox/navigation/base/internal/utils/Constants.kt 0.00% <0.00%> (ø)
...box/navigation/core/NavigationComponentProvider.kt 52.77% <0.00%> (ø)
...ore/routerefresh/RouteRefreshControllerProvider.kt 0.00% <0.00%> (ø)
...on/navigator/internal/MapboxNativeNavigatorImpl.kt 0.00% <0.00%> (ø)
...n/java/com/mapbox/navigation/core/SetRoutesInfo.kt 76.08% <80.00%> (ø)
.../mapbox/navigation/route/internal/RouterWrapper.kt 81.11% <88.23%> (+0.63%) ⬆️
...gation/base/internal/route/AnnotationsRefresher.kt 98.80% <96.42%> (+0.56%) ⬆️
...navigation/core/RouteRefreshRequestDataProvider.kt 96.55% <96.55%> (ø)
...avigation/base/internal/route/NavigationRouteEx.kt 84.37% <100.00%> (+3.60%) ⬆️
... and 8 more

@dzinad dzinad force-pushed the NAVAND-713-dd-response branch 2 times, most recently from a8c2e1a to 4709124 Compare October 27, 2022 09:53
Comment on lines 3 to 8
public final class Constants {

public static final class RouteResponse {
public static final String KEY_WAYPOINTS = "waypoints";
}
}

Choose a reason for hiding this comment

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

Why Java? object can be nested as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because in kotlin this code:

object ConstantsKt {
    object RouteResponse {
        const val KEY_WAYPOINTS = "waypoints"
    }
}

looks like this:

public final class ConstantsKt {
   @NotNull
   public static final ConstantsKt INSTANCE;

   private ConstantsKt() {
   }

   static {
      ConstantsKt var0 = new ConstantsKt();
      INSTANCE = var0;
   }

   @Metadata(
      mv = {1, 5, 1},
      k = 1,
      d1 = {"\u0000\u0012\n\u0002\u0018\u0002\n\u0002\u0010\u0000\n\u0002\b\u0002\n\u0002\u0010\u000e\n\u0000\bÆ\u0002\u0018\u00002\u00020\u0001B\u0007\b\u0002¢\u0006\u0002\u0010\u0002R\u000e\u0010\u0003\u001a\u00020\u0004X\u0086T¢\u0006\u0002\n\u0000¨\u0006\u0005"},
      d2 = {"Lcom/mapbox/navigation/base/internal/utils/ConstantsKt$RouteResponse;", "", "()V", "KEY_WAYPOINTS", "", "mapbox-navigation-android.libnavigation-base.main"}
   )
   public static final class RouteResponse {
      @NotNull
      public static final String KEY_WAYPOINTS = "waypoints";
      @NotNull
      public static final ConstantsKt.RouteResponse INSTANCE;

      private RouteResponse() {
      }

      static {
         ConstantsKt.RouteResponse var0 = new ConstantsKt.RouteResponse();
         INSTANCE = var0;
      }
   }
}

Isn't it too much for a string constant?
I can add the JvmStatic annotation, but it's not much better.
Do you have anything against java? I can easily convert it to kotlin but I think that for the purposes like this (when you just need a holder for some constants) java is better (probably the only situation where I'd say this).

Copy link
Contributor

Choose a reason for hiding this comment

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

I've wanted to try AutoValue classes written in java instead of writing custom equals, hashcode, toString for a long time but I'm too lazy to explore how java files affect compilation time in our project. Did you check if there are any possible issues we could have compiling java + kotlin in one project?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did you check if there are any possible issues we could have compiling java + kotlin in one project?

When you add kotlin to a java project compilation time increases. In the other direction it should be fine. In my experience 2 languages work fine.
Right now I don't see a regression. I've run the compilation several times and the time is between 1m 40s and 1m 50s with no obvious leader (sometimes one option is faster, sometimes the other).
But if you are worried there may be some issues, I can convert to kotlin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Converted to kotlin.

Choose a reason for hiding this comment

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

Thanks, that's an interesting point of view. I guess it's a general problem with objects that they are just singletons under the hood after compilation. Either way would work, I was just curious what was the motivation.

@@ -1665,6 +1665,38 @@ class MapboxNavigation @VisibleForTesting internal constructor(
developerMetadataAggregator.unregisterObserver(developerMetadataObserver)
}

/**
* Invoke when any component of EV data is changed so that it can be used in refresh requests.
* Pass **only changed** components of EV data via [data].

Choose a reason for hiding this comment

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

Suggested change
* Pass **only changed** components of EV data via [data].
*

As below, providing all the data at all times is not a problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not a problem but I want to specify that it's not necessary to implement the caching on the app side. Changed the docs.

@LukasPaczos
Copy link

Some minor comments above and let's wait for #6005 (if we need to) but in general LGTM!

@dzinad dzinad force-pushed the NAVAND-713-dd-response branch from 4709124 to 26f6ad3 Compare October 27, 2022 11:44
@dzinad
Copy link
Contributor Author

dzinad commented Oct 27, 2022

Some minor comments above and let's wait for #6005 (if we need to)

Whoever merges first will leave the second person supporting these changes. :)

@dzinad dzinad force-pushed the NAVAND-713-dd-response branch 3 times, most recently from 85b3180 to 208b7fb Compare October 28, 2022 10:51
@dzinad dzinad requested a review from LukasPaczos October 28, 2022 10:51
@dzinad dzinad force-pushed the NAVAND-713-dd-response branch 3 times, most recently from 6a84243 to 6e96401 Compare October 28, 2022 14:13
@RingerJK RingerJK force-pushed the NAVAND-713-dd-response branch from 6e96401 to fdd23c3 Compare October 28, 2022 16:10
@RingerJK RingerJK removed the ⚠️ DO NOT MERGE PR should not be merged! label Oct 28, 2022
@RingerJK
Copy link
Contributor

#6005 has landed

@LukasPaczos LukasPaczos enabled auto-merge (squash) October 28, 2022 17:19
@LukasPaczos LukasPaczos merged commit e893c20 into main Oct 28, 2022
@LukasPaczos LukasPaczos deleted the NAVAND-713-dd-response branch October 28, 2022 18:45
@Mundis2201
Copy link

CHANGELOG.md

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

Successfully merging this pull request may close these issues.

None yet

5 participants