Adds storage not low requirement#7395
Conversation
Added monitoring storage levels in RequirementsWatcher Added dependency on extension-workmanager to the demo app to be able to test with WorkManagerScheduler Added getSupportedRequirements method to Scheduler interface Implemented getSupportedRequirements for schedulers
| private boolean isStorageNotLow(Context context) { | ||
| IntentFilter intentFilter = new IntentFilter(); | ||
| intentFilter.addAction(Intent.ACTION_DEVICE_STORAGE_OK); | ||
| intentFilter.addAction(Intent.ACTION_DEVICE_STORAGE_LOW); |
There was a problem hiding this comment.
@jdegroot-dss It works only for internal storage. When downloading to external storage, this method of determining is useless and even false. If there is no space in the internal storage, we cannot download to the external storage when this requirement is enabled
There was a problem hiding this comment.
@jdegroot-dss Does this affect your intended usage at all, or is it fine for your use case? It still seems OK to support this to me, although perhaps this should be made clearer in the documentation. If someone is placing content onto external storage they can just not use the feature.
If it's still useful for you, please just confirm this here and I'll look at getting it merged in. Thanks!
There was a problem hiding this comment.
Thanks for the heads up @vigneshvg, I was unaware of that limitation.
@ojw28 I don't think this will affect our use case and it would still be useful to have. Agreed with updating the documentation. Do we want to restrict the usage of the requirement if downloads are going to external storage?
There was a problem hiding this comment.
I think just making it clear in the documentation is enough.
| } else { | ||
| switch (intent.getAction()) { | ||
| case Intent.ACTION_DEVICE_STORAGE_OK: | ||
| return true; |
There was a problem hiding this comment.
I guess based on the comment above that you copied this code from elsewhere, but if ACTION_DEVICE_STORAGE_OK is not sticky, how does this case ever occur :)? Seems to me we can completely remove ACTION_DEVICE_STORAGE_OK from this case without any change in behavior?
There was a problem hiding this comment.
As a summary, I wonder whether this entire method can just be:
@Nullable Intent intent =
context.registerReceiver(null, new IntentFilter(Intent.ACTION_DEVICE_STORAGE_LOW));
return intent == null;
There was a problem hiding this comment.
No need to fix this. I can do it when I merge. Although please check whether the proposed alternative is sufficient!
There was a problem hiding this comment.
You are correct! Tested this and it works as expected.
| @Override | ||
| public Requirements getSupportedRequirements(Requirements requirements) { | ||
| Requirements supportedRequirements = requirements; | ||
| if (Util.SDK_INT < 26) { |
There was a problem hiding this comment.
Hm. If it's supported for API level 26 and above, it needs wiring up in the buildJobInfo method below, doesn't it?
There was a problem hiding this comment.
No need to fix this. I can do it when I merge. Just confirming that the comment is correct is fine :).
There was a problem hiding this comment.
Good catch, that is correct.
…ement PiperOrigin-RevId: 313804207
|
I'm not sure why our tooling didn't mark this as merged, but it was (with some non-functional changes) in 496a315. Thanks! |
4 participants
Added monitoring storage levels in RequirementsWatcher
Added dependency on extension-workmanager to the demo app to be able to test with WorkManagerScheduler
Added getSupportedRequirements method to Scheduler interface
Implemented getSupportedRequirements for schedulers