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] Request to disable power saving mode and Battery optimisation before Navigation #8399

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kavikhalique
Copy link
Contributor

@kavikhalique kavikhalique commented Jun 6, 2024

#8336
Implements functionality to request users to disable Power saving mode and battery optimisation if it is enabled before the start of the navigation.

It is a cold request type i.e even if user denies the process continues.

Discussion Required -

  • Added a listener as well which listens to the state change of power saving mode, but haven't implemented the actions which should be taken on changes.
  • Should the same kind of listener be implemented for battery optimisation state as well?

@@ -2004,6 +2064,61 @@ public void onSearchRoutePoint(@RoutePointInfo.RouteMarkType int pointType)
@Override
public void onRoutingStart()
{
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M)
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to extract this section into a separate function.

Copy link
Contributor

Choose a reason for hiding this comment

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

Motivation: it will be called from Trace Path feature.

}

// Check for device battery saver mode
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP)
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to extract this section into a separate function.

private final ArrayList<String> POWER_SAVE_MODE_CHANGE_ACTIONS = new ArrayList<String>()
{
{
add("huawei.intent.action.POWER_MODE_CHANGED_ACTION");
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just use static final String HUAWEI_POWER_MODE_INTENT instead?

@@ -2200,4 +2200,12 @@
<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="battery_optimisation_dialog_title">Stop optimizing battery usage?</string>
Copy link
Contributor

Choose a reason for hiding this comment

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

This file is auto-regenerated from https://github.com/organicmaps/organicmaps/blob/master/data/strings/strings.txt. Please take a look at https://github.com/organicmaps/organicmaps/blob/master/docs/TRANSLATIONS.md#translations. Please complain in https://github.com/orgs/organicmaps/discussions/4515 if you think switching to native XML translations would be better.

Let's keep it as is until implemention is finalized. I will help you with strings.txt and translations later.


private boolean isPowerSaveModeCompat(Context context)
{
for (String name : POWER_SAVE_MODE_SETTINGS_NAMES)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it check for the vendor instead?

switch(Build.MANUFACTURER.toUpperCase(Locale.getDefault()))
case "HUAWEI" -> // huawei-specific stuff.
case "SAMSUNG" -> // samsung-specific stuff.
...

}

Copy link
Member

Choose a reason for hiding this comment

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

Are manufacturers guaranteed to be equal to these short strings? Would it be safer to check if a string contains the keyword?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are manufacturers guaranteed to be equal to these short strings? Would it be safer to check if a string contains the keyword?

For huawei and xiaomi i guess there is no issue. It will always be this.

{
if (POWER_SAVE_MODE_VALUES.containsKey(Build.MANUFACTURER.toUpperCase(Locale.getDefault())))
{
context.registerReceiver(new BroadcastReceiver()
Copy link
Contributor

Choose a reason for hiding this comment

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

Where this BroadcastReceiver is unregistered? It probably will leak MwmActivity class because of using such closure.

Do we need this event at all? Isn't it enough to check for the power save when routing is started? What do we want to do on this POWER_SAVE_MODE_CHANGED event? I found only one usage where the log message is printed. Probably it is not a big deal to remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes for now its of no use. should i remove the implementation of the same from PowerManagerCompat class? All the broadcast receivers and everything except the method for checking the status?

Copy link
Contributor

Choose a reason for hiding this comment

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

This version gets crashed in the background periodically:

    Fatal Exception: java.lang.RuntimeException: Unable to destroy activity {app.organicmaps.beta/app.organicmaps.MwmActivity}: java.lang.IllegalArgumentException: Receiver not registered: app.organicmaps.util.PowerManagerCompat$7@dc149b8
       at android.app.ActivityThread.performDestroyActivity(ActivityThread.java:5827)
       at android.app.ActivityThread.handleDestroyActivity(ActivityThread.java:5859)
       at android.app.servertransaction.DestroyActivityItem.execute(DestroyActivityItem.java:47)
       at android.app.servertransaction.ActivityTransactionItem.execute(ActivityTransactionItem.java:60)
       at android.app.servertransaction.TransactionExecutor.executeLifecycleItem(TransactionExecutor.java:254)
       at android.app.servertransaction.TransactionExecutor.executeLifecycleState(TransactionExecutor.java:228)
       at android.app.servertransaction.TransactionExecutor.execute(TransactionExecutor.java:91)
       at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2544)
       at android.os.Handler.dispatchMessage(Handler.java:107)
       at android.os.Looper.loopOnce(Looper.java:232)
       at android.os.Looper.loop(Looper.java:317)
       at android.app.ActivityThread.main(ActivityThread.java:8501)
       at java.lang.reflect.Method.invoke(Method.java)
       at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:552)
       at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:878)
        
          Caused by java.lang.IllegalArgumentException: Receiver not registered: app.organicmaps.util.PowerManagerCompat$7@dc149b8
       at android.app.LoadedApk.forgetReceiverDispatcher(LoadedApk.java:1670)
       at android.app.ContextImpl.unregisterReceiver(ContextImpl.java:1873)
       at android.content.ContextWrapper.unregisterReceiver(ContextWrapper.java:821)
       at android.content.ContextWrapper.unregisterReceiver(ContextWrapper.java:821)
       at app.organicmaps.util.PowerManagerCompat.unregister(PowerManagerCompat.java:75)
       at app.organicmaps.MwmActivity.onSafeDestroy(MwmActivity.java:1185)
       at app.organicmaps.base.BaseMwmFragmentActivity.onDestroy(BaseMwmFragmentActivity.java:105)
       at android.app.Activity.performDestroy(Activity.java:9048)
       at android.app.Instrumentation.callActivityOnDestroy(Instrumentation.java:1554)
       at android.app.ActivityThread.performDestroyActivity(ActivityThread.java:5814)
       at android.app.ActivityThread.handleDestroyActivity(ActivityThread.java:5859)
       at android.app.servertransaction.DestroyActivityItem.execute(DestroyActivityItem.java:47)
       at android.app.servertransaction.ActivityTransactionItem.execute(ActivityTransactionItem.java:60)
       at android.app.servertransaction.TransactionExecutor.executeLifecycleItem(TransactionExecutor.java:254)
       at android.app.servertransaction.TransactionExecutor.executeLifecycleState(TransactionExecutor.java:228)
       at android.app.servertransaction.TransactionExecutor.execute(TransactionExecutor.java:91)
       at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2544)
       at android.os.Handler.dispatchMessage(Handler.java:107)
       at android.os.Looper.loopOnce(Looper.java:232)
       at android.os.Looper.loop(Looper.java:317)
       at android.app.ActivityThread.main(ActivityThread.java:8501)
       at java.lang.reflect.Method.invoke(Method.java)
       at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:552)
       at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:878)

