[android] Trace Path (a.k.a. Recent Track) #8183
No reviewers
Labels
No labels
Accessibility
Accessibility
Address
Address
Android
Android
Android Auto
Android Auto
Android Automotive (AAOS)
Android Automotive (AAOS)
API
API
AppGallery
AppGallery
AppStore
AppStore
Battery and Performance
Battery and Performance
Blocker
Blocker
Bookmarks and Tracks
Bookmarks and Tracks
Borders
Borders
Bug
Bug
Build
Build
CarPlay
CarPlay
Classificator
Classificator
Community
Community
Core
Core
CrashReports
CrashReports
Cycling
Cycling
Desktop
Desktop
DevEx
DevEx
DevOps
DevOps
dev_sandbox
dev_sandbox
Directions
Directions
Documentation
Documentation
Downloader
Downloader
Drape
Drape
Driving
Driving
Duplicate
Duplicate
Editor
Editor
Elevation
Elevation
Enhancement
Enhancement
Epic
Epic
External Map Datasets
External Map Datasets
F-Droid
F-Droid
Fonts
Fonts
Frequently User Reported
Frequently User Reported
Fund
Fund
Generator
Generator
Good first issue
Good first issue
Google Play
Google Play
GPS
GPS
GSoC
GSoC
iCloud
iCloud
Icons
Icons
iOS
iOS
Legal
Legal
Linux Desktop
Linux Desktop
Linux packaging
Linux packaging
Linux Phone
Linux Phone
Mac OS
Mac OS
Map Data
Map Data
Metro
Metro
Navigation
Navigation
Need Feedback
Need Feedback
Night Mode
Night Mode
NLnet 2024-06-281
NLnet 2024-06-281
No Feature Parity
No Feature Parity
Opening Hours
Opening Hours
Outdoors
Outdoors
POI Info
POI Info
Privacy
Privacy
Public Transport
Public Transport
Raw Idea
Raw Idea
Refactoring
Refactoring
Regional
Regional
Regression
Regression
Releases
Releases
RoboTest
RoboTest
Route Planning
Route Planning
Routing
Routing
Ruler
Ruler
Search
Search
Security
Security
Styles
Styles
Tests
Tests
Track Recording
Track Recording
Translations
Translations
TTS
TTS
UI
UI
UX
UX
Walk Navigation
Walk Navigation
Watches
Watches
Web
Web
Wikipedia
Wikipedia
Windows
Windows
Won't fix
Won't fix
World Map
World Map
No milestone
No project
No assignees
2 participants
Due date
No due date set.
Dependencies
No dependencies set.
Reference: organicmaps/organicmaps-tmp#8183
Loading…
Add table
Reference in a new issue
No description provided.
Delete branch "recent-track-recorder"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
[Android] Implements Recent Track Recording in android app of OM
Actually, INTERVAL_TRACK_RECORDING_BACKGROUND = 10000 for the "Recent Track" feature is not a bad idea. The Recent Track probably doesn't need to be very precise. The only purpose of the Recent Track feature is to visualize the last traveled path on the map by using a dotted line. Even the 10 seconds pull interval is most likely good enough for this task. Let me play with this PR.
Please try to avoid reformatting. Yes, I know that the code is not properly formatted according to the latest Android Studio settings. Let's fix it separately.
Let me play with this PR on my real devices.
Ok sir will keep in mind. 👍
@ -0,0 +53,4 @@
}
@RequiresPermission(value = ACCESS_FINE_LOCATION)
public static void startForegroundService(@NonNull Context context)
I haven't seen any notifications, neither after enabling "Record Track" in the settings, nor after restarting the app. Please check for POST_NOTIFICATION permission. It is probably better to do from MwmActivity. See
a7127ccde8/android/app/src/main/java/app/organicmaps/MwmActivity.java (L1843-L1854)
.UPDATE: It has started to work after enabling notification in system settings:

