[ios][android] Replacing altitude difference to ascent and descent #3062

2022-07-31T09:02:29Z

ArsenyN commented

2022-07-31 09:02:29 +00:00

(Migrated from github.com)

Summary

The app for pedestrian and cyclists route show altitude difference
between highest and lowest points. It's not too useful data in multiple
cases. More informative for hikers and cyclists from values of
elevation total ascent and total descent on route.

I replaced altitude difference to total ascent and total descent.

Current state

Example of route elevation profile where altitude difference between highest and lowest points is useless.

Result

iOS

Android

### Summary The app for pedestrian and cyclists route show altitude difference between highest and lowest points. It's not too useful data in multiple cases. More informative for hikers and cyclists from values of elevation total ascent and total descent on route. I replaced altitude difference to total ascent and total descent. **Current state** ![ios-current300](https://user-images.githubusercontent.com/235630/182018536-d231e846-abaa-4d3b-99c3-37fdfb11e4d7.png) **Example of route elevation profile where altitude difference between highest and lowest points is useless.** ![altitude-difference650](https://user-images.githubusercontent.com/235630/182018672-a620c1f7-0dd7-45cb-a150-7b97e0f9bad8.png) **Result** **iOS** ![ios-result300](https://user-images.githubusercontent.com/235630/182018881-1e531f51-d595-4a01-92a0-91ac9f49b311.png) **Android** ![android-result300](https://user-images.githubusercontent.com/235630/182018886-8d0da837-4188-4eed-b9d9-e2c74af9933e.png)

👍 8

biodranik (Migrated from github.com) requested changes 2022-07-31 11:57:04 +00:00

biodranik (Migrated from github.com) left a comment

Thank you for your contribution! There are a few minor comments.

android/src/com/mapswithme/maps/routing/RoutingBottomMenuController.java

					
				@ -264,17 +264,10 @@ final class RoutingBottomMenuController implements View.OnClickListener

				    {

				      mAltitudeChart.setImageBitmap(bm);

biodranik (Migrated from github.com) commented

2022-07-31 11:46:35 +00:00

Looks like there are tabs and spaces mixed in the indentation. Please set the editor to use 2 spaces instead of tabs, and check/fix indentation in all your changes.

biodranik (Migrated from github.com) commented

2022-07-31 11:48:06 +00:00

This call is unnecessary in the metric system, and the previous call is useless in the imperial one. Please rewrite the code without unnecessary function calls. We want to have the fastest app that eats less battery 😉

This call is unnecessary in the metric system, and the previous call is useless in the imperial one. Please rewrite the code without unnecessary function calls. We want to have _the fastest_ app that eats _less_ battery 😉

iphone/Maps/Classes/CustomViews/NavigationDashboard/Views/RoutePreview/RoutePreviewStatus/BaseRoutePreviewStatus.swift

biodranik (Migrated from github.com) commented

2022-07-31 11:51:33 +00:00

Tabs and spaces are also mixed. Please clean up.
Is guard really needed here? Why not a simple if?

1. Tabs and spaces are also mixed. Please clean up. 2. Is guard really needed here? Why not a simple `if`?

iphone/Maps/Core/Routing/MWMRouter.mm

					
				@ -386,15 +387,16 @@ char const *kRenderAltitudeImagesQueueLabel = "mapsme.mwmrouter.renderAltitudeIm

				      router.altitudeImagesData[sizeValue] = imageData;

biodranik (Migrated from github.com) commented

2022-07-31 11:52:58 +00:00

Are feet (imperial system) work on iOS in this case?

iphone/Maps/Core/Routing/MWMRouter.mm

biodranik (Migrated from github.com) commented

2022-07-31 11:53:55 +00:00

tabs + spaces 😢

map/routing_manager.cpp

biodranik (Migrated from github.com) commented

2022-07-31 11:54:32 +00:00

If totalAscent/Descent can't be negative, please use size_t for it.

map/routing_manager.cpp

biodranik (Migrated from github.com) commented

2022-07-31 11:55:59 +00:00

    if (delta > 0)
      totalAscentM += delta;
    else
      totalDescentM += -delta;

```suggestion if (delta > 0) totalAscentM += delta; else totalDescentM += -delta; ```

biodranik (Migrated from github.com) commented

2022-07-31 11:56:15 +00:00

There is a code style guide in the repo :)

ArsenyN commented

2022-07-31 12:01:38 +00:00

(Migrated from github.com)

@biodranik Thanks for review and you comments.
Will check and fix all soon

@biodranik Thanks for review and you comments. Will check and fix all soon

ArsenyN (Migrated from github.com) reviewed 2022-08-01 05:48:50 +00:00