It is probably caused by this BroadcastReceiver.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback. This issue is probably because broadcast reciever unregister is called when no broadcast receiver was registered. I guess i placed unregister call in wrong method of mwmActivty.

I will check it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue was with unregister method actually i was not unregistering the broadcast reciever properly. Now i have created a variable and saved the instance into it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really understand the need for this entire PowerManagerCompat.

mDeviceBatterySaverPermission.launch(intent);
});
}
builder.show();
Copy link
Contributor

Choose a reason for hiding this comment

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

Please save the instance into mAlertDialog. Otherwise, dismissAlertDialog() will not work. There was a bug that rotation of the screen causes a crash if mAlertDialog is on the screen.

Suggested change
builder.show();
mAlertDialog = builder.show();

Logger.w(TAG, "Checking for App battery optimisation permission");
mNavigationBatterySaverPermission = true;
dismissAlertDialog();
final MaterialAlertDialogBuilder builder = new MaterialAlertDialogBuilder(this, R.style.MwmTheme_AlertDialog)
Copy link
Contributor

Choose a reason for hiding this comment

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

This dialog is displayed every time even if the use pressed DENY. Let's be less annoying and save the rejection resolution into Config to avoid showing this dialog one more time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. if user denies, in which condition we gonna again ask this permission?

  2. if user allowed and after navigation started he reverted the permissions. Will we be asking him instantly or on every new navigation start?

Copy link
Member

Choose a reason for hiding this comment

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

@kavikhalique How do other apps behave in such situations?

Copy link
Contributor Author

@kavikhalique kavikhalique Jun 9, 2024

Choose a reason for hiding this comment

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

I tested starva and mapmyride

  1. For battery optimisation and battery saver they asked only on first start after that they didn't asked even if the feature is turned on.

  2. For location permission they ask every time if it is disabled

  3. For post notification permission i am not sure because i do not have a device which have android 13 : )

