-
-
Notifications
You must be signed in to change notification settings - Fork 923
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
[android] Trace Path (a.k.a. Recent Track) #8183
Conversation
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. |
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:
|
android/app/src/main/java/app/organicmaps/location/TrackRecordingService.java
Show resolved
Hide resolved
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. |
Vendor: Samsung 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. |
android/app/src/main/java/app/organicmaps/util/AppStateListener.java
Outdated
Show resolved
Hide resolved
android/app/src/main/java/app/organicmaps/settings/SettingsPrefsFragment.java
Outdated
Show resolved
Hide resolved
android/app/src/main/java/app/organicmaps/location/TrackRecordingService.java
Outdated
Show resolved
Hide resolved
android/app/src/main/java/app/organicmaps/location/TrackRecordingService.java
Outdated
Show resolved
Hide resolved
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.
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.
android/app/src/main/java/app/organicmaps/settings/SettingsPrefsFragment.java
Outdated
Show resolved
Hide resolved
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
android/app/src/main/java/app/organicmaps/location/TrackRecordingService.java
Show resolved
Hide resolved
Pending tasks
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. |
Why toast |
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. |
✅ blue dot in bottom sheet when feature is enabled Pending
@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 ) |
33b7be0
to
4842537
Compare
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. |
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. |
Récent parcours is really not easy to understood Toast continue to be showed when I leave settings
|
Could you please suggest a better name? I intentionally didn't touch translations of
Fixed. Thanks! |
4842537
to
cf0fc09
Compare
|
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.
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" /> |
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.
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 }); |
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.
Is a coarse permission check required? Or only a fine permission check can be done?
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.
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)); |
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.
Is badge count -1 a valid 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.
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"); |
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
Logger.i(TAG, "Location helper knows app came in foreground"); | |
Logger.i(TAG, "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.
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) |
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: introduce a named constant to better understand what 24 means.
final int verticalOffset = dpToPx(8, context) + dpToPx(Integer.toString(0) | ||
.length() * 5, 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.
nit
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); |
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 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) |
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.
What are the pros of using experimental code?
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.
@kavikhalique , is this experimental stuff needed 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.
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)) |
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 here and below: space after if
data/strings/strings.txt
Outdated
[recording_location] | ||
comment = Shown as toast when starting the recent track recording | ||
tags = android | ||
en = Recording the location |
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.
Recording the track
to avoid renaming it later for phase 2?
We will add Hindi and English the next time and ask you to help with other translations. |
I suggest in French |
4610714
to
82adc52
Compare
Update the PR to remove dead code from Utils.java and actualize French translations. |
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.
Pleaase fix a crash in updateMenuBadge().
final int verticalOffset = dpToPx(8, context) + dpToPx(Integer.toString(0) | ||
.length() * 5, 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 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)
Thanks for the report ) |
Why Android continues to use recent path translations in menu? |
I have not manipulated those strings probably @rtsisyk has changed them : ) |
All the requested changes are done except the content for dialog box. Should work upon it? |
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]>
0b14b78
to
c812bc0
Compare
[Android] Implements Recent Track Recording in android app of OM