[API] Allow parsing and sharing coordinates in omaps.app links #10231

hemanggs · 2025-02-09T15:34:51Z

hemanggs commented

2025-02-09 15:34:51 +00:00

Partially Fixes #7482 ( iOS update remaining )

When Sharing location , Replaced the om:// format with URL with coordinates

before	after

Updated parser to accept coordinate URL's

Before

https://github.com/user-attachments/assets/cf5e56fa-de19-4576-809f-6e2165d55bc1

After

https://github.com/user-attachments/assets/bf054866-6235-435c-a479-f61fee3208ac

Wrote basic unit tests for the parser

Next step : Update parser for iOS

Partially Fixes #7482 ( iOS update remaining ) - When Sharing location , Replaced the `om://` format with URL with coordinates | before | after | |---------|---------| | ![before](https://github.com/user-attachments/assets/784b4657-9934-4248-8467-4237da050329)| ![image](https://github.com/user-attachments/assets/d0a80ec6-8e4a-48c4-9f37-a2a4c186c656)| - Updated parser to accept coordinate URL's # Before https://github.com/user-attachments/assets/cf5e56fa-de19-4576-809f-6e2165d55bc1 # After https://github.com/user-attachments/assets/bf054866-6235-435c-a479-f61fee3208ac - Wrote basic unit tests for the parser ![image](https://github.com/user-attachments/assets/a0465f30-66ae-4427-ab4b-d1bcd9a363bc) - Next step : Update parser for iOS

👍 2

RedAuburn reviewed 2025-02-09 15:34:51 +00:00

pastk reviewed 2025-02-09 15:34:51 +00:00

biodranik (Migrated from github.com) reviewed 2025-02-09 15:34:51 +00:00

RedAuburn requested changes 2025-02-09 17:08:33 +00:00

RedAuburn left a comment

Thanks!

android/app/src/main/java/app/organicmaps/util/SharingUtils.java

					
				@ -120,11 +120,11 @@ public class SharingUtils

				    final String subject = context.getString(R.string.share);

				    intent.putExtra(Intent.EXTRA_SUBJECT, subject);

RedAuburn commented

2025-02-09 16:44:46 +00:00

nit:

    final String text = context.getString(R.string.my_position_share_sms, httpUrl, coordUrl);

nit: ```suggestion final String text = context.getString(R.string.my_position_share_sms, httpUrl, coordUrl); ```

android/app/src/main/java/app/organicmaps/util/SharingUtils.java

					
				@ -140,12 +140,12 @@ public class SharingUtils

				                           context.getString(R.string.bookmark_share_email_subject);

				    intent.putExtra(Intent.EXTRA_SUBJECT, subject);

RedAuburn commented

2025-02-09 16:44:25 +00:00

nit:

    final String text = context.getString(R.string.my_position_share_email, address, httpUrl, coordUrl );

nit: ```suggestion final String text = context.getString(R.string.my_position_share_email, address, httpUrl, coordUrl ); ```

RedAuburn commented

2025-02-09 17:04:39 +00:00

It'd be good to append the POI name here like in the other URLs, then the encoded URL can be removed from the share message altogether (best to keep the handling though)