But here the issue is if power saving mode is turned on we don't get satellite fix that means no location update at all which makes trace path feature useless.

I do not have proper knowledge how these apps deal with this problem in power saving mode but probably since they are not offline apps they may acquire location from other sources (network, wifi etc). But this might not be case for us since many users might be using it because of its total offline functionality.

.setNegativeButton(R.string.deny, (dialog, which) -> onRoutingStart())
.setOnDismissListener(dialog -> mAlertDialog = null);

final Intent intent = Utils.makeAppBatteryOptimizationIntent(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please verify that MANUFACTURER is supported first before showing the dialog. It is just useless without intent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wouldn't it be beneficial if we inform the user to disable it atleast? I guess atleast informing them would be the better option even if we cant route them there to turn it off.

android/app/src/main/java/app/organicmaps/MwmActivity.java Outdated Show resolved Hide resolved
else
Logger.w(TAG, "Power Save mode is disabled on the device");

if (mNavigationDeviceBatterySaverPermission)
Copy link
Contributor

Choose a reason for hiding this comment

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

For the Trace Path we will need to distinguish between Routing vs Trace Path case. Please consider adding an enum instead of boolean flag.

Copy link
Contributor

Choose a reason for hiding this comment

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

How can I check what intent is needed on each particular device? I have Samsung, Huawei, vanilla Android.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used an app called "Activity Launcher" to get the package name and activity name to start any activity from our app. I used it to start battery saver activity of xiaomi device (haven't pushed i yet).

you can download it from play store in your device and lookout for the required activity and there you will get package name and activity name to start it from your app : )

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean that you just check the list of available activities of the Settings app?

Copy link
Contributor

Choose a reason for hiding this comment

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

Here is what I can see on my Samsung when MapMyRide opens "Power saving" screen:

06-08 08:49:38.566 976 3537 I ActivityTaskManager: START u0 {act=android.settings.BATTERY_SAVER_SETTINGS cmp=com.android.settings/.Settings$BatterySaverSettingsActivity} from uid 10334
06-08 08:49:38.567 976 3537 D ActivityTaskManager: TaskLaunchParamsModifier:task=null activity=ActivityRecord{ac371b u0 com.android.settings/.Settings$BatterySaverSettingsActivity display-area-from-source=DefaultTaskDisplayArea@93219380 display-id=0 display-windowing-mode=1 suggested-display-area=DefaultTaskDisplayArea@93219380
06-08 08:49:38.567 976 3537 D ActivityTaskManager: TaskLaunchParamsModifier:task=Task{13fba47 #811 type=standard A=10334:com.mapmyride.android2 U=0 visible=true mode=fullscreen translucent=false sz=1} activity=ActivityRecord{ac371b u0 com.android.settings/.Settings$BatterySaverSettingsActivity t-1} display-area-from-source=DefaultTaskDisplayArea@93219380 display-id=0 display-windowing-mode=1 suggested-display-area=DefaultTaskDisplayArea@93219380 inherit-from-source=fullscreen activity-options-fullscreen=Rect(0, 0 - 0, 0) non-freeform-display display-area=DefaultTaskDisplayArea@93219380 maximized-bounds
06-08 08:49:38.569 626 656 I SurfaceFlinger: id=279 createSurf (0x0),-1 flag=80004, ActivityRecord{ac371b u0 com.android.settings/.Settings$BatterySaverSettingsActivity t811}#0

I guess that it is "act=android.settings.BATTERY_SAVER_SETTINGS"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Android provides an intent action to access those activities but in few chinese vendors (i.e huawei, xiaomi) they do not use it. I searched for it on internet and got solution on stack overflow : )

For any other devices like samsung and all a default intent action provided by android is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean that you just check the list of available activities of the Settings app?

I did that to open battery saver activity from the app for xiaomi devices because from android provided intent action it was not starting (not pushed yet)

Copy link
Contributor Author

@kavikhalique kavikhalique Jun 9, 2024

Choose a reason for hiding this comment

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

Here is what I can see on my Samsung when MapMyRide opens "Power saving" screen:
I guess that it is "act=android.settings.BATTERY_SAVER_SETTINGS"

Is default intent not working on your samsung device to open power saver setting?
The intent action which you provided is already there in code it it the default one.

public static final String ACTION_BATTERY_SAVER_SETTINGS
            = "android.settings.BATTERY_SAVER_SETTINGS";

This is declared in android Settings file.

@kavikhalique
Copy link
Contributor Author

All changes done. Please have a look @rtsisyk

String generation is pending and content for dialog box is required.

Copy link
Contributor

@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.

Please see minified version in #8499


private PowerManager mPowerManager;

private RequestedBy currRequest;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make it less generic. Please also use m prefix for members.

Suggested change
private RequestedBy currRequest;
@Nullable
private BatterySaverRequestedBy mBatterySaverRequestedBy;

@@ -225,6 +237,10 @@ public interface LeftAnimationTrackListener
void onTrackLeftAnimation(float offset);
}

public enum RequestedBy
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use less generic name.

Suggested change
public enum RequestedBy
public enum BatterySaverRequestedBy

@SuppressWarnings("NotNullFieldNotInitialized")
@NonNull
private DisplayManager mDisplayManager;

private boolean mRemoveDisplayListener = true;
private int mLastUiMode = Configuration.UI_MODE_TYPE_UNDEFINED;

private PowerManagerCompat mPowerManagerCompat;

private PowerManager mPowerManager;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private PowerManager mPowerManager;
@SuppressWarnings("NotNullFieldNotInitialized")
@NonNull
private PowerManager mPowerManager;

@@ -209,13 +211,23 @@ public class MwmActivity extends BaseMwmFragmentActivity
@NonNull
private ActivityResultLauncher<IntentSenderRequest> mLocationResolutionRequest;

private ActivityResultLauncher<Intent> mBatterySaverPermission;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private ActivityResultLauncher<Intent> mBatterySaverPermission;
@Nullable
private ActivityResultLauncher<Intent> mBatterySaverPermission;

@@ -225,6 +237,10 @@ public interface LeftAnimationTrackListener
void onTrackLeftAnimation(float offset);
}

