[android] Show the speed limit like in iOS #3817

Closed
alensiljak wants to merge 2 commits from speed-limit-android into master
4 changed files with 48 additions and 7 deletions

View file

@ -1185,7 +1185,7 @@ Java_app_organicmaps_Framework_nativeGetRouteFollowingInfo(JNIEnv * env, jclass)
"(Ljava/lang/String;Ljava/lang/String;"
"Ljava/lang/String;Ljava/lang/String;"
"Ljava/lang/String;Ljava/lang/String;DIIIII"
"[Lapp/organicmaps/routing/SingleLaneInfo;ZZ)V");
"[Lapp/organicmaps/routing/SingleLaneInfo;ZZD)V");
vector<routing::FollowingInfo::SingleLaneInfoClient> const & lanes = info.m_lanes;
jobjectArray jLanes = nullptr;
@ -1221,7 +1221,8 @@ Java_app_organicmaps_Framework_nativeGetRouteFollowingInfo(JNIEnv * env, jclass)
jni::ToJavaString(env, info.m_turnUnitsSuffix), jni::ToJavaString(env, info.m_sourceName),
jni::ToJavaString(env, info.m_displayedStreetName), info.m_completionPercent, info.m_turn,
info.m_nextTurn, info.m_pedestrianTurn, info.m_exitNum, info.m_time, jLanes,
static_cast<jboolean>(isSpeedCamLimitExceeded), static_cast<jboolean>(shouldPlaySignal));
static_cast<jboolean>(isSpeedCamLimitExceeded), static_cast<jboolean>(shouldPlaySignal),
info.m_speedLimitMps);
ASSERT(result, (jni::DescribeException()));
return result;
}

View file

@ -13,7 +13,7 @@
biodranik commented 2022-11-07 09:24:02 +00:00 (Migrated from github.com)
Review

In all other files, the indent is 2 spaces

In all other files, the indent is 2 spaces
alensiljak commented 2023-01-22 13:34:56 +00:00 (Migrated from github.com)
Review

I believe this is resolved by the second attempt, applying only the changes without reformatting.

I believe this is resolved by the second attempt, applying only the changes without reformatting.
biodranik commented 2022-11-07 09:24:02 +00:00 (Migrated from github.com)
Review

In all other files, the indent is 2 spaces

In all other files, the indent is 2 spaces
alensiljak commented 2023-01-22 13:34:56 +00:00 (Migrated from github.com)
Review

I believe this is resolved by the second attempt, applying only the changes without reformatting.

I believe this is resolved by the second attempt, applying only the changes without reformatting.
android:id="@+id/speed_view_container"
android:layout_width="0dp"
android:layout_height="match_parent"
android:layout_weight="1"
biodranik commented 2022-11-07 09:24:02 +00:00 (Migrated from github.com)
Review

In all other files, the indent is 2 spaces

In all other files, the indent is 2 spaces
alensiljak commented 2023-01-22 13:34:56 +00:00 (Migrated from github.com)
Review

I believe this is resolved by the second attempt, applying only the changes without reformatting.

I believe this is resolved by the second attempt, applying only the changes without reformatting.
android:layout_weight="1.2"
biodranik commented 2022-11-07 09:24:02 +00:00 (Migrated from github.com)
Review

In all other files, the indent is 2 spaces

In all other files, the indent is 2 spaces
alensiljak commented 2023-01-22 13:34:56 +00:00 (Migrated from github.com)
Review

I believe this is resolved by the second attempt, applying only the changes without reformatting.

I believe this is resolved by the second attempt, applying only the changes without reformatting.
biodranik commented 2022-11-07 09:24:23 +00:00 (Migrated from github.com)
Review

These changes look unnecessary.

These changes look unnecessary.
alensiljak commented 2022-11-07 09:31:36 +00:00 (Migrated from github.com)
Review

I didn't make those changes. I was editing the file in the visual editor. For everything else you can blame Google and JetBrains. The formatting is applied using the project formatter for Android Studio you mentioned above.

I didn't make those changes. I was editing the file in the visual editor. For everything else you can blame Google and JetBrains. The formatting is applied using the project formatter for Android Studio you mentioned above.
biodranik commented 2022-11-07 09:56:51 +00:00 (Migrated from github.com)
Review

It doesn't matter who made these changes. Our repository doesn't need them. There are zero benefits in merging them. They pollute the project git history. Please undo them.

We generally don't apply automatic code formatters, but stick to the defined code style.

It doesn't matter who made these changes. Our repository doesn't need them. There are zero benefits in merging them. They pollute the project git history. Please undo them. We generally don't apply automatic code formatters, but stick to the defined code style.
alensiljak commented 2022-11-07 14:37:46 +00:00 (Migrated from github.com)
Review

I understand and agree with your comments. But, unfortunately, I really don't have time at the moment to fiddle around with XML that was modified by the default tool for the job, using default or project settings as advised.

I understand and agree with your comments. But, unfortunately, I really don't have time at the moment to fiddle around with XML that was modified by the default tool for the job, using default or project settings as advised.
Review

I started looking into showing speed limit too, try reset the XML, then remove android:paddingStart & android:paddingEndlines from the speed number element

I started looking into showing speed limit too, try reset the XML, then remove `android:paddingStart` & `android:paddingEnd`lines from the speed number element
android:background="@drawable/speed_cams_bg"
android:gravity="center"
android:minWidth="@dimen/nav_numbers_side_min_width"
@ -28,7 +28,7 @@
biodranik commented 2022-11-07 09:24:02 +00:00 (Migrated from github.com)
Review

In all other files, the indent is 2 spaces

In all other files, the indent is 2 spaces
alensiljak commented 2023-01-22 13:34:56 +00:00 (Migrated from github.com)
Review

I believe this is resolved by the second attempt, applying only the changes without reformatting.

I believe this is resolved by the second attempt, applying only the changes without reformatting.
biodranik commented 2022-11-07 09:24:02 +00:00 (Migrated from github.com)
Review

In all other files, the indent is 2 spaces

In all other files, the indent is 2 spaces
alensiljak commented 2023-01-22 13:34:56 +00:00 (Migrated from github.com)
Review

I believe this is resolved by the second attempt, applying only the changes without reformatting.

I believe this is resolved by the second attempt, applying only the changes without reformatting.
android:includeFontPadding="false"
android:lines="1"
android:textAppearance="@style/MwmTextAppearance.NavMenu.Number"
tools:text="999" />
biodranik commented 2022-11-07 09:24:02 +00:00 (Migrated from github.com)
Review

In all other files, the indent is 2 spaces

In all other files, the indent is 2 spaces
alensiljak commented 2023-01-22 13:34:56 +00:00 (Migrated from github.com)
Review

I believe this is resolved by the second attempt, applying only the changes without reformatting.

I believe this is resolved by the second attempt, applying only the changes without reformatting.
tools:text="999/999" />
biodranik commented 2022-11-07 09:24:02 +00:00 (Migrated from github.com)
Review

In all other files, the indent is 2 spaces

In all other files, the indent is 2 spaces
alensiljak commented 2023-01-22 13:34:56 +00:00 (Migrated from github.com)
Review

I believe this is resolved by the second attempt, applying only the changes without reformatting.

I believe this is resolved by the second attempt, applying only the changes without reformatting.
<!-- Speed -->
<TextView
@ -48,7 +48,7 @@
biodranik commented 2022-11-07 09:24:02 +00:00 (Migrated from github.com)
Review

In all other files, the indent is 2 spaces

In all other files, the indent is 2 spaces
alensiljak commented 2023-01-22 13:34:56 +00:00 (Migrated from github.com)
Review

I believe this is resolved by the second attempt, applying only the changes without reformatting.

I believe this is resolved by the second attempt, applying only the changes without reformatting.
biodranik commented 2022-11-07 09:24:02 +00:00 (Migrated from github.com)
Review

In all other files, the indent is 2 spaces

In all other files, the indent is 2 spaces
alensiljak commented 2023-01-22 13:34:56 +00:00 (Migrated from github.com)
Review

I believe this is resolved by the second attempt, applying only the changes without reformatting.

I believe this is resolved by the second attempt, applying only the changes without reformatting.
<RelativeLayout
android:layout_width="0dp"
android:layout_height="match_parent"
biodranik commented 2022-11-07 09:24:54 +00:00 (Migrated from github.com)
Review

Please don't commit unnecessary changes.

Please don't commit unnecessary changes.
android:layout_weight="1.5"
biodranik commented 2022-11-07 09:24:02 +00:00 (Migrated from github.com)
Review

In all other files, the indent is 2 spaces

In all other files, the indent is 2 spaces
alensiljak commented 2023-01-22 13:34:56 +00:00 (Migrated from github.com)
Review

I believe this is resolved by the second attempt, applying only the changes without reformatting.

I believe this is resolved by the second attempt, applying only the changes without reformatting.
android:layout_weight="1.2"
biodranik commented 2022-11-07 09:24:02 +00:00 (Migrated from github.com)
Review

In all other files, the indent is 2 spaces

In all other files, the indent is 2 spaces
alensiljak commented 2023-01-22 13:34:56 +00:00 (Migrated from github.com)
Review

I believe this is resolved by the second attempt, applying only the changes without reformatting.

I believe this is resolved by the second attempt, applying only the changes without reformatting.
android:gravity="center"
android:minWidth="@dimen/nav_numbers_side_min_width"
android:paddingStart="@dimen/nav_numbers_margin"

biodranik commented 2022-11-07 09:24:02 +00:00 (Migrated from github.com)
Review

In all other files, the indent is 2 spaces

In all other files, the indent is 2 spaces
alensiljak commented 2023-01-22 13:34:56 +00:00 (Migrated from github.com)
Review

I believe this is resolved by the second attempt, applying only the changes without reformatting.

I believe this is resolved by the second attempt, applying only the changes without reformatting.
biodranik commented 2022-11-07 09:24:02 +00:00 (Migrated from github.com)
Review

In all other files, the indent is 2 spaces

In all other files, the indent is 2 spaces
alensiljak commented 2023-01-22 13:34:56 +00:00 (Migrated from github.com)
Review

I believe this is resolved by the second attempt, applying only the changes without reformatting.

I believe this is resolved by the second attempt, applying only the changes without reformatting.

View file

@ -27,6 +27,7 @@ public class RoutingInfo
public final CarDirection nextCarDirection;
public final int exitNum;
public final SingleLaneInfo[] lanes;
public final double speedLimit;
// For pedestrian routing.
public final PedestrianTurnDirection pedestrianTurnDirection;
private final boolean speedLimitExceeded;
@ -157,7 +158,7 @@ public class RoutingInfo
public RoutingInfo(String distToTarget, String units, String distTurn, String turnSuffix, String currentStreet, String nextStreet, double completionPercent,
int vehicleTurnOrdinal, int vehicleNextTurnOrdinal, int pedestrianTurnOrdinal, int exitNum,
int totalTime, SingleLaneInfo[] lanes, boolean speedLimitExceeded,
boolean shouldPlayWarningSignal)
boolean shouldPlayWarningSignal, double speedLimit)
{
this.distToTarget = distToTarget;
this.targetUnits = units;
@ -174,6 +175,7 @@ public class RoutingInfo
this.pedestrianTurnDirection = PedestrianTurnDirection.values()[pedestrianTurnOrdinal];
this.speedLimitExceeded = speedLimitExceeded;
this.shouldPlayWarningSignal = shouldPlayWarningSignal;
this.speedLimit = speedLimit;
}
public boolean isSpeedLimitExceeded()

View file

@ -208,11 +208,49 @@ public class NavMenu
Pair<String, String> speedAndUnits = StringUtils.nativeFormatSpeedAndUnits(last.getSpeed());
pm4rcin commented 2022-11-07 09:32:04 +00:00 (Migrated from github.com)
Review

A bit off-topic but I'm wondering why do people often use 18/5 when converting to kph when it's just 3.6? Is it easier to remember or what?

A bit off-topic but I'm wondering why do people often use 18/5 when converting to kph when it's just 3.6? Is it easier to remember or what?
alensiljak commented 2022-11-07 09:34:31 +00:00 (Migrated from github.com)
Review

It is easier to copy/paste from a search result for "m/s to mph" ;)

It is easier to copy/paste from a search result for "m/s to mph" ;)
biodranik commented 2022-11-07 09:55:06 +00:00 (Migrated from github.com)
Review

3600 / 1000 is way better and more obvious than 18/5

`3600 / 1000` is way better and more obvious than 18/5
pm4rcin commented 2022-11-07 10:10:22 +00:00 (Migrated from github.com)
Review

@alensiljak also if you change it in one place change in every other to keep it consistent.

@alensiljak also if you change it in one place change in every other to keep it consistent.
alensiljak commented 2022-11-07 10:12:15 +00:00 (Migrated from github.com)
Review

Seems that you guys have more time to work on this than I do. :D

Seems that you guys have more time to work on this than I do. :D
// (temporarily) show the speed limit together with the current speed.
String speedIndicator = formatSpeed(speedAndUnits, info);
mSpeedValue.setText(speedIndicator);
mSpeedUnits.setText(speedAndUnits.second);
mSpeedValue.setText(speedAndUnits.first);
mSpeedViewContainer.setActivated(info.isSpeedLimitExceeded());
}
private String formatSpeed(Pair<String, String> speedAndUnits, RoutingInfo info)
{
double currentSpeedMps = Double.parseDouble(speedAndUnits.first);
String speedIndicator = String.format("%.0f", currentSpeedMps);
if (info.speedLimit > 0.0)
{
double speedLimit = convertMpsIntoUnits(info.speedLimit, speedAndUnits.second);
speedIndicator += String.format("/%.0f", speedLimit);
}
jordane-quincy commented 2023-01-22 11:25:32 +00:00 (Migrated from github.com)
Review

[Pure suggestion as I would love to see this PR merged even without it and I wouldn't slow down the reviewing process]

iOS pr use space around the slash:

      speedIndicator += String.format(" / %.0f", speedLimit);
[Pure suggestion as I would love to see this PR merged even without it and I wouldn't slow down the reviewing process] iOS [pr](https://git.omaps.dev/organicmaps/organicmaps/pulls/2963/files#diff-689d1838cea59440095256c64d22da973383adbb1143f45a1c545d32621b1de4R160) use space around the slash: ```suggestion speedIndicator += String.format(" / %.0f", speedLimit); ```
alensiljak commented 2023-01-22 12:07:51 +00:00 (Migrated from github.com)
Review

iOS use space around the slash:

The initial implementation was like that. Unfortunately, it takes too much screen space, in my opinion. Hence I narrowed it down.

> iOS use space around the slash: The initial implementation was like that. Unfortunately, it takes too much screen space, in my opinion. Hence I narrowed it down.
jordane-quincy commented 2023-01-22 13:15:29 +00:00 (Migrated from github.com)
Review

Oh ! ok. Sorry I didn't see this from the history because of the force push.

Is there anything I can do to help the merge of this pr ?
Speed limit information is one of key features I really love to see in Organic Maps

Oh ! ok. Sorry I didn't see this from the history because of the force push. Is there anything I can do to help the merge of this pr ? Speed limit information is one of key features I really love to see in Organic Maps
return speedIndicator;
}
private double convertMpsIntoUnits(double speedInMps, String units)
{
double result;
switch (units)
{
case "km/h":
// 1 m/s = 3600s/1000m = 3.6 km/h
result = speedInMps * 3.6;
break;
case "mph":
// 1 m/s = 2.2369 mph
result = speedInMps * 2.2369;
break;
default:
// should throw an exception. Use km/h as a workaround.
result = speedInMps * 3.6;
}
return result;
}
vng commented 2022-11-08 19:43:17 +00:00 (Migrated from github.com)
Review

Why not to use existing approach? Like:
Pair<String, String> speedAndUnits = StringUtils.nativeFormatSpeedAndUnits(last.getSpeed());

Why not to use _existing_ approach? Like: Pair<String, String> speedAndUnits = StringUtils.nativeFormatSpeedAndUnits(last.getSpeed());
alensiljak commented 2022-11-23 10:52:50 +00:00 (Migrated from github.com)
Review

In this case, the unit is already known to the caller and that would somewhat obfuscate the point of the function, I think.

In this case, the unit is already known to the caller and that would somewhat obfuscate the point of the function, I think.
biodranik commented 2023-01-27 22:25:09 +00:00 (Migrated from github.com)
Review

Is this duplication really necessary?

Is this duplication really necessary?
public void update(@NonNull RoutingInfo info)
{
updateSpeedView(info);