android/src/com/mapswithme/maps/routing/RoutingBottomMenuController.java

					
				@ -264,17 +264,10 @@ final class RoutingBottomMenuController implements View.OnClickListener

				    {

				      mAltitudeChart.setImageBitmap(bm);

ArsenyN (Migrated from github.com) commented

2022-08-01 05:48:50 +00:00

Fixed at d80c3a0

ArsenyN (Migrated from github.com) reviewed 2022-08-01 05:51:11 +00:00

android/src/com/mapswithme/maps/routing/RoutingBottomMenuController.java

					
				@ -264,17 +264,10 @@ final class RoutingBottomMenuController implements View.OnClickListener

				    {

				      mAltitudeChart.setImageBitmap(bm);

ArsenyN (Migrated from github.com) commented

2022-08-01 05:51:10 +00:00

Removed unnecessary function calls d80c3a0

ArsenyN (Migrated from github.com) reviewed 2022-08-01 05:51:56 +00:00

iphone/Maps/Classes/CustomViews/NavigationDashboard/Views/RoutePreview/RoutePreviewStatus/BaseRoutePreviewStatus.swift

ArsenyN (Migrated from github.com) commented

2022-08-01 05:51:55 +00:00

Fixed idents
Replaced 2 guard by if
d80c3a0

- Fixed idents - Replaced 2 guard by if d80c3a0

ArsenyN (Migrated from github.com) reviewed 2022-08-01 05:52:41 +00:00

iphone/Maps/Core/Routing/MWMRouter.mm

					
				@ -386,15 +387,16 @@ char const *kRenderAltitudeImagesQueueLabel = "mapsme.mwmrouter.renderAltitudeIm

				      router.altitudeImagesData[sizeValue] = imageData;

ArsenyN (Migrated from github.com) commented

2022-08-01 05:52:40 +00:00

Checked on iOS and Android there are similar code. All works with both measurement systems.

ArsenyN (Migrated from github.com) reviewed 2022-08-01 05:53:10 +00:00

iphone/Maps/Core/Routing/MWMRouter.mm

ArsenyN (Migrated from github.com) commented

2022-08-01 05:53:10 +00:00

fixed

ArsenyN (Migrated from github.com) reviewed 2022-08-01 05:53:54 +00:00

map/routing_manager.cpp

ArsenyN (Migrated from github.com) commented

2022-08-01 05:53:53 +00:00

I used uint32_t instead size_t

biodranik (Migrated from github.com) reviewed 2022-08-01 11:57:56 +00:00

biodranik (Migrated from github.com) left a comment

Almost there )

android/jni/com/mapswithme/maps/Framework.cpp

biodranik (Migrated from github.com) commented

2022-08-01 11:50:53 +00:00

Is there a warning here about uint to int conversion? static_cast<int32_t> is needed, probably also asserted that the uint value is not greater than an int one.

android/src/com/mapswithme/maps/routing/RoutingBottomMenuController.java

biodranik (Migrated from github.com) commented

2022-08-01 11:51:19 +00:00

final

iphone/Maps/Classes/CustomViews/NavigationDashboard/Views/RoutePreview/RoutePreviewStatus/BaseRoutePreviewStatus.swift

biodranik (Migrated from github.com) commented

2022-08-01 11:54:55 +00:00

Can this constant be a part of the class and reused, to avoid unnecessary memory allocations on each moment?

map/routing_manager.hpp

biodranik (Migrated from github.com) commented

2022-08-01 11:57:37 +00:00

  /// \param totalDescent is total descent of the route in altitudeUnits.

```suggestion /// \param totalDescent is total descent of the route in altitudeUnits. ```

ArsenyN (Migrated from github.com) reviewed 2022-08-01 14:31:48 +00:00

android/jni/com/mapswithme/maps/Framework.cpp

ArsenyN (Migrated from github.com) commented

2022-08-01 14:31:48 +00:00

Added explicit cast. But without assertion

biodranik (Migrated from github.com) approved these changes 2022-08-01 20:59:20 +00:00

biodranik (Migrated from github.com) left a comment

I would argue about the swift lambda formatting, but let's leave it for future PRs )

ArsenyN commented

2022-08-02 07:44:40 +00:00

(Migrated from github.com)

Thanks

This repo is archived. You cannot comment on pull requests.

No reviewers

biodranik

No labels

Android Automotive (AAOS)

API

AppGallery

AppStore

Battery and Performance

External Map Datasets

F-Droid

Fonts

Frequently User Reported

No milestone

No project

No assignees

1 participant

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: organicmaps/organicmaps-tmp#3062

				`@ -386,15 +387,16 @@ char const *kRenderAltitudeImagesQueueLabel = "mapsme.mwmrouter.renderAltitudeIm`
				`router.altitudeImagesData[sizeValue] = imageData;`

Rows
Columns