public enum RequestedBy
{
NAVIGATION
Copy link
Contributor

Choose a reason for hiding this comment

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

Please see public @interface ChoosePositionMode

return true;

currRequest = requestedBy;
Config.setNeedToReuestBatterySaverPermission(requestedBy, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be after getting the negative result of the dialog. Currently, we don't show the dialog at all if the device wasn't in powersave mode on the first call.

mAlertDialog = builder.show();
}
}
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return false;
return true;

mBatterySaverPermission.launch(intent);
});
}
mAlertDialog = builder.show();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
mAlertDialog = builder.show();
mAlertDialog = builder.show();
return false;

mAlertDialog = builder.show();
}
}
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return false;
return true;

mDeviceBatterySaverPermission.launch(intent);
});
}
mAlertDialog = builder.show();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
mAlertDialog = builder.show();
currRequest = requestedBy;
mAlertDialog = builder.show();
return false;

@rtsisyk
Copy link
Contributor

rtsisyk commented Jun 20, 2024

Originally posted by @rtsisyk in #8499 (comment)

Unfortunally, ACTION_REQUEST_IGNORE_BATTERY_OPTIMIZATIONS added by this PR doesn't help on Samsung. GPS signal is getting lost in "Power saving" mode no matter of "Unrestricted" baterry setting for app. The only solution that works is disabling the "Power saving" mode on the device.

What is about Xiaomi, Huawei and others? What information do we have?

@kavikhalique
Copy link
Contributor Author

Unfortunally, ACTION_REQUEST_IGNORE_BATTERY_OPTIMIZATIONS added by this PR doesn't help on Samsung. GPS signal is getting lost in "Power saving" mode no matter of "Unrestricted" baterry setting for app. The only solution that works is disabling the "Power saving" mode on the device.

Yes, I have stated this in my past comments too, actually system is not hindering our app to access gps locations but it hinders the gnss hardware's power supply in power saving mode which results in not fixing of satellite.

Power optimisation was just an additional feature, it was not going to help in satellite fix in any ways.

@kavikhalique
Copy link
Contributor Author

Our main focus is disabling power saving mode or at least warning the user to disable it otherwise they will not be able to record properly.

@rtsisyk
Copy link
Contributor

rtsisyk commented Jun 20, 2024

Our main focus is disabling power saving mode or at least warning the user to disable it otherwise they will not be able to record properly.

Yes. Let's remove ACTION_REQUEST_IGNORE_BATTERY_OPTIMIZATIONS and focus on detecting power-save mode and opening system settings to disable it.

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

Successfully merging this pull request may close these issues.

None yet

3 participants