android/app/src/main/java/app/organicmaps/util/SharingUtils.java

					
				@ -161,3 +161,2 @@

				    final String geoUrl = Framework.nativeGetGe0Url(bookmark.getLat(), bookmark.getLon(),

				                                                    bookmark.getScale(), bookmark.getName());

				    final String coordUrl = Framework.nativeGetCoordUrl(bookmark.getLat(), bookmark.getLon(),

RedAuburn commented

2025-02-09 16:46:24 +00:00

same for adding a name here

biodranik (Migrated from github.com) reviewed 2025-02-09 17:33:59 +00:00

biodranik (Migrated from github.com) left a comment

Thanks for the good start! A few comments about the implementation.

android/app/src/main/java/app/organicmaps/util/SharingUtils.java

					
				@ -140,12 +140,12 @@ public class SharingUtils

				                           context.getString(R.string.bookmark_share_email_subject);

				    intent.putExtra(Intent.EXTRA_SUBJECT, subject);

biodranik (Migrated from github.com) commented

2025-02-09 17:33:30 +00:00

The idea is that the full link (that includes zoom, name, and maybe in the future some other parameters) is generated in C++. Then iOS, Android and desktop platforms can reuse it and extend it without unnecessary copy-pasting.

ge0/ge0_tests/parser_tests.cpp

biodranik (Migrated from github.com) commented

2025-02-09 16:51:00 +00:00

Add test with one or more slashes after the text (Place)? Also, adding ? and # in tests may be helpful. Note that any URL can be passed to the app, and the code should properly handle any possible combination (including harmful links made by bad actors).

Add test with one or more slashes after the text (Place)? Also, adding `?` and `#` in tests may be helpful. Note that _any_ URL can be passed to the app, and the code should properly handle _any_ possible combination (including harmful links made by bad actors).

biodranik (Migrated from github.com) commented

2025-02-09 16:51:57 +00:00

Add tests for /0,0, /-10,10, /10,0.32, /.123,-.456? Should they be valid or invalid?

Add tests for `/0,0`, `/-10,10`, `/10,0.32`, `/.123,-.456`? Should they be valid or invalid?

ge0/ge0_tests/parser_tests.cpp

					
				@ -312,4 +312,74 @@ UNIT_TEST(OtherPrefixes)

				  TestSuccess("https://omaps.app/Byqqqqqqqq", 45, 0, 4.25, "");

				  TestFailure("https://omaps.app/Byqqqqqqq");

				}

				UNIT_TEST(PlainCoordinateUrl_Valid)

biodranik (Migrated from github.com) commented

2025-02-09 17:03:10 +00:00

The current ge0 format stores and passes zoom level, so opened links can show roughly the same picture on the screen as it was when the link was created. This is very handy because the best zoom is already chosen at the moment of sharing the link.

There are different ways how to support zoom level in links, which one would be the best (compact, readable)? Maybe /lat,lon,zoom/name ? It likely also should be optional, and support floating-point format for better precision, right?

The current ge0 format stores and passes zoom level, so opened links can show roughly the same picture on the screen as it was when the link was created. This is very handy because the best zoom is already chosen at the moment of sharing the link. There are different ways how to support zoom level in links, which one would be the best (compact, readable)? Maybe `/lat,lon,zoom/name` ? It likely also should be optional, and support floating-point format for better precision, right?

ge0/ge0_tests/parser_tests.cpp

					
				@ -315,0 +374,4 @@

				  TestFailure("https://omaps.app//Name");

				  TestFailure("https://omaps.app/13.02227,77.76043foo");

				  TestFailure("https://omaps.app/foo13.02227,77.76043");

biodranik (Migrated from github.com) commented

2025-02-09 16:50:02 +00:00

Add tests for lat/lon that are out of the valid range?

ge0/parser.cpp

biodranik (Migrated from github.com) commented

2025-02-09 17:05:53 +00:00

std::string_view avoids creating copies of the original string, while uses the same string api.

biodranik (Migrated from github.com) commented

2025-02-09 17:26:02 +00:00

Would using sscanf be cleaner/easier to read and support? For example:

    double lat, lon, zoom;
    switch (sscanf(url.c_str(), "%lf,%lf,%lf", &lat, &lon, &zoom))
    {
      case 2:  // Only lat and lon were parsed.
      break;
      case 3:  // lat, lon and zoom were parsed.
      break;
      default:  // Incompatible URL format
      break;
    }

Would using [sscanf](https://en.cppreference.com/w/cpp/io/c/fscanf) be cleaner/easier to read and support? For example: ```C++ double lat, lon, zoom; switch (sscanf(url.c_str(), "%lf,%lf,%lf", &lat, &lon, &zoom)) { case 2: // Only lat and lon were parsed. break; case 3: // lat, lon and zoom were parsed. break; default: // Incompatible URL format break; } ```

hemanggs reviewed 2025-02-10 10:35:40 +00:00

android/app/src/main/java/app/organicmaps/util/SharingUtils.java

					
				@ -140,12 +140,12 @@ public class SharingUtils

				                           context.getString(R.string.bookmark_share_email_subject);

				    intent.putExtra(Intent.EXTRA_SUBJECT, subject);

hemanggs commented

2025-02-10 10:35:40 +00:00

are you sure , we should remove encoded message completely

hemanggs reviewed 2025-02-10 10:44:48 +00:00

ge0/ge0_tests/parser_tests.cpp

hemanggs commented

2025-02-10 10:44:48 +00:00

okay 👍 , they are in range should be valid.

hemanggs reviewed 2025-02-10 11:02:30 +00:00

ge0/parser.cpp

hemanggs commented

2025-02-10 11:02:30 +00:00

yes that is a lot cleaner , thanks!

pastk reviewed 2025-02-26 04:39:21 +00:00

android/app/src/main/cpp/app/organicmaps/Framework.cpp

pastk commented

2025-02-26 04:34:11 +00:00

is it url encoded?

pastk commented

2025-02-26 04:34:50 +00:00

please use a const for https://omaps.app/

please use a const for `https://omaps.app/`

pastk commented

2025-02-26 04:37:41 +00:00

are there tests for url generation?

ge0/ge0_tests/parser_tests.cpp

					
				@ -315,0 +326,4 @@

				  TestSuccess("https://omaps.app/89.99999,-179.99999", 89.99999, -179.99999, 17.0, "");

				  TestSuccess("https://omaps.app/-89.99999,179.99999", -89.99999, 179.99999, 17.0, "");

				  TestSuccess("https://omaps.app/12.34567,76.54321/Place", 12.34567, 76.54321, 17.0, "Place");

pastk commented

2025-02-26 04:31:28 +00:00

ideally it'd use default zoom

pastk commented

2025-02-26 04:32:39 +00:00

what about whitespace? works also?

ge0/parser.cpp

pastk commented

2025-02-26 04:24:11 +00:00

better use consts, e.g. there is GetUpperComfortScale() in scales.hpp

pastk commented

2025-02-26 04:25:31 +00:00

max zoom is GetUpperStyleScale()

pastk commented

2025-02-26 04:25:54 +00:00

are there any consts for these already?

ge0/parser.cpp

					
				@ -53,0 +125,4 @@

				      zoom = 1.0;

				    if (zoom > scales::GetUpperStyleScale())

				      zoom = scales::GetUpperStyleScale();

				    result.m_lat = lat;

pastk commented

2025-02-26 04:30:53 +00:00

if zoom is out of range then would be better to use default instead of failing completely?
or even better use the closest in-range value
e.g. if its 0.5 then fix to 1
if its 22 then fix to 20

if zoom is out of range then would be better to use default instead of failing completely? or even better use the closest in-range value e.g. if its 0.5 then fix to 1 if its 22 then fix to 20

biodranik (Migrated from github.com) reviewed 2025-03-01 10:07:49 +00:00

android/app/src/main/cpp/app/organicmaps/Framework.cpp

biodranik (Migrated from github.com) commented

2025-03-01 10:07:49 +00:00

Why URLs are generated in Android code only? What are the pros of repeating this code in iOS and on desktop? Why it can't be written in C++ and covered with unit tests, as everything else?

hemanggs reviewed 2025-03-03 08:36:42 +00:00

android/app/src/main/cpp/app/organicmaps/Framework.cpp

hemanggs commented

2025-03-03 08:36:42 +00:00

Agreed this is should be better , thanks.

hemanggs reviewed 2025-03-03 11:54:20 +00:00

ge0/ge0_tests/parser_tests.cpp

					
				@ -315,0 +326,4 @@

				  TestSuccess("https://omaps.app/89.99999,-179.99999", 89.99999, -179.99999, 17.0, "");

				  TestSuccess("https://omaps.app/-89.99999,179.99999", -89.99999, 179.99999, 17.0, "");

				  TestSuccess("https://omaps.app/12.34567,76.54321/Place", 12.34567, 76.54321, 17.0, "Place");

hemanggs commented

2025-03-03 11:54:20 +00:00

yes works added a test

biodranik (Migrated from github.com) reviewed 2025-03-03 20:32:17 +00:00

android/app/src/main/cpp/app/organicmaps/Framework.cpp

biodranik (Migrated from github.com) commented

2025-03-03 20:03:18 +00:00

nit:

  std::string const nameStr = jni::ToNativeString(env, name);

nit: ```suggestion std::string const nameStr = jni::ToNativeString(env, name); ```

biodranik (Migrated from github.com) commented

2025-03-03 20:03:27 +00:00

```suggestion ```

biodranik (Migrated from github.com) commented

2025-03-03 20:03:49 +00:00

nit:

  std::string const url = ge0::GenerateCoordUrl(lat, lon, scale, nameStr);

nit: ```suggestion std::string const url = ge0::GenerateCoordUrl(lat, lon, scale, nameStr); ```

android/app/src/main/java/app/organicmaps/Framework.java

biodranik (Migrated from github.com) commented

2025-03-03 20:05:03 +00:00

Is this java wrapper method really needed? Why is it better than calling Framework.nativeGetCoordUrl(lat, lon, zoomLevel, name); directly where necessary?

Is this java wrapper method really needed? Why is it better than calling `Framework.nativeGetCoordUrl(lat, lon, zoomLevel, name);` directly where necessary?

android/app/src/main/java/app/organicmaps/util/SharingUtils.java

					
				@ -120,11 +120,11 @@ public class SharingUtils

				    final String subject = context.getString(R.string.share);

				    intent.putExtra(Intent.EXTRA_SUBJECT, subject);

biodranik (Migrated from github.com) commented

2025-03-03 20:08:59 +00:00

```suggestion ```

android/app/src/main/java/app/organicmaps/util/SharingUtils.java

					
				@ -142,3 +142,2 @@

				    final String geoUrl = Framework.nativeGetGe0Url(object.getLat(), object.getLon(),

				                                                    object.getScale(), object.getName());

				    final String coordUrl = Framework.nativeGetCoordUrl(object.getLat(), object.getLon(),

biodranik (Migrated from github.com) commented

2025-03-03 20:07:40 +00:00

nit:

                                                  object.getScale(), object.getName());

nit: ```suggestion object.getScale(), object.getName()); ```

biodranik (Migrated from github.com) commented

2025-03-03 20:10:13 +00:00

```suggestion ```

android/app/src/main/java/app/organicmaps/util/SharingUtils.java

					
				@ -163,2 +162,3 @@

				                                                    bookmark.getScale(), bookmark.getName());

				    final String coordUrl = Framework.nativeGetCoordUrl(bookmark.getLat(), bookmark.getLon(),

				                                                        bookmark.getScale(), bookmark.getName());

				    final String httpUrl = Framework.getHttpGe0Url(bookmark.getLat(), bookmark.getLon(),

biodranik (Migrated from github.com) commented

2025-03-03 20:11:34 +00:00

Did you see any warnings about unused variables?

Did you see any warnings about unused variables? ```suggestion ```

ge0/ge0_tests/parser_tests.cpp

					
				@ -315,0 +343,4 @@

				  TestSuccess("https://omaps.app/12.34567,76.54321,10.5", 12.34567, 76.54321, 10.5, "");

				  TestSuccess("https://omaps.app/12.34567,76.54321,15.75/Place", 12.34567, 76.54321, 15.75, "Place");

				  TestSuccess("https://omaps.app/13.02227,77.76043,18.0", 13.02227, 77.76043, 18.0, "");

biodranik (Migrated from github.com) commented

2025-03-03 20:15:28 +00:00

  TestSuccess("https://omaps.app/13.02227,77.76043,18.0", 13.02227, 77.76043, 18.0, "");
  TestSuccess("https://omaps.app/13.02227,77.76043,18.0#", 13.02227, 77.76043, 18.0, "");
    TestSuccess("https://omaps.app/13.02227,77.76043,18.0?", 13.02227, 77.76043, 18.0, "");

```suggestion TestSuccess("https://omaps.app/13.02227,77.76043,18.0", 13.02227, 77.76043, 18.0, ""); TestSuccess("https://omaps.app/13.02227,77.76043,18.0#", 13.02227, 77.76043, 18.0, ""); TestSuccess("https://omaps.app/13.02227,77.76043,18.0?", 13.02227, 77.76043, 18.0, ""); ```

ge0/ge0_tests/parser_tests.cpp

					
				@ -315,0 +351,4 @@

				  // Out of range clamping

				  TestSuccess("https://omaps.app/13.02227,77.76043,100", 13.02227, 77.76043, 20.0, "");

				  TestSuccess("https://omaps.app/13.02227,77.76043,0.5", 13.02227, 77.76043, 1.0, "");

biodranik (Migrated from github.com) commented

2025-03-03 20:13:28 +00:00

Are negative zoom values discarded or clamped?

ge0/ge0_tests/url_generator_tests.cpp

biodranik (Migrated from github.com) commented

2025-03-03 20:16:37 +00:00

nit here and below:

  auto const url = GenerateCoordUrl(0, 0, 19, "Name");

nit here and below: ```suggestion auto const url = GenerateCoordUrl(0, 0, 19, "Name"); ```

biodranik (Migrated from github.com) commented

2025-03-03 20:18:08 +00:00

Can all these tests be moved to the list of PlainCoordinateUrl_Valid ? Why are they separated?

ge0/url_generator.cpp

biodranik (Migrated from github.com) commented

2025-03-03 20:23:19 +00:00

No need in introducing unnecessary allocation and copying here:

  std::string url = "https://omaps.app/";

No need in introducing unnecessary allocation and copying here: ```suggestion std::string url = "https://omaps.app/"; ```

biodranik (Migrated from github.com) commented

2025-03-03 20:29:26 +00:00

  auto const writtenChars = snprintf(buf, sizeof(buf), "%.5f,%.5f,%.0f", lat, lon, zoom);
  CHECK(writtenChars > 0 && writtenChars < sizeof(buf), ("Buffer is too small", writtenChars, lat, lon, zoom));

```suggestion auto const writtenChars = snprintf(buf, sizeof(buf), "%.5f,%.5f,%.0f", lat, lon, zoom); CHECK(writtenChars > 0 && writtenChars < sizeof(buf), ("Buffer is too small", writtenChars, lat, lon, zoom)); ```

biodranik (Migrated from github.com) commented

2025-03-03 20:31:37 +00:00

  {
    url.push_back('/');
    url.append(UrlEncodeString(TransformName(name)));
  }

```suggestion { url.push_back('/'); url.append(UrlEncodeString(TransformName(name))); } ```

hemanggs reviewed 2025-03-04 03:09:51 +00:00

ge0/ge0_tests/parser_tests.cpp

					
				@ -315,0 +351,4 @@

				  // Out of range clamping

				  TestSuccess("https://omaps.app/13.02227,77.76043,100", 13.02227, 77.76043, 20.0, "");

				  TestSuccess("https://omaps.app/13.02227,77.76043,0.5", 13.02227, 77.76043, 1.0, "");