Skip to content
This repository has been archived by the owner on Nov 14, 2018. It is now read-only.

Adding extension function for scheduling jobs #555

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

Adding extension function for scheduling jobs #555

wants to merge 2 commits into from

Conversation

RamV13
Copy link
Contributor

@RamV13 RamV13 commented May 23, 2018

Resolves #554

Signature

inline fun <reified T : Service> Context.scheduleJob(
    jobId: Int,
    buildSequence: JobInfo.Builder.() -> Unit
): Int

Before

val serviceComponent = ComponentName(this, MyService::class.java)
val builder = JobInfo.Builder(jobId++, serviceComponent)

if (wiFiConnectivityRadioButton.isChecked) {
    builder.setRequiredNetworkType(JobInfo.NETWORK_TYPE_UNMETERED)
} else if (anyConnectivityRadioButton.isChecked) {
    builder.setRequiredNetworkType(JobInfo.NETWORK_TYPE_ANY)
}

systemService<JobScheduler>().schedule(builder.build())

After

scheduleJob<MyService>(jobId++) {
    if (wiFiConnectivityRadioButton.isChecked) {
        setRequiredNetworkType(JobInfo.NETWORK_TYPE_UNMETERED)
    } else if (anyConnectivityRadioButton.isChecked) {
        setRequiredNetworkType(JobInfo.NETWORK_TYPE_ANY)
    }
}

Benefits

  • no need for a Context parameter
  • compile-time enforcement of the Service type upper bound for the class given to the JobInfo.Builder via the ComponentName (also cleaner by avoiding ::class.java)
  • automatic construction of the ComponentName instance
  • lambda with receiver for building the JobInfo
  • automatic system service retrieval as well as build and schedule calls

* code snippet adapted from Google Samples

}

var looperWaitCount = MAX_LOOPER_WAIT_COUNT
lock.withLock {
Copy link
Contributor

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.

Copy link
Contributor Author

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) {
Copy link
Contributor

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.

Copy link
Contributor Author

@RamV13 RamV13 May 24, 2018

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?

Copy link
Contributor Author

@RamV13 RamV13 May 25, 2018

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.

@romtsn
Copy link
Contributor

romtsn commented May 24, 2018

Shouldn't this be an extension function of Context then?

@RamV13
Copy link
Contributor Author

RamV13 commented May 24, 2018

Good point ... fixed

@@ -70,4 +74,29 @@ class ContextTest {
assertTrue(getInt(R.styleable.SampleAttrs_sample, -1) != -1)
}
}

@Test fun testScheduleJob() {
context.scheduleJob<TestService>( 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Formatting

@RamV13 RamV13 changed the title Adding helper function for JobScheduler Adding extension function for JobScheduler May 25, 2018
@RamV13 RamV13 changed the title Adding extension function for JobScheduler Adding extension function for scheduling jobs May 25, 2018
@AllanWang
Copy link

Shouldn't this return the int that is returned from schedule? And in light of that, I'm not sure if this constitutes a feature addition or not, but you could also use safe casting and return JobScheduler.RESULT_FAILURE if the service isn't found, which seems to happen a lot.

@RamV13
Copy link
Contributor Author

RamV13 commented May 28, 2018

Fixed

@AllanWang
Copy link

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.

@RamV13
Copy link
Contributor Author

RamV13 commented May 28, 2018

Just keeping it on separate lines for formatting

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

Successfully merging this pull request may close these issues.

4 participants