-
-
Notifications
You must be signed in to change notification settings - Fork 855
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] Reimplement "Recent Track" feature #8183
base: master
Are you sure you want to change the base?
[android] Reimplement "Recent Track" feature #8183
Conversation
Signed-off-by: kavikhalique <[email protected]>
Signed-off-by: kavikhalique <[email protected]>
android/app/src/main/java/app/organicmaps/location/LocationHelper.java
Outdated
Show resolved
Hide resolved
android/app/src/main/java/app/organicmaps/location/LocationHelper.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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.
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
|
Update summary of preference with value selected by the user |
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. Next time I will compare it with my Garmin Edge. Visually it was nice, but I lost proofs ( Screenrecorder-2024-05-15-18-44-27-816.mp4 |
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. )
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. |
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.
It should be noted that the Debug build is much slower than the release build and probably consumes more battery |
Thanks for the feedback :)
Will look into this issue 👍
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. |
Signed-off-by: kavikhalique <[email protected]>
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. 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...) |
Feedback by @RicoElectrico from Telegram chat:
|
return null; | ||
} | ||
@RequiresPermission(value = ACCESS_FINE_LOCATION) | ||
public static void startForegroundService(@NonNull Context context) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
organicmaps/android/app/src/main/java/app/organicmaps/MwmActivity.java
Lines 1843 to 1854 in a7127cc
public void requestPostNotificationsPermission() | |
{ | |
if (Build.VERSION.SDK_INT < Build.VERSION_CODES.TIRAMISU || | |
ActivityCompat.checkSelfPermission(this, POST_NOTIFICATIONS) == PERMISSION_GRANTED) | |
{ | |
Logger.i(TAG, "Permissions POST_NOTIFICATIONS is granted"); | |
return; | |
} | |
Logger.i(TAG, "Requesting POST_NOTIFICATIONS permission"); | |
mPostNotificationPermissionRequest.launch(POST_NOTIFICATIONS); | |
} |
UPDATE: It has started to work after enabling notification in system settings:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
6de4a18 (HEAD -> recent-track-recorder)
Author: kavikhalique [email protected]
Date: Fri May 17 02:45:21 2024 +0530
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please call requestPostNotificationsPermission() as well as check for location!
Is it possible to check if notifications are disabled by the user and ask to enable them? Would it be helpful? |
Thanks for the feedback 👍 Accuracy seems low in comparison to Garmin. Can you please provide battery drain if by chance you remember it. |
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. |
Signed-off-by: kavikhalique <[email protected]>
Vendor: Samsung Result: no points recorded when app is in the background |
Signed-off-by: kavikhalique <[email protected]>
There was a problem hiding this 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.
{ | ||
mListeners.unregister(listener); | ||
} | ||
public static native void nativeSetEnabled(boolean enable); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
public static void setRecentTrackRecorderDuration(int value) | ||
{ | ||
nativeSetInt(KEY_RECENT_TRACK_RECORDER_DURATION, value); | ||
if(value!=0) TrackRecorder.nativeSetDuration(value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: please fix the code style here
@@ -2194,4 +2194,9 @@ | |||
<string name="type.amenity.events_venue">Events Venue</string> | |||
<string name="type.shop.auction">Auction</string> | |||
<string name="type.shop.collector">Collectables</string> | |||
<string name="hour_recent_track">" hour"</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does string contain " ?
" hour"
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry this is a mistake i will remove it.
Actually it automatically got added to keep the space " " attached with that
android/app/src/main/java/app/organicmaps/settings/SettingsPrefsFragment.java
Outdated
Show resolved
Hide resolved
@@ -292,6 +297,8 @@ private long calcLocationUpdatesInterval() | |||
{ | |||
if (RoutingController.get().isNavigating()) | |||
return INTERVAL_NAVIGATION_MS; | |||
if (!mAppInForeground && TrackRecorder.nativeIsEnabled()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!mAppInForeground doesn't matter here, just check of TrackRecorder.nativeIsEnabled()
if (isActive()) | ||
return; | ||
{ | ||
if (LocationUtils.checkLocationPermission(mContext)) restartWithNewMode(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's not change interval depending on background/foreground. Just always use INTERVAL_TRACK_RECORDING_BACKGROUND if recording is enabled.
@@ -365,6 +364,9 @@ else if (RoutingController.get().isNavigating()) | |||
Logger.i(LOCATION_TAG, "Navigation is in progress, keeping location in the background"); | |||
else if (!Map.isEngineCreated() || LocationState.getMode() == LocationState.PENDING_POSITION) | |||
Logger.i(LOCATION_TAG, "PENDING_POSITION mode, keeping location in the background"); | |||
else if(TrackRecorder.nativeIsEnabled()){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: formatting
@@ -345,18 +346,16 @@ public void onActivityDestroyed(@NonNull Activity activity) | |||
private void onForeground() | |||
{ | |||
Logger.d(TAG); | |||
|
|||
mLocationHelper.onAppForeground(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the order has changed? it could be important...
android/app/src/main/java/app/organicmaps/location/TrackRecordingService.java
Show resolved
Hide resolved
Signed-off-by: kavikhalique <[email protected]>
Signed-off-by: kavikhalique <[email protected]>
Signed-off-by: kavikhalique <[email protected]>
Pedestrian 5 km/h: ✅ Huawei P30 / Android 12 / Battery > 85% / No power safe / Airplane / Google Location (no Internet) All devices were in the same backpack on my back. Traces are almost identical when outdoors. Vehicle 20-30 km/h: ✅ Huawei P30 / Android 12 / Battery > 85% / No power safe / Airplane / Google Location (no Internet) All devices were in the same backpack on the front seat. It is surprising, but only Huawei had working GPS. Two other devices had something like that:
So far, I haven't found any issues related to the Recent Track itself. Location was propagated to the track recorder on every update:
I am waiting for the full (-A)GPS re-sync from wi-fi and sky and re-testing again. |
@rtsisyk Thanks for the detailed feedback : ) |
GPS gaps on Pixel 4a are caused by broken device / firmware: #8289 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
public static int getRecentTrackRecorderDuration() | ||
{ | ||
return nativeGetInt(KEY_RECENT_TRACK_RECORDER_DURATION, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 https://github.com/organicmaps/organicmaps/pull/1807/files#diff-4a1a76e5c1d535b59bc9159048bd2c941dc112f5052e4e8ec650756c1ed0eed0R594-R642. The original patch didn't have Config.java changes at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes i was about to ask you the same. I think it can be removed its redundant.
Will remove it 👍
@@ -220,6 +220,10 @@ | |||
<string name="prefs_group_route">Navigation</string> | |||
<string name="pref_zoom_title">Zoom buttons</string> | |||
<string name="pref_zoom_summary">Display on the map</string> | |||
<!--Title of Recent track Recorder in settings--> | |||
<string name="recent_track">Recent Track</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please re-use existting strings from iOS? See https://github.com/organicmaps/organicmaps/pull/1807/files#diff-8e9891f34f779c3f69c75ce68b56207e638f199a26123422c079734a32496c7cR4138
- Change
tags = ios
totags = ios,android
to in strings.txt - Run ./tools/unix/generate_localizations.sh to regenerate values-*/strings.xml
Please refer to https://github.com/organicmaps/organicmaps/blob/master/docs/TRANSLATIONS.md for more details.
@@ -333,7 +337,8 @@ public void start() | |||
{ | |||
if (isActive()) | |||
{ | |||
Logger.d(TAG, "Already started"); | |||
restartWithNewMode(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please be careful here. It can lead into infinite loop. It would be better to not to add restartWithNewMode(); here.
{ | ||
mListeners.unregister(listener); | ||
} | ||
public static native void nativeSetEnabled(boolean enable); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Why don't just call TrackRecordingService.startForegroundService(this)
from SettingsActivity? Why these listeners are needed?
Logger.i(TAG, "Location permission is not there"); | ||
return; | ||
} | ||
TrackRecordingService.startForegroundService(this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -532,6 +556,10 @@ protected void onSafeCreate(@Nullable Bundle savedInstanceState) | |||
initViews(isLaunchByDeepLink); | |||
updateViewsInsets(); | |||
|
|||
mTrackRecorder = TrackRecorder.getInstance(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This instance will organically disappear after removing listeners.
return null; | ||
} | ||
@RequiresPermission(value = ACCESS_FINE_LOCATION) | ||
public static void startForegroundService(@NonNull Context context) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.
We have been discussion bike sheds for a while. It is time to make decisions. I am proposing the following plan:
|
What's the benefit of throwing out all discussion arguments and pushing something questionable? I have several questions:
|
Naming has been discussed in another thread: #6678. Honestly, any decision here is better than no decision. I have tossed my coin already. You don't need to agree with the outcome.
Bike shed? We will change the constant in the code if it doesn't work well.
This feature implies significantly battery consumption. I think that it makes sense to notify the user at least once. |
It is not the first time I have heard this hypothesis. Are there any facts confirming it? What is significantly and insignificantly? |
Few days back i had done a testing for about 13 hours approx. Please have a look into the testing data below
|
Signed-off-by: kavikhalique <[email protected]>
✅ Add "Trace Path" button to main menu (placeholder icon used) @rtsisyk i have finished all this above mentioned. Please review it. Few small works are pending which i will finish in next few commits probably. |
Before start record path, we need to check location is enabled. |
@kavikhalique comparing the state when an app is not used to the state when the app is used doesn't reflect reality well. Can you please compare the running track recorder with the running navigation feature? Is there a warning about battery usage when users start navigation in Organic Maps and other maps apps? |
Absolutely nothing happens when I press "OK" in the dialog. Please add POST_NOTIFICATION check finally! |
|
But there already we are changing the color of the icon.
Sure will do that 👍 |
It is really hard to say when it is enabled and it is not... Let's fix the notification first and I will re-test again. |
I've moved GPS + Power Save research into a separate ticket #8336 |
Signed-off-by: kavikhalique <[email protected]>
✅ Toast message on start and stop
|
Signed-off-by: kavikhalique <[email protected]>
✅ Request user to disable battery optimisation on app in case it is enabled |
[Android] Implements Recent Track Recording in android app of OM