[android] Fixes of the track recorder #9174

rtsisyk · 2024-08-31T07:58:03Z

rtsisyk commented

2024-08-31 07:58:03 +00:00

Don't clear the recent track on the app start

TrackRecorder.nativeSetEnabled() calls ConnectToGpsTracker()
which calls m_drapeEngine->ClearGpsTrackPoints().

Make the power-saving dialog non-blocking and show it
only if location can't be accured in 30 seconds.

Closes #9146

1. Don't clear the recent track on the app start TrackRecorder.nativeSetEnabled() calls ConnectToGpsTracker() which calls m_drapeEngine->ClearGpsTrackPoints(). 2. Make the power-saving dialog non-blocking and show it only if location can't be accured in 30 seconds. Closes #9146

biodranik (Migrated from github.com) reviewed 2024-08-31 09:17:03 +00:00

biodranik (Migrated from github.com) left a comment

Thanks!

Should a toast/warning be displayed if startTrackRecording failed to start? How is it handled now?
The current implementation is clunky:

Instead of

    if (toast)
      Toast.makeText(this, R.string.track_recording, Toast.LENGTH_SHORT).show();

this may look clearer:

if (!startTrackRecording())
        Toast.makeText(this, R.string.track_recording, Toast.LENGTH_SHORT).show();

Thanks! 1. Should a toast/warning be displayed if startTrackRecording failed to start? How is it handled now? 2. The current implementation is clunky: Instead of ``` if (toast) Toast.makeText(this, R.string.track_recording, Toast.LENGTH_SHORT).show(); ``` this may look clearer: ``` if (!startTrackRecording()) Toast.makeText(this, R.string.track_recording, Toast.LENGTH_SHORT).show(); ```

biodranik (Migrated from github.com) reviewed 2024-09-01 18:11:35 +00:00

biodranik (Migrated from github.com) left a comment

The current body of startTrackRecording() checks permissions and battery optimization, and returns/fails silently if conditions are not satisfied. Either the failed return should be handled, or these checks should be done outside of the startTrackRecording() to ask user/let user know about these failure cases.

The current body of `startTrackRecording()` checks permissions and battery optimization, and returns/fails silently if conditions are not satisfied. Either the failed return should be handled, or these checks should be done outside of the startTrackRecording() to ask user/let user know about these failure cases.

kavikhalique (Migrated from github.com) requested changes 2024-09-02 10:01:05 +00:00

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

					
				@ -284,3 +276,4 @@

				    }

				    processIntent();

				    migrateOAuthCredentials();

kavikhalique (Migrated from github.com) commented

2024-09-02 09:55:05 +00:00

There is one issue
If user do not turns off battery saver or turns it on later and clears the app from recent apps then restarts the app again then startTrackRecording(false) will be called which will return false because battery saver is on and then this condition will turn off the native part and then restart it again which will remove all the tracks.

same situation happens if we rotate the map.

Steps to regenerate -

1 - Turn on battery saver
2 - Now turn on Recent Track feature and do not turn off the battery saver (just click not now)
3 - Close the app and clear it from recent apps or simply rotate your phone to landscape orientation
4 - The tracks will get cleaned in both the cases since native will be disabled and re enabled again.

There is one issue If user do not turns off battery saver or turns it on later and clears the app from recent apps then restarts the app again then startTrackRecording(false) will be called which will return false because battery saver is on and then this condition will turn off the native part and then restart it again which will remove all the tracks. same situation happens if we rotate the map. Steps to regenerate - 1 - Turn on battery saver 2 - Now turn on Recent Track feature and do not turn off the battery saver (just click not now) 3 - Close the app and clear it from recent apps or simply rotate your phone to landscape orientation 4 - The tracks will get cleaned in both the cases since native will be disabled and re enabled again.

rtsisyk reviewed 2024-09-03 09:18:59 +00:00

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

					
				@ -284,3 +276,4 @@

				    }

				    processIntent();

				    migrateOAuthCredentials();

rtsisyk commented

2024-09-03 09:18:59 +00:00

What do you think about making battery saver dialog non-blocking? It will make the logic significantly simpler..

rtsisyk reviewed 2024-09-03 09:33:13 +00:00

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

					
				@ -284,3 +276,4 @@

				    }

				    processIntent();

				    migrateOAuthCredentials();

rtsisyk commented

2024-09-03 09:33:13 +00:00

I thought about it twice - we have a red alert in the notification bar if the location doesn't work. I think that it will be fine to show to start the recording and then show the dialog about power saving in non-blocking mode.

biodranik (Migrated from github.com) reviewed 2024-09-03 14:32:06 +00:00

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

					
				@ -284,3 +276,4 @@

				    }

				    processIntent();

				    migrateOAuthCredentials();

biodranik (Migrated from github.com) commented

2024-09-03 14:32:06 +00:00

With a red alert the dialog may not be necessary at all (if alert properly explains the issue and maybe even opens the battery/app system settings?)

kavikhalique (Migrated from github.com) reviewed 2024-09-03 14:40:51 +00:00

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

					
				@ -284,3 +276,4 @@

				    }

				    processIntent();

				    migrateOAuthCredentials();

kavikhalique (Migrated from github.com) commented

2024-09-03 14:40:51 +00:00

But that notification may become red due to many reasons other than
battery saver issue such as if location fix is not there because user entered in tunnel or indoor (temporarily).

If suppose battery saver is turned off and still notification became red due to some location errors.

But that notification may become red due to many reasons other than battery saver issue such as if location fix is not there because user entered in tunnel or indoor (temporarily). If suppose battery saver is turned off and still notification became red due to some location errors.

biodranik (Migrated from github.com) reviewed 2024-09-03 15:53:11 +00:00

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

					
				@ -284,3 +276,4 @@

				    }

				    processIntent();

				    migrateOAuthCredentials();

biodranik (Migrated from github.com) commented

2024-09-03 15:53:11 +00:00

Can notification properly handle different cases? Then it would perfectly solve these issues.

kavikhalique (Migrated from github.com) reviewed 2024-09-03 17:40:41 +00:00

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

					
				@ -284,3 +276,4 @@

				    }

				    processIntent();

				    migrateOAuthCredentials();

kavikhalique (Migrated from github.com) commented

2024-09-03 17:40:41 +00:00

Can notification properly handle different cases? Then it would perfectly solve these issues.

Yess, notification will switch to red as soon as location updates to track recorder will be blocked for 30 seconds.

> Can notification properly handle different cases? Then it would perfectly solve these issues. Yess, notification will switch to red as soon as location updates to track recorder will be blocked for 30 seconds.

kavikhalique (Migrated from github.com) reviewed 2024-09-03 17:46:29 +00:00

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

					
				@ -284,3 +276,4 @@

				    }

				    processIntent();

				    migrateOAuthCredentials();

kavikhalique (Migrated from github.com) commented

2024-09-03 17:46:29 +00:00

I thought about it twice - we have a red alert in the notification bar if the location doesn't work. I think that it will be fine to show to start the recording and then show the dialog about power saving in non-blocking mode.

Yess, that will also solve the issue of track deletion.

I have just one more concern, since there are two non blocking dialogs simultaneously one after other (post notifications and battery saver). Wouldn't they interfere each other? Like killing the first dialog and showing second one without even taking user input?

> I thought about it twice - we have a red alert in the notification bar if the location doesn't work. I think that it will be fine to show to start the recording and then show the dialog about power saving in non-blocking mode. > > Yess, that will also solve the issue of track deletion. I have just one more concern, since there are two non blocking dialogs simultaneously one after other (post notifications and battery saver). Wouldn't they interfere each other? Like killing the first dialog and showing second one without even taking user input?

rtsisyk reviewed 2024-09-04 08:53:40 +00:00

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

					
				@ -284,3 +276,4 @@

				    }

				    processIntent();

				    migrateOAuthCredentials();

rtsisyk commented

2024-09-04 08:53:40 +00:00

@kavikhalique , I have reworked the PR. PTAL.

rtsisyk reviewed 2024-09-04 08:54:53 +00:00

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

					
				@ -284,3 +276,4 @@

				    }

				    processIntent();

				    migrateOAuthCredentials();

rtsisyk commented

2024-09-04 08:54:53 +00:00

In this particular place, please call TrackRecorder.saveTrack() after rebasing on the new API.

rtsisyk reviewed 2024-09-04 11:20:42 +00:00

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

					
				@ -284,3 +276,4 @@

				    }

				    processIntent();

				    migrateOAuthCredentials();

rtsisyk commented

2024-09-04 11:20:42 +00:00

@kavikhalique do you see any issues with a test case you described?

kavikhalique (Migrated from github.com) reviewed 2024-09-04 12:51:09 +00:00

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

					
				@ -284,3 +276,4 @@

				    }

				    processIntent();

				    migrateOAuthCredentials();

kavikhalique (Migrated from github.com) commented

2024-09-04 12:51:09 +00:00

@kavikhalique do you see any issues with a test case you described?

No, it works properly now. I am trying to cover all the cases where it could fail then i will test it too. Will provide feedback in some time.

> @kavikhalique do you see any issues with a test case you described? No, it works properly now. I am trying to cover all the cases where it could fail then i will test it too. Will provide feedback in some time.

kavikhalique (Migrated from github.com) reviewed 2024-09-04 14:16:01 +00:00

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

kavikhalique (Migrated from github.com) commented

2024-09-04 14:16:00 +00:00

Here is one issue. Since both are non blocking notifications both are called simultaneously due to which the battery saver permission do not stays and gets removed by post notification permission request. I have tested it in android 14 device.

Steps to regenerate
1- Use android 13 or above phone
2- Remove notification permission for OM and also turn on battery saver
3- Start the trace path feature in OM.
4- for a second dialog to disable power saver will appear and then it will be removed and post notification permission dialog will appear.

Another not much important concern is about asking post notification permission again and again on every start of track recorder. Since the UX logic for battery saver is to ask only once then it would be better to design this in same manner as well. Although android do not let the app ask post notification permission again and again if once it has been denied by user but i am proposing it for OM's UX : )

Here is one issue. Since both are non blocking notifications both are called simultaneously due to which the battery saver permission do not stays and gets removed by post notification permission request. I have tested it in android 14 device. Steps to regenerate 1- Use android 13 or above phone 2- Remove notification permission for OM and also turn on battery saver 3- Start the trace path feature in OM. 4- for a second dialog to disable power saver will appear and then it will be removed and post notification permission dialog will appear. Another not much important concern is about asking post notification permission again and again on every start of track recorder. Since the UX logic for battery saver is to ask only once then it would be better to design this in same manner as well. Although android do not let the app ask post notification permission again and again if once it has been denied by user but i am proposing it for OM's UX : )

rtsisyk reviewed 2024-09-06 08:51:43 +00:00

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

rtsisyk commented

2024-09-06 08:51:43 +00:00

Here is one issue. Since both are non blocking notifications both are called simultaneously due to which the battery saver permission do not stays and gets removed by post notification permission request. I have tested it in android 14 device.

Confirmed. I think that the right solution would be implementing a stateful step-by-step wizard to request all prerequisites: 1. Fine location 2. POST_NOTIFICATION 3. Battery saver off, both for the track recording and navigation. This is a complex topic that would be better addressed separately. I will file a ticket. I am searching for a workaround to move forward here.

> Here is one issue. Since both are non blocking notifications both are called simultaneously due to which the battery saver permission do not stays and gets removed by post notification permission request. I have tested it in android 14 device. Confirmed. I think that the right solution would be implementing a stateful step-by-step wizard to request all prerequisites: 1. Fine location 2. POST_NOTIFICATION 3. Battery saver off, both for the track recording and navigation. This is a complex topic that would be better addressed separately. I will file a ticket. I am searching for a workaround to move forward here.

rtsisyk reviewed 2024-09-06 09:37:56 +00:00

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

rtsisyk commented

2024-09-06 09:37:56 +00:00

@kavikhalique , PR has been reworked to show the dialog on 30 second timeout.

rtsisyk reviewed 2024-09-06 09:39:01 +00:00

android/app/src/main/java/app/organicmaps/location/LocationHelper.java

rtsisyk commented

2024-09-06 09:39:01 +00:00

@kavikhalique , I've moved the timeout handling logic out of TrackRecordingService into LocationHelper to re-use from MwmActivity.

rtsisyk reviewed 2024-09-06 09:39:35 +00:00

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

rtsisyk commented

2024-09-06 09:39:35 +00:00

@kavikhalique , the dialog is now only shown in case if location timed out.

rtsisyk reviewed 2024-09-06 09:40:08 +00:00

data/strings/strings.txt

rtsisyk commented

2024-09-06 09:40:08 +00:00

Re-used [current_location_unknown_error_title]
tags = android,ios
en = Current location is unknown.

Re-used [current_location_unknown_error_title] tags = android,ios en = Current location is unknown.

kavikhalique (Migrated from github.com) reviewed 2024-09-06 17:17:30 +00:00

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

kavikhalique (Migrated from github.com) commented

2024-09-06 17:00:20 +00:00

This idea seems good to me but do location updates stop when app runs in foreground? As you mentioned earlier location updates stops only when app goes in background. If that is the case then dialog will never be shown.

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

This idea seems good to me but do location updates stop when app runs in foreground? As you mentioned earlier location updates stops only when app goes in background. If that is the case then dialog will never be shown. >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

android/app/src/main/java/app/organicmaps/location/LocationHelper.java

kavikhalique (Migrated from github.com) commented

2024-09-06 17:14:04 +00:00

This removal will arise a case in which runnable will be removed from empty handler.

steps
1 - on start runnable will be added in handler
2 - if location update stops it will trigger notifyLocationUpdateTimeout() which will remove runnable.
3 - Now again location gets fixed and it updates then runnable will be removed which do not exists in handler.

This removal will arise a case in which runnable will be removed from empty handler. steps 1 - on start runnable will be added in handler 2 - if location update stops it will trigger notifyLocationUpdateTimeout() which will remove runnable. 3 - Now again location gets fixed and it updates then runnable will be removed which do not exists in handler.

rtsisyk reviewed 2024-09-06 20:04:53 +00:00

android/app/src/main/java/app/organicmaps/location/LocationHelper.java

rtsisyk commented

2024-09-06 20:04:53 +00:00

in case 3 notifyLocationChanged() re-adds the handler

rtsisyk reviewed 2024-09-07 10:37:11 +00:00

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

rtsisyk commented

2024-09-07 10:37:11 +00:00

I enabled "For the most accurate navigation, we recommend disabling power saving mode in phone's battery settings" dialog only for the new track recorder to avoid potential regressions in navigation and other places. Let's pilot it in the track recorder and enable globally for the next release.

CC: @kavikhalique

I enabled "For the most accurate navigation, we recommend disabling power saving mode in phone's battery settings" dialog only for the new track recorder to avoid potential regressions in navigation and other places. Let's pilot it in the track recorder and enable globally for the next release. CC: @kavikhalique

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

No reviewers

kavikhalique