@ -0,0 +53,4 @@
}
@RequiresPermission(value = ACCESS_FINE_LOCATION)
public static void startForegroundService(@NonNull Context context)
I have transferred the calling of service from preference class to mwmActivity class.
Although without notification permission i don't think it will show any notification. I tried with navigation as well but no notification shows if notification is turned off in settings.
@ -0,0 +53,4 @@
}
@RequiresPermission(value = ACCESS_FINE_LOCATION)
public static void startForegroundService(@NonNull Context context)
This bug still present. Moreover, I don't see a notification when I start the app with the enabled option. I suspect that the foreground service doesn't start either. Enabling/disabling option in the settings fixes the issue.
6de4a18c0b210c8eded23a8842011c341f0d9bec (HEAD -> recent-track-recorder)
Author: kavikhalique kavikhalique3@gmail.com
Date: Fri May 17 02:45:21 2024 +0530
I have been playing with this PR for two days, using different devices. 10000ms is definitely not enough. It produces ragged tracks even when walking, not to mention driving. The effective minimum on Android is 1000ms (any value less than 1000ms is effectively treated as 1000ms). 10000ms is too slow. We'll determine the optimal value within this range or create a setting. For now, please update INTERVAL_TRACK_RECORDING_BACKGROUND to 3000ms.
I suggest not wasting time on it now and always use the best precision/interval (zero). It will provide a good understanding and base raw data/stats that we can rely on later if decide to improve something.
Note that any OSMer or driver/cycler expects the track to be recorded with the max possible precision.
TrackRecorder is an abstract class for native methods, isn't it? I expect that TrackRecorderService.startForegroundService() will do all the work with native part. The invariant here is straightforward: native recording is enabled when the service is active. No service means no recording.
It is too early to start it here. Please try to start TrackRecordingService in onRenderingInitializationFinished() after restoring navigation:
8d27c3250c/android/app/src/main/java/app/organicmaps/MwmActivity.java (L244-L257)
I suggest removing these listeners and calling LocationHelper directly below. Keep things simple.
nit: no reformatting
nit: no reformatting
TAG
, not"kavi"
.Please see resumeLocationInForeground() in this class.
I suggest renaming resumeLocationInForeground() to onAppForeground() and extending it instead of adding one more wrapper.
Update onCalcUpdateInterval() instead. It should take all use-cases into account, including navigation.
Please kindly check the existing logic of MwmApplication.onBackground().
8d27c3250c/android/app/src/main/java/app/organicmaps/MwmApplication.java (L354-L373)
I suggest reintegrating this part of the code into MwmApplication.onBackground() first to handle all use-cases. We will figure out which parts can be moved back to LocationHelper later.
@ -0,0 +4,4 @@
public class TrackRecorder
{
public static native void nativeSetEnabled(boolean enable);
I understand the part with native methods. However, I don't understand the purpose of this class at all. Why are all these listeners needed at all?
No hard-coded strings please.
No hard-coded strings please.
TAG
, not"kavi"
.@ -0,0 +181,4 @@
locationHelper.addListener(this);
// Restart the location with more frequent refresh interval for Track Recording.
locationHelper.restartWithNewMode();
This conflicts with the navigation service. Please think about a situation when both services are working in parallel. In this case, the lower interval should be set.
Please try to follow the existing pattern above and avoid hard-coded strings.
I suggest creating LocationHelper.onAppBackground()/onAppForeground() and call them from MwmActivity. AFAIK there are already some methonds like that. Let's keep things simple.
@ -69,4 +52,3 @@
<string-array name="mobile_data_options">
<item>@string/mobile_data_option_ask</item>
<item>@string/mobile_data_option_always</item>
What happened to these settings?
@ -69,4 +52,3 @@
<string-array name="mobile_data_options">
<item>@string/mobile_data_option_ask</item>
<item>@string/mobile_data_option_always</item>
Since on and off was implemented separately so removed it from time selection section
@ -0,0 +181,4 @@
locationHelper.addListener(this);
// Restart the location with more frequent refresh interval for Track Recording.
locationHelper.restartWithNewMode();
Yess i have to re-lookup into the whole implementation of interval and have to manage it systematically.
@ -0,0 +4,4 @@
public class TrackRecorder
{
public static native void nativeSetEnabled(boolean enable);
Actually after your suggestion to start the foreground service from mwmActivity, i created a listener class which notifies the listener in mwmActivity that track recording has started and then it starts the track recording service.
The sole purpose of the "Recent Track" feature is to display the most recent path traveled on the map, using dotted line. Is a 10-second interval sufficient for this purpose? It appears not. While the minimum value is 1 second, the optimal setting likely lies somewhere in between, perhaps around 3 seconds (TBD).
Please elaborate. The app neither collector nor share any "stats".
As I previously stated, the current feature in this PR provides the functionality to view the most recent path traveled on the map. The functionality of recording and sharing tracks with proper user controls will be addressed in the next phase. I probably agree with you that for the recording track for sharing or mapping, the best (=1 second precision) should be used.
@ -0,0 +4,4 @@
public class TrackRecorder
{
public static native void nativeSetEnabled(boolean enable);
It should be inverted... MwmActivity is the central dispatch point.
@ -69,4 +52,3 @@
<string-array name="mobile_data_options">
<item>@string/mobile_data_option_ask</item>
<item>@string/mobile_data_option_always</item>
What was wrong with using 0 as OFF?
@ -69,4 +52,3 @@
<string-array name="mobile_data_options">
<item>@string/mobile_data_option_ask</item>
<item>@string/mobile_data_option_always</item>
There are two buttons one for turning off and turning on and another to set the duration.
If "off" will be there in hour selector too then it will be a duplicate functionality.
If we implement "off" button in hour selector too then i think there will not be any need of the on/off button because there will be only one click difference.
In ios, only one setting is there which have off button as well in the hour selector
The better solution would be to add the off button in hour selector and keep it there but move the on/off button somewhere which is more easy to access for user like the bottom sheet dialog box where we are planning to place track recorder button
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.
What is about just calling onTrackRecordingStarted() ?
why the order has changed? it could be important...
nit: formatting
please don't shuffle the code
it will pollute logs, please remove after finishing debugging
it will pollute logs, please remove after finishing debugging
We can do something like that:
But I think that you will see the actual interval between messages in the logs.
@ -293,6 +294,9 @@ public class LocationHelper implements BaseLocationProvider.Listener
if (RoutingController.get().isNavigating())
return INTERVAL_NAVIGATION_MS;
!mAppInForeground doesn't matter here, just check of TrackRecorder.nativeIsEnabled()
let's not change interval depending on background/foreground. Just always use INTERVAL_TRACK_RECORDING_BACKGROUND if recording is enabled.
@ -0,0 +4,4 @@
public class TrackRecorder
{
public static native void nativeSetEnabled(boolean enable);
I don't really buy this idea with listeners in this class. Can we just check the status of Recent Track in MwmActivity.onResume() and start or stop the service? It will be 2 lines of code instead of 100.
@ -0,0 +181,4 @@
locationHelper.addListener(this);
// Restart the location with more frequent refresh interval for Track Recording.
locationHelper.restartWithNewMode();
Actually, it is no-op if the interval is the same. Even if we decide to use different intervals, Navigation will take precedence:
nit: fix identation
Can it be null? I don't see any checks in other parts:
36924897f8/android/app/src/main/java/app/organicmaps/settings/SettingsPrefsFragment.java (L116-L118)
36924897f8/android/app/src/main/java/app/organicmaps/settings/SettingsPrefsFragment.java (L116-L120)
Why is String compared with Integer? Convert newValue to Integer first.
nit: move to the next line
nit: please fix the code style here
Why does string contain " ?
" hour"
?Sorry this is a mistake i will remove it.
Actually it automatically got added to keep the space " " attached with that
I searched on internet and I found that jni have nothing to do with class type.
done
ya intervals are already their in logs no need for introducing extra intervals. I will remove that log.
@ -0,0 +53,4 @@
}
@RequiresPermission(value = ACCESS_FINE_LOCATION)
public static void startForegroundService(@NonNull Context context)
This problem is still their even after calling the requestPostNotification() method from MwmActivity, It do not asks permissions for notifications even if it disabled in setiings although functionality works fine but it do not shows any notification.
Please test this with navigation as well. @rtsisyk
actually i was doing some testing for non working of the service in background by adding extra permission of background location permission, in a hope to get it solved. So when i removed i might have missed the correct order. Sorry for that
@ -0,0 +4,4 @@
public class TrackRecorder
{
public static native void nativeSetEnabled(boolean enable);
There is one UX issue-
Suppose a users enables the recent track recorder from settings and directly exits the app without going to map screen. In that case service will never be started.
If we will place this in onResume() method, service will only turn on when mwmActivity will be resumed (i.e user will return to map screen) and same for turning it off as well.
Kavi Khalique:
It looks like that 1 vs 2 vs 3 intervals don't really matter. 10 seconds is obviously not enough, but 1-2-3 seconds don't make any difference to the battery consumption. I am in favor of enforcing setting interval = 0 for all recordings. Let's settle on 0 and move forward. No more speculations on thi topics as part of this project.
This small feature is functionally complete. Let's polish the code and merge it. The work on the full-featured track recorder will continue in new PRs.
There are two cases:
Starting/stopping the service after enabling/disabled it in the SettingsActivity.
It is safe to call TrackRecordingService.startForegroundService(this)/stopService() from SettingsActivity.
Starting the service with the app
Keep this current code that check for the settings and call TrackRecordingService.startForegroundService(this) if enabled.
This instance will organically disappear after removing listeners.
Please be careful here. It can lead into infinite loop. It would be better to not to add restartWithNewMode(); here.
@ -0,0 +4,4 @@
public class TrackRecorder
{
public static native void nativeSetEnabled(boolean enable);
Why don't just call
TrackRecordingService.startForegroundService(this)
from SettingsActivity? Why these listeners are needed?@ -0,0 +53,4 @@
}
@RequiresPermission(value = ACCESS_FINE_LOCATION)
public static void startForegroundService(@NonNull Context context)
POST_NOTIFICATIONS is available on Android 13+ (API 33+). Please see https://developer.android.com/develop/ui/views/notifications/notification-permission. Let's try to call requestPostNotificationsPermission() at least.
Do we really need these settings in Config.java? I suppose that TrackRecorder.nativeSetEnabled() already saves the setting persistently via C++ code. Please kindly take a look at organicmaps/organicmaps#1807/files. The original patch didn't have Config.java changes at all.
Could you please re-use existting strings from iOS? See organicmaps/organicmaps#1807/files
tags = ios
totags = ios,android
to in strings.txtPlease refer to https://github.com/organicmaps/organicmaps/blob/master/docs/TRANSLATIONS.md for more details.
Yes i was about to ask you the same. I think it can be removed its redundant.
Will remove it 👍
@ -0,0 +4,4 @@
public class TrackRecorder
{
public static native void nativeSetEnabled(boolean enable);
Since settings has been removed so all these are also gone : )
@ -0,0 +53,4 @@
}
@RequiresPermission(value = ACCESS_FINE_LOCATION)
public static void startForegroundService(@NonNull Context context)
Please call requestPostNotificationsPermission() as well as check for location!
This PR generally looks good. Let's merge organicmaps/organicmaps#8399 first and then rebase this one.
Don't we need to set OFF icon here also?
Please rework to use strings.txt. https://github.com/organicmaps/organicmaps/blob/master/docs/TRANSLATIONS.md#translation-files
https://stackoverflow.com/questions/43123650/android-request-access-notification-policy-and-mute-phone
@ -1918,10 +1930,16 @@ public class MwmActivity extends BaseMwmFragmentActivity
Logger.w(LOCATION_TAG, "Permission " + permission + " has been refused");
}
space after if here and in other places.
Let's call it startTrackRecording to avoid renaming later.
Don't use one-line ifs, they're harder to debug.
@ -0,0 +161,4 @@
startForeground(TrackRecordingService.TRACK_REC_NOTIFICATION_ID, getNotificationBuilder(this).build());
} catch (ForegroundServiceStartNotAllowedException e)
{
Logger.e(TAG, "Oops! ForegroundService is not allowed", e);
Should user see some UI explanation of why it doesn't start?
?
@ -0,0 +161,4 @@
startForeground(TrackRecordingService.TRACK_REC_NOTIFICATION_ID, getNotificationBuilder(this).build());
} catch (ForegroundServiceStartNotAllowedException e)
{
Logger.e(TAG, "Oops! ForegroundService is not allowed", e);
This case never happens.
bafee9fa0b/android/app/src/main/java/app/organicmaps/routing/NavigationService.java (L227-L236)
All these flags are subject to replacement with enums to differentiate between navigation/recording actions when requesting permissions.
@ -207,3 +209,4 @@
private boolean mLocationPermissionRequestedForRecording = false;
@SuppressWarnings("NotNullFieldNotInitialized")
@NonNull
It is hard to explain, but this code differs from the branch I've pushed yesterday. Something is wrong with GitHub.
using this permission we can show notifications on lock screen as well
Its implementation was already there but since this permission was not there thats why notifications for navigations and trace path were not shown on lockscreen.
this log got pushed by mistake : )
The logic is it only shows on icon if feature is turned on in all other cases it shows off icon.
So we dont actually have to explicitly set it to off.
What is the preferred way then?
What breaks if we remove this permission?
Thanks, looks good in general. Shall we test it in alpha/beta first before the merge?
OSMand, for example, does not declare this permission. Is it really needed?
Is a coarse permission check required? Or only a fine permission check can be done?
Is badge count -1 a valid value?
nit
@ -0,0 +55,4 @@
@RequiresPermission(value = ACCESS_FINE_LOCATION)
public static void startForegroundService(@NonNull Context context)
{
if (TrackRecorder.nativeGetDuration() != 24)
nit: introduce a named constant to better understand what 24 means.
@ -196,2 +223,4 @@
}
@OptIn(markerClass = com.google.android.material.badge.ExperimentalBadgeUtils.class)
public void updateMenuBadge()
What are the pros of using experimental code?
@ -206,2 +235,3 @@
final int verticalOffset = dpToPx(8, context) + dpToPx(Integer.toString(count).length() * 5, context);
final int verticalOffset = dpToPx(8, context) + dpToPx(Integer.toString(0)
.length() * 5, context);
BadgeUtils.detachBadgeDrawable(mBadgeDrawable, menuButton);
nit
nit here and below: space after if
Recording the track
to avoid renaming it later for phase 2?Probably it would be better not to touch the code that don't need to be changed.
@ -196,2 +223,4 @@
}
@OptIn(markerClass = com.google.android.material.badge.ExperimentalBadgeUtils.class)
public void updateMenuBadge()
@kavikhalique , is this experimental stuff needed at all?
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.
checking both would be convenient i guess. I have copied it from the implementation of navigation system since it is already implemented and tested.
In my xiaomi device when i use this permission it shows notifications of OM on lockscreen and without this permission it do not shows anything.
But i tested on realme and motorola devices and nothing actually happens with and without this permission. Notification do not appears on lock screen in any case.
I feel this permission can be removed.
@ -196,2 +223,4 @@
}
@OptIn(markerClass = com.google.android.material.badge.ExperimentalBadgeUtils.class)
public void updateMenuBadge()
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.
Pleaase fix a crash in updateMenuBadge().
@ -206,2 +235,3 @@
final int verticalOffset = dpToPx(8, context) + dpToPx(Integer.toString(count).length() * 5, context);
final int verticalOffset = dpToPx(8, context) + dpToPx(Integer.toString(0)
.length() * 5, context);
BadgeUtils.detachBadgeDrawable(mBadgeDrawable, menuButton);
This function crashes on attempt to make a route: