Use Android View instead of runtime styling for way name #1621

danesfeder · 2018-12-11T01:20:33Z

Closes #1222 Closes #1173

This PR replaces the MapWayName runtime styling implementation with an Android View .xml implementation. This aims to improve map performance as update the map way name layer with a new bitmap every time we have a new way name is expensive.

TODO:

Fix / add tests
Add javadoc
Before / after performance comparison

codecov-io · 2018-12-13T20:52:14Z

Codecov Report

Merging #1621 into master will decrease coverage by 0.19%.
The diff coverage is 25.3%.

@@             Coverage Diff             @@
##             master    #1621     +/-   ##
===========================================
- Coverage     23.84%   23.65%   -0.2%     
+ Complexity      735      714     -21     
===========================================
  Files           194      191      -3     
  Lines          8303     8214     -89     
  Branches        610      605      -5     
===========================================
- Hits           1980     1943     -37     
+ Misses         6137     6092     -45     
+ Partials        186      179      -7

danesfeder · 2018-12-13T23:14:19Z

@Guardiola31337 @devotaaabel this is ready for review - I'm working on test coverage.

Guardiola31337

This is looking great @danesfeder

While testing this, I've noticed that the pill shows up empty at the beginning 👀

I've also left some minor comments / questions.

Guardiola31337 · 2018-12-17T11:10:15Z

codecov.yml

+  - "**/LaneStyleKit.java"
+  - "**/LaneStyleKit.java"


This is duplicated.

Guardiola31337 · 2018-12-17T11:14:56Z

...gation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/NavigationContract.java

@@ -18,6 +19,8 @@

    void updateWaynameVisibility(boolean isVisible);

+    void updateWaynameView(@NonNull String wayName);


For consistency, should we call this method updateWayNameView instead of updateWaynameView? If so, we should also update ☝️ updateWaynameVisibility or is this to avoid SemVer?

Yeah it was mainly to avoid SemVer - worth it to break?

Guardiola31337 · 2018-12-17T11:50:23Z

...navigation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/map/MapWayName.java

