-
Notifications
You must be signed in to change notification settings - Fork 319
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
Use Android View instead of runtime styling for way name #1621
Conversation
78bd24e
to
65e97a6
Compare
Codecov Report
@@ 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 |
7aee6b0
to
725df7d
Compare
@Guardiola31337 @devotaaabel this is ready for review - I'm working on test coverage. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
codecov.yml
Outdated
- "**/LaneStyleKit.java" | ||
- "**/LaneStyleKit.java" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is duplicated.
@@ -18,6 +19,8 @@ | |||
|
|||
void updateWaynameVisibility(boolean isVisible); | |||
|
|||
void updateWaynameView(@NonNull String wayName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency, should we call this method updateWayNameView
instead of updateWaynameView
? If so, we should also update ☝️ updateWaynameVisibility
or is this to avoid SemVer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it was mainly to avoid SemVer - worth it to break?
|
||
String retrieveWayname() { | ||
return wayname; | ||
String retrieveWayName() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Method 'retrieveWayName()' is never used
if (waynameLayer != null) { | ||
createWaynameIcon(wayname, waynameLayer); | ||
} | ||
void updateWayNameWith(String wayName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Method 'updateWayNameWith(java.lang.String)' is never used
52fb41b
to
177bb1c
Compare
Thanks for addressing the feedback @danesfeder
I'm getting some numbers for ☝️ after that we can 🚀 |
Working on #1621 (comment) I've noticed that we're not hiding the pill after calling |
70ce0d9
to
a92926a
Compare
@Guardiola31337 yeah good call on #1621 (comment) I updated the code to hide the "pill" if |
a92926a
to
bd2a590
Compare
bd2a590
to
01153cb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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 useAverage improvement 👉 2.096666667 mAh CPU usageAverage improvement 👉 13.79310345% 90% of the samples have improved Note 👇 for understanding percentages greater than 100% shown in the graph
|
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: