[core] Introduce Distance class to store distance and units separately #5286

AndrewShkrob · 2023-06-07T17:32:22Z

AndrewShkrob commented

2023-06-07 17:32:22 +00:00

This PR solves 2 problems:

Broken translations in Android
Android Auto requires explicitly specifying remaining distance in double and units for it in Meters/Kilometers/Feet/Miles

Problem 2 can be solved by parsing "old" string values for distance and units as the translation is broken and we always have units in en: m/km/ft/mi. When translations fixed - Android Auto is broken.

I believe, this is a better solution.
What do you think? If this approach is OK, I will continue the implementation.

refactor measurment_utils
add Distance class to other places where the translation is broken
and so on

Potentionaly, it may be extended, f.e. with arithmetic operations to use it directly during route calculations.

Please, check UTs to better understand how it works now.

Before	After

This PR solves 2 problems: 1. Broken translations in Android 2. Android Auto requires explicitly specifying remaining distance in `double` and units for it in `Meters/Kilometers/Feet/Miles` Problem 2 can be solved by parsing "old" string values for distance and units as the translation is broken and we always have units in en: `m/km/ft/mi`. When translations fixed - Android Auto is broken. I believe, this is a better solution. What do you think? If this approach is OK, I will continue the implementation. * refactor `measurment_utils` * add `Distance` class to other places where the translation is broken * and so on Potentionaly, it may be extended, f.e. with arithmetic operations to use it directly during route calculations. Please, check UTs to better understand how it works now. | Before | After | |--------|--------| | <img width="300" src="https://github.com/organicmaps/organicmaps/assets/10351358/a15385cf-2981-455a-a44d-6ae28790c1e8"/> | <img width="300" src="https://github.com/organicmaps/organicmaps/assets/10351358/d8a94c2a-c1ae-4da4-9aad-85569d41ca48"/> | | <img width="300" src="https://github.com/organicmaps/organicmaps/assets/10351358/4541e0ed-15b5-46ba-8ff0-89e5d0b75e54"/> | <img width="300" src="https://github.com/organicmaps/organicmaps/assets/10351358/2c61feb5-48a3-481a-8aa9-b2480bec8163"/> | <img width="600" alt="image" src="https://github.com/organicmaps/organicmaps/assets/10351358/48bcba54-f2a7-45b5-a303-a082d553509e">

biodranik (Migrated from github.com) reviewed 2023-06-07 18:10:33 +00:00

biodranik (Migrated from github.com) left a comment

Thanks!

Is it the simplest approach possible? Looks like it requires a lot of boilerplate code.

Thanks! Is it the simplest approach possible? Looks like it requires a lot of boilerplate code.

platform/distance.cpp

biodranik (Migrated from github.com) commented

2023-06-07 18:06:52 +00:00

Is there an issue for it on Android? Did you see why is it broken?

biodranik (Migrated from github.com) commented

2023-06-07 18:09:18 +00:00

stod or better fast double converter from base library (we'll later replace it with even more faster converter).

platform/distance.cpp

					
				@ -0,0 +54,4 @@

				    return {measurement_utils::MilesToMeters(distance) / 1000, Distance::Units::Kilometers};

				  case Distance::Units::Feet:

				    return {measurement_utils::MilesToFeet(distance), Distance::Units::Feet};

				  default: UNREACHABLE();

biodranik (Migrated from github.com) commented

2023-06-07 18:09:32 +00:00

CHECK and crash here and in similar places?

platform/distance.hpp

biodranik (Migrated from github.com) commented

2023-06-07 18:05:04 +00:00

Are they used somewhere? To convert features data in generator?

platform/platform_tests/distance_tests.cpp

biodranik (Migrated from github.com) commented

2023-06-07 18:05:48 +00:00

No global using directives, please. Only in namespaces.

vng (Migrated from github.com) reviewed 2023-06-08 12:10:21 +00:00

android/jni/app/organicmaps/Framework.cpp

vng (Migrated from github.com) commented

2023-06-08 11:57:43 +00:00

Is it really necessary to pass string-like param here? Declare enums in cpp and java and write comments, that they should have equal values. We did it many times already.

android/src/app/organicmaps/routing/NavigationController.java

vng (Migrated from github.com) commented

2023-06-08 11:58:35 +00:00

Good, so probably better to replace 2 params in formatUnitsText with one Distance param here?

android/src/app/organicmaps/routing/RoutingBottomMenuController.java

vng (Migrated from github.com) commented

2023-06-08 11:59:53 +00:00

nit: Why do we use a fancy formatting delimiter here :)

iphone/Maps/Core/Framework/ProxyObjects/Routing/MWMRoutingManager.mm

					
				@ -94,11 +93,11 @@

				  }

vng (Migrated from github.com) commented

2023-06-08 12:03:13 +00:00

nit: Parameters order is mind blowing here :)

platform/distance.cpp

vng (Migrated from github.com) commented

2023-06-08 12:04:33 +00:00

nit: With the new line like in other cases is much better, no?

vng (Migrated from github.com) commented

2023-06-08 12:05:21 +00:00

Good here if precision matters.

vng (Migrated from github.com) commented

2023-06-08 12:06:51 +00:00

Also don't understand, Many other strings with GetLocalizedString work fine in Android.

platform/distance.hpp

vng (Migrated from github.com) commented

2023-06-08 12:07:42 +00:00

explicit is not needed

vng (Migrated from github.com) commented

2023-06-08 12:08:55 +00:00

Well, I think that nodiscard here and below is useless and makes only visual trash in code, IMO.

Imagine that we will mark like this all our functions now in the whole code ..

Well, I think that [[nodiscard]] here and below is useless and makes only visual trash in code, IMO. Imagine that we will mark like this all our functions now in the whole code ..

platform/distance.hpp

					
				@ -0,0 +43,4 @@

				  Distance ToPlatformUnitsFormatted() const;

				  double GetDistance() const;

				  Units GetUnits() const;

vng (Migrated from github.com) commented

2023-06-08 12:09:37 +00:00

BTW, following this logic, why this functions are not nodiscard ?

BTW, following this _logic_, why this functions are not [[nodiscard]] ?

vng (Migrated from github.com) reviewed 2023-06-08 12:23:48 +00:00

platform/platform_tests/distance_tests.cpp

vng (Migrated from github.com) commented

2023-06-08 12:23:48 +00:00

Is it good for you (if you are using formaters) put 120 symbols in a line?
It looks ugly, no?

Is it good for you (if you are using formaters) put 120 symbols in a line? It looks ugly, no?

vng (Migrated from github.com) reviewed 2023-06-08 12:27:59 +00:00

platform/distance.hpp

vng (Migrated from github.com) commented

2023-06-08 12:27:59 +00:00

Yes, do we really need NauticalMiles and Inches?

AndrewShkrob reviewed 2023-06-09 20:46:16 +00:00

platform/distance.cpp

AndrewShkrob commented

2023-06-09 20:46:15 +00:00

That's the first reason of this PR 😁
No, it's not working. Why? Idk.
Anyway, there is no sense to call this function from Android:

In cpp call Java to return string
Put it in std::string
Convert it to jstring
Pass to java

It's better to get string in java directly

That's the first reason of this PR 😁 No, it's not working. Why? Idk. Anyway, there is no sense to call this function from Android: * In cpp call Java to return string * Put it in std::string * Convert it to jstring * Pass to java It's better to get string in java directly

AndrewShkrob reviewed 2023-06-09 20:48:11 +00:00

platform/distance.cpp

					
				@ -0,0 +54,4 @@

				    return {measurement_utils::MilesToMeters(distance) / 1000, Distance::Units::Kilometers};

				  case Distance::Units::Feet:

				    return {measurement_utils::MilesToFeet(distance), Distance::Units::Feet};

				  default: UNREACHABLE();

AndrewShkrob commented

2023-06-09 20:48:11 +00:00

Yes. To let the developer know that such conversion is not implemented and he has to implement it by himself

AndrewShkrob reviewed 2023-06-09 20:53:54 +00:00

platform/distance.hpp

AndrewShkrob commented

2023-06-09 20:53:53 +00:00

nodiscard shows that it's a bad idea to call the function without storing the result (and using it later)

It was just an experiment and I also don't like how it looks

[[nodiscard]] shows that it's a bad idea to call the function without storing the result (and using it later) It was just an experiment and I also don't like how it looks

AndrewShkrob reviewed 2023-06-09 20:54:49 +00:00

platform/distance.hpp

					
				@ -0,0 +43,4 @@

				  Distance ToPlatformUnitsFormatted() const;

				  double GetDistance() const;

				  Units GetUnits() const;

AndrewShkrob commented

2023-06-09 20:54:48 +00:00

Because these functions just return values and don't perform any time-consuming operations.

AndrewShkrob reviewed 2023-06-09 21:59:11 +00:00

platform/distance.hpp

AndrewShkrob commented

2023-06-09 21:59:11 +00:00

I thought about using this class directly in generator (and other places) in future.
But let's leave it for the next iteration. I'll remove them now

I thought about using this class directly in generator (and other places) in future. But let's leave it for the next iteration. I'll remove them now

AndrewShkrob reviewed 2023-06-09 22:00:41 +00:00

platform/distance.hpp

AndrewShkrob commented

2023-06-09 22:00:41 +00:00

What do you prefer more?

return {1, Distance::Units::Meters};

or

return Distance(1, Distance::Units::Meters);

What do you prefer more? ```cpp return {1, Distance::Units::Meters}; ``` or ```cpp return Distance(1, Distance::Units::Meters); ```

AndrewShkrob reviewed 2023-06-09 22:01:21 +00:00

platform/platform_tests/distance_tests.cpp

AndrewShkrob commented

2023-06-09 22:01:20 +00:00

I'm using .clang-format in repo 😄

I'm using `.clang-format` in repo 😄

biodranik (Migrated from github.com) reviewed 2023-06-09 22:22:10 +00:00

platform/platform_tests/distance_tests.cpp

biodranik (Migrated from github.com) commented

2023-06-09 22:22:10 +00:00

It looks ugly.

vng (Migrated from github.com) reviewed 2023-06-09 22:57:09 +00:00

platform/distance.hpp

vng (Migrated from github.com) commented

2023-06-09 22:57:09 +00:00

I don't mind both variants.

AndrewShkrob reviewed 2023-06-10 09:36:47 +00:00

platform/distance.cpp

AndrewShkrob commented

2023-06-10 09:36:47 +00:00

Hm... That's weird but in some cases it works 🤔

vng (Migrated from github.com) approved these changes 2023-06-12 13:08:47 +00:00

android/src/app/organicmaps/util/Distance.java

vng (Migrated from github.com) commented

2023-06-12 12:57:49 +00:00

@NonNull is enough here, no?

```@NonNull``` is enough here, no?

qt/draw_widget.cpp

vng (Migrated from github.com) commented

2023-06-12 13:07:33 +00:00

nit: Can add friend function into Distance:

friend std::string DebugPrint(Distance const & d) { return d.ToString(); }

And can write here LOG(LDEBUG, (loc.m_distToTarget));

nit: Can add friend function into Distance: ``` friend std::string DebugPrint(Distance const & d) { return d.ToString(); } ``` And can write here LOG(LDEBUG, (loc.m_distToTarget));

biodranik (Migrated from github.com) approved these changes 2023-06-15 05:20:09 +00:00

android/jni/app/organicmaps/util/StringUtils.cpp

biodranik (Migrated from github.com) commented

2023-06-15 05:06:09 +00:00

nit:

Java_app_organicmaps_util_StringUtils_nativeFormatDistance(JNIEnv * env, jclass, jdouble distanceInMeters)

nit: ```suggestion Java_app_organicmaps_util_StringUtils_nativeFormatDistance(JNIEnv * env, jclass, jdouble distanceInMeters) ```

android/src/app/organicmaps/routing/RoutingController.java

biodranik (Migrated from github.com) commented

2023-06-15 05:08:21 +00:00

nit

    SpannableStringBuilder displayedH = Utils.formatTime(context, textSize, unitsSize, String.valueOf(hours), hour);
    SpannableStringBuilder displayedM = Utils.formatTime(context, textSize, unitsSize, String.valueOf(minutes), min);

nit ```suggestion SpannableStringBuilder displayedH = Utils.formatTime(context, textSize, unitsSize, String.valueOf(hours), hour); SpannableStringBuilder displayedM = Utils.formatTime(context, textSize, unitsSize, String.valueOf(minutes), min); ```

android/src/app/organicmaps/util/Distance.java

biodranik (Migrated from github.com) commented

2023-06-15 05:10:18 +00:00

What is this case? Should it be logged or should exception be thrown for faster debugging and fixing?

biodranik (Migrated from github.com) commented

2023-06-15 05:10:23 +00:00

What is this case? Should it be logged or should exception be thrown for faster debugging and fixing?

platform/distance.cpp

biodranik (Migrated from github.com) commented

2023-06-15 05:16:17 +00:00

Why is it not enabled for all platforms? If Linux UI will be rewritten for Navigation, it will need the same logic.

biodranik (Migrated from github.com) commented