-
Notifications
You must be signed in to change notification settings - Fork 6k
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
Added battery not low and storage not low as download requirements #7138
base: dev-v2
Are you sure you want to change the base?
Changes from 2 commits
9fbd399
36d11e5
6ddd904
56fd460
4299342
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,7 +45,7 @@ public final class Requirements implements Parcelable { | |
@Retention(RetentionPolicy.SOURCE) | ||
@IntDef( | ||
flag = true, | ||
value = {NETWORK, NETWORK_UNMETERED, DEVICE_IDLE, DEVICE_CHARGING}) | ||
value = {NETWORK, NETWORK_UNMETERED, DEVICE_IDLE, DEVICE_CHARGING, DEVICE_BATTERY_NOT_LOW, DEVICE_STORAGE_NOT_LOW}) | ||
public @interface RequirementFlags {} | ||
|
||
/** Requirement that the device has network connectivity. */ | ||
|
@@ -56,6 +56,16 @@ public final class Requirements implements Parcelable { | |
public static final int DEVICE_IDLE = 1 << 2; | ||
/** Requirement that the device is charging. */ | ||
public static final int DEVICE_CHARGING = 1 << 3; | ||
/** Requirement that the storage is not low. */ | ||
public static final int DEVICE_STORAGE_NOT_LOW = 1 << 4; | ||
/** Requirement that the battery is not low. */ | ||
public static final int DEVICE_BATTERY_NOT_LOW = 1 << 5; | ||
|
||
/** Constant indicating the battery is not plugged in a power source */ | ||
private static final int BATTERY_PLUGGED_NONE = 0; | ||
/** Constant when the battery is considered low (in percentage) */ | ||
private static final float BATTERY_LOW_PERCENTAGE = 0.15f; | ||
|
||
|
||
@RequirementFlags private final int requirements; | ||
|
||
|
@@ -94,6 +104,14 @@ public boolean isIdleRequired() { | |
return (requirements & DEVICE_IDLE) != 0; | ||
} | ||
|
||
public boolean isStorageNotLowRequired() { | ||
return (requirements & DEVICE_STORAGE_NOT_LOW) != 0; | ||
} | ||
|
||
public boolean isBatteryNotLowRequired() { | ||
return (requirements & DEVICE_BATTERY_NOT_LOW) != 0; | ||
} | ||
|
||
/** | ||
* Returns whether the requirements are met. | ||
* | ||
|
@@ -119,6 +137,12 @@ public int getNotMetRequirements(Context context) { | |
if (isIdleRequired() && !isDeviceIdle(context)) { | ||
notMetRequirements |= DEVICE_IDLE; | ||
} | ||
if (isBatteryNotLowRequired() && !isBatteryNotLow(context)) { | ||
notMetRequirements |= DEVICE_BATTERY_NOT_LOW; | ||
} | ||
if (isStorageNotLowRequired() && !isStorageNotLow(context)) { | ||
notMetRequirements |= DEVICE_STORAGE_NOT_LOW; | ||
} | ||
return notMetRequirements; | ||
} | ||
|
||
|
@@ -162,6 +186,54 @@ private boolean isDeviceIdle(Context context) { | |
: Util.SDK_INT >= 20 ? !powerManager.isInteractive() : !powerManager.isScreenOn(); | ||
} | ||
|
||
/** | ||
* Implementation taken from the the WorkManager source. | ||
* @see <a href="https://android.googlesource.com/platform/frameworks/support/+/androidx-master-dev/work/workmanager/src/main/java/androidx/work/impl/constraints/trackers/BatteryNotLowTracker.java">BatteryNotLowTracker</a> | ||
*/ | ||
private boolean isBatteryNotLow(Context context) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The extent this is decoupled from How thoroughly did you test this, and are you certain it's always going to work? Is it possible to use As a more general point, I think we should probably move evaluation of the requirements out of Depending on how confident you are about the battery part of this change, I see two paths forward here:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it happens infrequently, it may be acceptable to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will retest the battery part more thoroughly, I've been testing on emulators and the Intents fired consistently between I'd prefer to submit the storage and battery changes, including refactor, as one change if that's OK with you. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One change sounds fine provided you can get the battery part to a state where it looks robust, without the need to additional refactoring. If additional refactoring is needed then we should start splitting it into smaller changes, which are easier to review and merge. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did some more thorough testing on the battery part and have the following findings:
This is indeed not possible as
|
||
IntentFilter intentFilter = new IntentFilter(Intent.ACTION_BATTERY_CHANGED); | ||
Intent intent = context.registerReceiver(null, intentFilter); | ||
if (intent == null) { | ||
return true; | ||
} | ||
int plugged = intent.getIntExtra(BatteryManager.EXTRA_PLUGGED, BATTERY_PLUGGED_NONE); | ||
int status = intent.getIntExtra(BatteryManager.EXTRA_STATUS, -1); | ||
int level = intent.getIntExtra(BatteryManager.EXTRA_LEVEL, -1); | ||
int scale = intent.getIntExtra(BatteryManager.EXTRA_SCALE, -1); | ||
float batteryPercentage = level / (float) scale; | ||
return (plugged != BATTERY_PLUGGED_NONE | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this is part of the logic, you'll want Do you need to change If this is a bug, it's probably also a bug in the referenced There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right that this indeed seems to be a bug in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I spoke to the maintainers of There remains an additional issue that by using Given this, do we want to add support for battery-not-low at all? It's not supported by two of the ExoPlayer There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Could you elaborate on this point a bit more? Based on the documentation for Additionally, are there plans to remove support for battery not low for the
As you say there's no robust solution for battery not low, so it seems reasonable to remove support for it until the necessary platform changes are made. Shall I close this PR and reopen a new one with just the storage not low requirement? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Right, sorry, that wasn't clear. What I meant is that they can't listen register a receiver in the manifest, which is what they need to do.
There's an active discussion happening now about what to do about work manager. From the latest update it seems the 15% magic value for firing If we're able to validate the 15% magic value across different devices, including the one where you saw 10% being used for something, then it seems OK that we would add support in ExoPlayer. We will at least need to wait until a new release of workmanager to pick up the change that removes the Apologies for the gradual sharing of new information; the discussions internally have been ongoing, so I'm just sharing things as I find out about them. In terms of next steps:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
On that test device In any case it seems that not every device abides the 15% rule, so in terms of next steps I'll put up a new PR with just the storage not low requirement. If something changes with the battery monitoring implementation we could try adding the battery requirement as well?
No worries, I appreciate you sharing the info! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Sounds good to me! Maybe just keep this one open as well so that we have the battery stuff to hand for if/when we want to use it.
Interesting. I will follow up. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If the battery goes from 9% to 11% to 9%, is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In that case Forgot to mention that neither |
||
|| status == BatteryManager.BATTERY_STATUS_UNKNOWN | ||
|| batteryPercentage > BATTERY_LOW_PERCENTAGE); | ||
} | ||
|
||
/** | ||
* Implementation taken from the the WorkManager source. | ||
* @see <a href="https://android.googlesource.com/platform/frameworks/support/+/androidx-master-dev/work/workmanager/src/main/java/androidx/work/impl/constraints/trackers/StorageNotLowTracker.java">StorageNotLowTracker</a> | ||
*/ | ||
private boolean isStorageNotLow(Context context) { | ||
IntentFilter intentFilter = new IntentFilter(); | ||
intentFilter.addAction(Intent.ACTION_DEVICE_STORAGE_OK); | ||
intentFilter.addAction(Intent.ACTION_DEVICE_STORAGE_LOW); | ||
Intent intent = context.registerReceiver(null, intentFilter); | ||
if (intent == null || intent.getAction() == null) { | ||
// ACTION_DEVICE_STORAGE_LOW is a sticky broadcast that is removed when sufficient | ||
// storage is available again. ACTION_DEVICE_STORAGE_OK is not sticky. So if we | ||
// don't receive anything here, we can assume that the storage state is okay. | ||
return true; | ||
} else { | ||
switch (intent.getAction()) { | ||
case Intent.ACTION_DEVICE_STORAGE_OK: | ||
return true; | ||
case Intent.ACTION_DEVICE_STORAGE_LOW: | ||
return false; | ||
default: | ||
// This should never happen because the intent filter is configured | ||
// correctly. | ||
return true; | ||
} | ||
} | ||
} | ||
|
||
private static boolean isInternetConnectivityValidated(ConnectivityManager connectivityManager) { | ||
// It's possible to query NetworkCapabilities from API level 23, but RequirementsWatcher only | ||
// fires an event to update its Requirements when NetworkCapabilities change from API level 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.
I think
Scheduler
needs to expose a proper API for this, since you're currently only handling one particular case (e.g., the deprecatedJobDispatcherScheduler
isn't handled correctly, and neither are customScheduler
implementations).You could have, for example, a
Requirements Scheduler.getSupportedRequirements(Requirements)
that returns the argument if everything is supported, or a newRequirements
containing the supported subset otherwise. The caller can see what's changed usingRequirements.getRequirements
on the original & return objects.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.
Got it, that seems like a better approach, will change.