-
-  String retrieveWayname() {
-    return wayname;
+  String retrieveWayName() {


Method 'retrieveWayName()' is never used

Guardiola31337 · 2018-12-17T12:39:22Z

...navigation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/map/MapWayName.java

-    if (waynameLayer != null) {
-      createWaynameIcon(wayname, waynameLayer);
-    }
+  void updateWayNameWith(String wayName) {


Method 'updateWayNameWith(java.lang.String)' is never used

Guardiola31337 · 2018-12-17T16:20:17Z

Thanks for addressing the feedback @danesfeder

Before / after performance comparison

I'm getting some numbers for ☝️ after that we can 🚀

Guardiola31337 · 2018-12-18T13:06:09Z

Working on #1621 (comment) I've noticed that we're not hiding the pill after calling NavigationView#stopNavigation. Should we? I believe so, right @danesfeder?

danesfeder · 2018-12-18T16:39:14Z

@Guardiola31337 yeah good call on #1621 (comment)

I updated the code to hide the "pill" if NavgationView#stopNavigation is called

Guardiola31337

This looks good to me 👍

Before / after performance comparison

I'm finishing up getting the numbers for the comparison but we shouldn't let it block the PR. I'll report back here when I finish so we can 👀 the performance 📈 from these changes.

Thanks for the great work here @danesfeder

devotaaabel

Looks good! Not sure why we didn't do this to begin with. I didn't test it but I trust @Guardiola31337 's testing skills

Guardiola31337 · 2019-01-16T12:45:31Z

Had the chance to analyze the numbers 🎉📈🎉

Results were obtained after running the same (automated) navigation session / route 30 times with and without OP's fix in a Nexus 5 👀

Power use

Average improvement 👉 2.096666667 mAh
Max improvement 👉 6.2 mAh
Min improvement 👉 1.5 mAh

CPU usage

Average improvement 👉 13.79310345%
Max improvement 👉 25%
Min improvement 👉 -1%

90% of the samples have improved

Note 👇 for understanding percentages greater than 100% shown in the graph

%CPU -- CPU Usage : The percentage of your CPU that is being used by the process. By default, top displays this as a percentage of a single CPU. On multi-core systems, you can have percentages that are greater than 100%. For example, if 3 cores are at 60% use, top will show a CPU use of 180%. See here for more information. You can toggle this behavior by hitting Shift + i while top is running to show the overall percentage of available CPUs in use.

Source for above quote.

cc @danesfeder @andrlee @akitchen

danesfeder added ⚠️ DO NOT MERGE PR should not be merged! refactor backwards incompatible Requires a SEMVER major version change. labels Dec 11, 2018

danesfeder added this to the v0.25.0 milestone Dec 11, 2018

danesfeder self-assigned this Dec 11, 2018

danesfeder requested review from Guardiola31337 and devotaaabel December 11, 2018 01:20

danesfeder force-pushed the dan-wayname-view branch 2 times, most recently from 78bd24e to 65e97a6 Compare December 13, 2018 20:52

danesfeder changed the title ~~[WIP] Use Android View instead of runtime styling for way name~~ Use Android View instead of runtime styling for way name Dec 13, 2018

danesfeder added ✓ ready for review and removed ⚠️ DO NOT MERGE PR should not be merged! labels Dec 13, 2018

danesfeder force-pushed the dan-wayname-view branch 2 times, most recently from 7aee6b0 to 725df7d Compare December 13, 2018 21:49

akitchen assigned Guardiola31337 Dec 14, 2018

Guardiola31337 reviewed Dec 17, 2018

View reviewed changes

danesfeder force-pushed the dan-wayname-view branch 3 times, most recently from 52fb41b to 177bb1c Compare December 17, 2018 15:15

danesfeder force-pushed the dan-wayname-view branch 2 times, most recently from 70ce0d9 to a92926a Compare December 18, 2018 16:38

danesfeder removed this from the v0.25.0 milestone Dec 18, 2018

danesfeder added this to the v0.26.0 milestone Dec 18, 2018

danesfeder force-pushed the dan-wayname-view branch from a92926a to bd2a590 Compare December 19, 2018 17:44

Use Android View instead of runtime styling for way name

01153cb

danesfeder force-pushed the dan-wayname-view branch from bd2a590 to 01153cb Compare December 19, 2018 20:23

Guardiola31337 approved these changes Dec 19, 2018

View reviewed changes

devotaaabel approved these changes Dec 19, 2018

View reviewed changes

danesfeder merged commit 012452f into master Dec 19, 2018

danesfeder deleted the dan-wayname-view branch December 19, 2018 23:23

Guardiola31337 mentioned this pull request Dec 20, 2018

Release v0.26.0 #1644

Closed

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Android View instead of runtime styling for way name #1621

Use Android View instead of runtime styling for way name #1621

danesfeder commented Dec 11, 2018 •

edited by Guardiola31337

codecov-io commented Dec 13, 2018 •

edited

danesfeder commented Dec 13, 2018

Guardiola31337 left a comment

Guardiola31337 Dec 17, 2018

Guardiola31337 Dec 17, 2018

danesfeder Dec 17, 2018

Guardiola31337 Dec 17, 2018

Guardiola31337 Dec 17, 2018

Guardiola31337 commented Dec 17, 2018

Guardiola31337 commented Dec 18, 2018

danesfeder commented Dec 18, 2018

Guardiola31337 left a comment

devotaaabel left a comment

Guardiola31337 commented Jan 16, 2019

		@@ -18,6 +19,8 @@

		void updateWaynameVisibility(boolean isVisible);

		void updateWaynameView(@NonNull String wayName);

Use Android View instead of runtime styling for way name #1621

Use Android View instead of runtime styling for way name #1621

Conversation

danesfeder commented Dec 11, 2018 • edited by Guardiola31337

codecov-io commented Dec 13, 2018 • edited

Codecov Report

danesfeder commented Dec 13, 2018

Guardiola31337 left a comment

Choose a reason for hiding this comment

Guardiola31337 Dec 17, 2018

Choose a reason for hiding this comment

Guardiola31337 Dec 17, 2018

Choose a reason for hiding this comment

danesfeder Dec 17, 2018

Choose a reason for hiding this comment

Guardiola31337 Dec 17, 2018

Choose a reason for hiding this comment

Guardiola31337 Dec 17, 2018

Choose a reason for hiding this comment

Guardiola31337 commented Dec 17, 2018

Guardiola31337 commented Dec 18, 2018

danesfeder commented Dec 18, 2018

Guardiola31337 left a comment

Choose a reason for hiding this comment

devotaaabel left a comment

Choose a reason for hiding this comment

Guardiola31337 commented Jan 16, 2019

Power use

CPU usage

danesfeder commented Dec 11, 2018 •

edited by Guardiola31337

codecov-io commented Dec 13, 2018 •

edited