-
Notifications
You must be signed in to change notification settings - Fork 562
Adding extension function for scheduling jobs #555
base: master
Are you sure you want to change the base?
Conversation
} | ||
|
||
var looperWaitCount = MAX_LOOPER_WAIT_COUNT | ||
lock.withLock { |
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.
All of this locking seems like it can be replaced with a simple CountDownLatch(1)
and await with a reasonable timeout. Right now it seems overly complicated for no good reason.
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.
Agreed and updated. I was just following the convention I observed in the Android CTS.
|
||
class JobSchedulerTest { | ||
@Test fun testScheduleJob() { | ||
scheduleJob(context, ComponentName(context, TestService::class.java), 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.
Having to use the context twice here is weird. This function call does not read very well. Perhaps we should address #526 first. But beyond that I'm not sure I'm convinced we need to handle scheduling and job creation in a single function.
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've updated the API to
scheduleJob<TestService>(context, jobId) {
/* ... */
}
which I think reads much nicer. Also, #526 won't change the function call in this case. Do you think this version makes it worth it to handle the scheduling and creation in a single function?
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've updated the API again as per @rom4ek's suggestion and also added an upper bound on the type passed to the ComponentName
. Please check out the Benefits section in the PR description to see why I think this would be worth it. Looking forward to hearing your thoughts! If you still feel that scheduling and job creation shouldn't be coupled like this, then I can just update the extension to simply provide the functionality of job creation.
Shouldn't this be an extension function of |
Good point ... fixed |
@@ -70,4 +74,29 @@ class ContextTest { | |||
assertTrue(getInt(R.styleable.SampleAttrs_sample, -1) != -1) | |||
} | |||
} | |||
|
|||
@Test fun testScheduleJob() { | |||
context.scheduleJob<TestService>( 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.
Formatting
Shouldn't this return the int that is returned from |
Fixed |
You can have the get system service and schedule calls in one line. Also note that I'm just a visitor and not a contributor. Those who manage the project may have a different opinion on the safe casting. |
Just keeping it on separate lines for formatting |
Resolves #554
Signature
Before
After
Benefits
Context
parameterService
type upper bound for the class given to theJobInfo.Builder
via theComponentName
(also cleaner by avoiding::class.java
)ComponentName
instanceJobInfo
build
andschedule
calls* code snippet adapted from Google Samples