Skip to content
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

[android] Trace Path (a.k.a. Recent Track) #8183

Merged
merged 2 commits into from
Aug 24, 2024

Conversation

kavikhalique
Copy link
Contributor

[Android] Implements Recent Track Recording in android app of OM

@rtsisyk rtsisyk self-assigned this May 15, 2024
@rtsisyk rtsisyk self-requested a review May 15, 2024 11:49
Copy link
Member

@rtsisyk rtsisyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me play with this PR on my real devices.

@Jean-BaptisteC
Copy link
Contributor

Subtitle of record time is not update after user select a duration

@kavikhalique
Copy link
Contributor Author

Subtitle of record time is not update after user select a duration

Sorry if i am misunderstanding but do you mean the description below Record time title? Which contains text

Select duration for which recorded point will be shown on map ?

@Jean-BaptisteC
Copy link
Contributor

Update summary of preference with value selected by the user

@dmitrygribenchuk
Copy link

The setup failed my expectations. If you turn on/off track recording, the track disappears, despite the fact that the option to show 24 hours is set.
The track behaves strangely when changing the scale
6% of battery for bicycle ride for 25 minutes.

Next time I will compare it with my Garmin Edge. Visually it was nice, but I lost proofs (

Screenshot_2024-05-15-18-44-44-355_app organicmaps web debug

Screenrecorder-2024-05-15-18-44-27-816.mp4

@kavikhalique
Copy link
Contributor Author

The setup failed my expectations. If you turn on/off track recording, the track disappears, despite the fact that the option to show 24 hours is set.

Hi @dmitrygribenchuk thanks for your feedback :)

This feature is still underdevelopment. We will surely try to implement the features which are beneficial for the users. )

6% of battery for bicycle ride for 25 minutes.

Thanks for this feedback

Can you please provide details of your device to better understand app's behaviour and also kindly provide what was your initial battery percentage if you remember it.

@dmitrygribenchuk
Copy link

Sure. I have Xiaomi Redmi Note 12 Pro, it's starts with 37% end end with 31%

Also here is a 0.5 km dog walk to compare track from Garmin Device (it's pretty accurate)

Screenshot_2024-05-15-19-25-55-924_app organicmaps web debug-edit

@deevroman
Copy link
Contributor

deevroman commented May 15, 2024

Hello, I also tested this assembly on Samsung A52 Android 14 and in general it copes with the task of recording a track 👍

I also encountered the fact that after turning off the track recording, most of the track disappears, and the remaining random (!) part begins to blink.

From the wishes: the recording interval is now long, the track turns out rough even when walking quickly.

6% of battery for bicycle ride for 25 minutes.

It should be noted that the Debug build is much slower than the release build and probably consumes more battery

@kavikhalique
Copy link
Contributor Author

Hello, I also tested this assembly on Samsung A52 Android 14 and in general it copes with the task of recording a track 👍

Thanks for the feedback :)

I also encountered the fact that after turning off the track recording, most of the track disappears, and the remaining random (!) part begins to blink.

Will look into this issue 👍

From the wishes: the recording interval is now long, the track turns out rough even when walking quickly.

For now in background it is 10 sec interval keeping battery consumption as focus, but if it will affect accuracy then surely interval need to be decreased.

@rtsisyk rtsisyk changed the title Implements recent track recorder [android] Reimplement "Recent Track" feature May 16, 2024
@rtsisyk rtsisyk added the Android Android development label May 16, 2024
@biodranik
Copy link
Member

Is it possible to make it easier for users to enable/disable recent track feature in one click in Settings?

@kavikhalique
Copy link
Contributor Author

Is it possible to make it easier for users to enable/disable recent track feature in one click in Settings?

we can bring options on main setting screen instead of taking users to another activity for track recording controls.
But in future some more functionalities related to track recorder might be implemented and keeping all related functionalities in separate activity would be better.

Alternatively, we can create a separate section in main setting and there we can put all related settings. (Like we have general settings section, navigation etc...)

@rtsisyk
Copy link
Member

rtsisyk commented May 16, 2024

Feedback by @RicoElectrico from Telegram chat:

  • I would rename it to something else. Its more like plotting ;)
  • the notification should have some disable or settings button, in short: be actionable
  • does it need a submenu in settings? Why not off/1h/.../24h? And display that below option name just like with night mode

@dmitrygribenchuk
Copy link

dmitrygribenchuk commented May 16, 2024

One more test (2024.05.15-8-Google-beta, bicycle, average speed 17.2 kph) vs Garmin Edge

Screenshot_2024-05-16-22-36-03-694_app organicmaps beta

@biodranik
Copy link
Member

UPDATE: It has started to work after enabling notification in system settings:

Is it possible to check if notifications are disabled by the user and ask to enable them? Would it be helpful?

@kavikhalique
Copy link
Contributor Author

One more test (2024.05.15-8-Google-beta, bicycle, average speed 17.2 kph) vs Garmin Edge

Thanks for the feedback 👍

Accuracy seems low in comparison to Garmin. Can you please provide battery drain if by chance you remember it.

@kavikhalique
Copy link
Contributor Author

UPDATE: It has started to work after enabling notification in system settings:

Is it possible to check if notifications are disabled by the user and ask to enable them? Would it be helpful?

I have tested it in 2 devices and what i found is even if the notifications are disabled and notifications are not showing the app still runs in background as usual : )

For navigation it might not be good but for track recording its not a problem i guess.

@rtsisyk
Copy link
Member

rtsisyk commented May 17, 2024

Vendor: Samsung
Model: S10
Android: 12
ROM: One UI 4.1
Mobile data: OFF
Power save: ON

Result: no points recorded when app is in the background

@rtsisyk
Copy link
Member

rtsisyk commented May 17, 2024

Vendor: Google
Model: Pixel 4a
Android: 14
ROM: CalyxOS 5.6.3
Mobile data: OFF
Power save: OFF / ON - I don't see any difference
Google Play Location Services: ON (microG is installed, but no internet anyway)

Result: the track is ragged

@rtsisyk
Copy link
Member

rtsisyk commented May 17, 2024

Vendor: Samsung
Model: S10
Android: 12
ROM: One UI 4.1
Mobile data: OFF
Power save: OFF
Google Play Location Services: ON (no internet)
Navigation was enabled, i.e. interval was 0

The track is perfectly fine. I don't see any difference from iPhone.

@kavikhalique
Copy link
Contributor Author

Vendor: Samsung

Model: S10

Android: 12

ROM: One UI 4.1

Mobile data: OFF

Power save: ON

Result: no points recorded when app is in the background

I have looked on various websites and i have found Samsung kills most of the foreground services except some system app services.

Also some people stated that after the update to android 13 Samsung have stopped killing foreground services but in android 11/12 its very frequent.

Copy link
Member

@rtsisyk rtsisyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a few comments. Sorry for being picky here, but since this is your first major contribution, please pay attention to the code style. I won't comment on it further.

@kavikhalique
Copy link
Contributor Author

Pending tasks

  • Add blue dot in front of trace path feature in bottom sheet
  • Write Dialog box content for first time feature use
  • Reuse strings from ios
  • Regenerate strings

I have gone through all the review comments and found these 4 things which are pending, if there is anything else which i am missing please mention it here.

@Jean-BaptisteC
Copy link
Contributor

Jean-BaptisteC commented Aug 11, 2024

Why toast trace path is on appears after I change settings and leave screen settings (I have tried with theme app)?

@kavikhalique
Copy link
Contributor Author

kavikhalique commented Aug 11, 2024

Why toast trace path is on appears after I change settings and leave screen settings (I have tried with theme app)?

Do u mean bottom sheet should hide after the toast get finished?

Current implementation is that bottom sheet will hide as soon as trace path option will be clicked and all other tasks like toast, permission, first time use dialog will be shown on main map screen.

@kavikhalique
Copy link
Contributor Author

✅ blue dot in bottom sheet when feature is enabled
✅ rename the feature to start/stop trace path

Pending

  • Reuse of strings from ios
  • Content for first use dialog

@rtsisyk please provide me the content for first time use dialog so that i can regenerate strings and if there is anything else which i am missing please inform me )

@rtsisyk
Copy link
Member

rtsisyk commented Aug 12, 2024

I have updated the code to reduce the number of changes to translation strings. The current translation system is too inefficient for developers when adding new strings.

@biodranik
Copy link
Member

The current translation system is too inefficient for developers when adding new strings.

Which steps did you take to add translation strings, and what alternate steps could be better to compare the efficiency?

@rtsisyk
Copy link
Member

rtsisyk commented Aug 12, 2024

The current translation system is too inefficient for developers when adding new strings.

Which steps did you take to add translation strings, and what alternate steps could be better to compare the efficiency?

As a developer, I would like to add English version and leave all the translation work to translators. I am not proficient in 40 languages to review what crap translate.py is generating.

@Jean-BaptisteC
Copy link
Contributor

Récent parcours is really not easy to understood

Toast continue to be showed when I leave settings
How reproduce:

  • Start recorder path
  • Go to settings
  • Change theme settings
  • Go back to main screen -> Toast is showed again

@rtsisyk
Copy link
Member

rtsisyk commented Aug 13, 2024

Récent parcours is really not easy to understood

Could you please suggest a better name? I intentionally didn't touch translations of [pref_track_record_title], but I am not surprised that the existing translation, like many others, don't make sense.

Toast continue to be showed when I leave settings

Fixed. Thanks!

@biodranik
Copy link
Member

As a developer, I would like to add English version and leave all the translation work to translators. I am not proficient in 40 languages to review what crap translate.py is generating.

@rtsisyk

  1. What stops you from adding only en and ru versions which you know well with the current approach?
  2. Automated translations, from our experience, are great for most simpler cases. Why releasing versions untranslated is better than translated in such cases? I don't believe that Weblate timely provides all missing translations, right before a release. A more reliable alternative is to ask professional translators to do it before the release. An even better alternative is to ask to proof-read automated translations. WDYT?

Copy link
Member

@biodranik biodranik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good in general. Shall we test it in alpha/beta first before the merge?

<!--
Android 13 (API level 33) and higher supports a runtime permission for sending non-exempt (including Foreground
Services (FGS)) notifications from an app.
//-->
<uses-permission android:name="android.permission.POST_NOTIFICATIONS" />

<uses-permission android:name="android.permission.ACCESS_NOTIFICATION_POLICY" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OSMand, for example, does not declare this permission. Is it really needed?

// according to action of user. Calling it hack because we are avoiding
// creation of new methods by using this variable.
mLocationPermissionRequestedForRecording = true;
mLocationPermissionRequest.launch(new String[] { ACCESS_COARSE_LOCATION, ACCESS_FINE_LOCATION });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is a coarse permission check required? Or only a fine permission check can be done?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checking both would be convenient i guess. I have copied it from the implementation of navigation system since it is already implemented and tested.

@@ -2265,6 +2327,7 @@ public ArrayList<MenuBottomSheetItem> getMenuBottomSheetItems(String id)
if (!TextUtils.isEmpty(mDonatesUrl))
items.add(new MenuBottomSheetItem(R.string.donate, R.drawable.ic_donate, this::onDonateOptionSelected));
items.add(new MenuBottomSheetItem(R.string.settings, R.drawable.ic_settings, this::onSettingsOptionSelected));
items.add(new MenuBottomSheetItem(R.string.recent_track, R.drawable.ic_trace_path_off, -1, this::onTrackRecordingOptionSelected));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is badge count -1 a valid value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually it sends the count to show in the badge infront of bottom sheet menu item. But for trace path we needed only a dot so i sent -1 as a flag to distinguish between feasible number.

{
Logger.i(TAG, "Location helper knows app came in foreground");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit

Suggested change
Logger.i(TAG, "Location helper knows app came in foreground");
Logger.i(TAG, "onAppForeground");

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably it would be better not to touch the code that don't need to be changed.

@RequiresPermission(value = ACCESS_FINE_LOCATION)
public static void startForegroundService(@NonNull Context context)
{
if (TrackRecorder.nativeGetDuration() != 24)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: introduce a named constant to better understand what 24 means.

Comment on lines +206 to +210
final int verticalOffset = dpToPx(8, context) + dpToPx(Integer.toString(0)
.length() * 5, context);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit

Suggested change
final int verticalOffset = dpToPx(8, context) + dpToPx(Integer.toString(0)
.length() * 5, context);
final int verticalOffset = dpToPx(8, context) + dpToPx(Integer.toString(0).length() * 5, context);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function crashes on attempt to make a route:

Process: app.organicmaps.debug, PID: 5395
java.lang.NullPointerException: Attempt to invoke virtual method 'void android.view.View.getDrawingRect(android.graphics.Rect)' on a null object reference
at com.google.android.material.badge.BadgeUtils.setBadgeDrawableBounds(BadgeUtils.java:254)
at com.google.android.material.badge.BadgeUtils.attachBadgeDrawable(BadgeUtils.java:91)
at com.google.android.material.badge.BadgeUtils.attachBadgeDrawable(BadgeUtils.java:78)
at app.organicmaps.maplayer.MapButtonsController.updateMenuBadge(MapButtonsController.java:218)
at app.organicmaps.maplayer.MapButtonsController.$r8$lambda$Y3oUvRix7qylpmituPx43QoaZcg(Unknown Source:0)
at app.organicmaps.maplayer.MapButtonsController$$ExternalSyntheticLambda5.onChanged(D8$$SyntheticClass:0)
at androidx.lifecycle.LiveData.considerNotify(LiveData.java:133)
at androidx.lifecycle.LiveData.dispatchingValue(LiveData.java:146)
at androidx.lifecycle.LiveData$ObserverWrapper.activeStateChanged(LiveData.java:483)
at androidx.lifecycle.LiveData$LifecycleBoundObserver.onStateChanged(LiveData.java:440)
at androidx.lifecycle.LifecycleRegistry$ObserverWithState.dispatchEvent(LifecycleRegistry.jvm.kt:320)
at androidx.lifecycle.LifecycleRegistry.addObserver(LifecycleRegistry.jvm.kt:198)
at androidx.lifecycle.LiveData.observe(LiveData.java:205)
at app.organicmaps.maplayer.MapButtonsController.onStart(MapButtonsController.java:355)
at androidx.fragment.app.Fragment.performStart(Fragment.java:3192)
at androidx.fragment.app.FragmentStateManager.start(FragmentStateManager.java:648)
at androidx.fragment.app.FragmentStateManager.moveToExpectedState(FragmentStateManager.java:304)
at androidx.fragment.app.FragmentManager.executeOpsTogether(FragmentManager.java:2164)
at androidx.fragment.app.FragmentManager.removeRedundantOperationsAndExecute(FragmentManager.java:2059)
at androidx.fragment.app.FragmentManager.execPendingActions(FragmentManager.java:2002)
at androidx.fragment.app.FragmentManager$5.run(FragmentManager.java:702)
at android.os.Handler.handleCallback(Handler.java:938)
at android.os.Handler.dispatchMessage(Handler.java:99)
at android.os.Looper.loopOnce(Looper.java:226)
at android.os.Looper.loop(Looper.java:313)
at android.app.ActivityThread.main(ActivityThread.java:8751)
at java.lang.reflect.Method.invoke(Native Method)
at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:571)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1135)

@@ -193,6 +196,29 @@ private static int dpToPx(float dp, Context context)
return (int) TypedValue.applyDimension(TypedValue.COMPLEX_UNIT_DIP, dp, context.getResources().getDisplayMetrics());
}

@OptIn(markerClass = ExperimentalBadgeUtils.class)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the pros of using experimental code?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kavikhalique , is this experimental stuff needed at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method was pre implemented and these experimental badge features too. I have just created one more similar method with arguments.

I guess this is necessary here since we are dealing with badge which is experimental probably.

Please have a look into updateBadge() method in current master branch. This is already used there to show badges.

Intent intent = new Intent();
intent.setData(Uri.parse("package:" + context.getPackageName()));
intent.setAction(android.provider.Settings.ACTION_REQUEST_IGNORE_BATTERY_OPTIMIZATIONS);
if(isIntentSupported(context, intent))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit here and below: space after if

[recording_location]
comment = Shown as toast when starting the recent track recording
tags = android
en = Recording the location
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recording the track to avoid renaming it later for phase 2?

@rtsisyk
Copy link
Member

rtsisyk commented Aug 14, 2024

  1. What stops you from adding only en and ru versions which you know well with the current approach?

We will add Hindi and English the next time and ask you to help with other translations.

@Jean-BaptisteC
Copy link
Contributor

I suggest in French Enregistreur de traces

@rtsisyk
Copy link
Member

rtsisyk commented Aug 15, 2024

Update the PR to remove dead code from Utils.java and actualize French translations.

Copy link
Member

@rtsisyk rtsisyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pleaase fix a crash in updateMenuBadge().

Comment on lines +206 to +210
final int verticalOffset = dpToPx(8, context) + dpToPx(Integer.toString(0)
.length() * 5, context);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function crashes on attempt to make a route:

Process: app.organicmaps.debug, PID: 5395
java.lang.NullPointerException: Attempt to invoke virtual method 'void android.view.View.getDrawingRect(android.graphics.Rect)' on a null object reference
at com.google.android.material.badge.BadgeUtils.setBadgeDrawableBounds(BadgeUtils.java:254)
at com.google.android.material.badge.BadgeUtils.attachBadgeDrawable(BadgeUtils.java:91)
at com.google.android.material.badge.BadgeUtils.attachBadgeDrawable(BadgeUtils.java:78)
at app.organicmaps.maplayer.MapButtonsController.updateMenuBadge(MapButtonsController.java:218)
at app.organicmaps.maplayer.MapButtonsController.$r8$lambda$Y3oUvRix7qylpmituPx43QoaZcg(Unknown Source:0)
at app.organicmaps.maplayer.MapButtonsController$$ExternalSyntheticLambda5.onChanged(D8$$SyntheticClass:0)
at androidx.lifecycle.LiveData.considerNotify(LiveData.java:133)
at androidx.lifecycle.LiveData.dispatchingValue(LiveData.java:146)
at androidx.lifecycle.LiveData$ObserverWrapper.activeStateChanged(LiveData.java:483)
at androidx.lifecycle.LiveData$LifecycleBoundObserver.onStateChanged(LiveData.java:440)
at androidx.lifecycle.LifecycleRegistry$ObserverWithState.dispatchEvent(LifecycleRegistry.jvm.kt:320)
at androidx.lifecycle.LifecycleRegistry.addObserver(LifecycleRegistry.jvm.kt:198)
at androidx.lifecycle.LiveData.observe(LiveData.java:205)
at app.organicmaps.maplayer.MapButtonsController.onStart(MapButtonsController.java:355)
at androidx.fragment.app.Fragment.performStart(Fragment.java:3192)
at androidx.fragment.app.FragmentStateManager.start(FragmentStateManager.java:648)
at androidx.fragment.app.FragmentStateManager.moveToExpectedState(FragmentStateManager.java:304)
at androidx.fragment.app.FragmentManager.executeOpsTogether(FragmentManager.java:2164)
at androidx.fragment.app.FragmentManager.removeRedundantOperationsAndExecute(FragmentManager.java:2059)
at androidx.fragment.app.FragmentManager.execPendingActions(FragmentManager.java:2002)
at androidx.fragment.app.FragmentManager$5.run(FragmentManager.java:702)
at android.os.Handler.handleCallback(Handler.java:938)
at android.os.Handler.dispatchMessage(Handler.java:99)
at android.os.Looper.loopOnce(Looper.java:226)
at android.os.Looper.loop(Looper.java:313)
at android.app.ActivityThread.main(ActivityThread.java:8751)
at java.lang.reflect.Method.invoke(Native Method)
at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:571)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1135)

@kavikhalique
Copy link
Contributor Author

Pleaase fix a crash in updateMenuBadge().

Thanks for the report )
Fixed the crash.

@Jean-BaptisteC
Copy link
Contributor

Why Android continues to use recent path translations in menu?

@kavikhalique
Copy link
Contributor Author

Why Android continues to use recent path translations in menu?

I have not manipulated those strings probably @rtsisyk has changed them : )

@kavikhalique
Copy link
Contributor Author

All the requested changes are done except the content for dialog box. Should work upon it?
@rtsisyk

kavikhalique and others added 2 commits August 24, 2024 08:19
Signed-off-by: kavikhalique <[email protected]>
Co-authored-by: Roman Tsisyk <[email protected]>
Signed-off-by: Roman Tsisyk <[email protected]>
Signed-off-by: Roman Tsisyk <[email protected]>
@rtsisyk rtsisyk merged commit 90ee2af into organicmaps:master Aug 24, 2024
9 checks passed
@dmitrygribenchuk
Copy link

Hello. Since last beta (2024.08.29-23-Google-beta) found a casual UI bug during the navigation to wrong direction. Track record works rapidly and I saw this artefact
Screenshot_2024-08-29-19-45-35-444_app.organicmaps.beta.jpg

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Android Android development
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants