Add @Extra in @EService #500

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
3 participants
@DayS
Contributor

DayS commented Feb 8, 2013

Related to #466

@@ -36,6 +36,11 @@
public class EBeanHolder {
+ public static enum GeneratedClassType {

This comment has been minimized.

@pyricau

pyricau Feb 28, 2013

Contributor

I hope this class will be useful. Currently, this introduce a change to a lot of processors, just to let ExtraProcessor be able to distinguish between service and activity.

@pyricau

pyricau Feb 28, 2013

Contributor

I hope this class will be useful. Currently, this introduce a change to a lot of processors, just to let ExtraProcessor be able to distinguish between service and activity.

This comment has been minimized.

@DayS

DayS Feb 28, 2013

Contributor

I agree that it seems to be too much just for this case. But I think it could be great to have this information on every processor even if it's not used yet. I prefer that than a simple boolean isActivity or whatever

@DayS

DayS Feb 28, 2013

Contributor

I agree that it seems to be too much just for this case. But I think it could be great to have this information on every processor even if it's not used yet. I prefer that than a simple boolean isActivity or whatever

This comment has been minimized.

@pyricau

pyricau Mar 4, 2013

Contributor

By the way, did you know about the eBeanAnnotation in EBeanHolder ? You should have used it to determine GeneratedClassType.

@pyricau

pyricau Mar 4, 2013

Contributor

By the way, did you know about the eBeanAnnotation in EBeanHolder ? You should have used it to determine GeneratedClassType.

This comment has been minimized.

@DayS

DayS Mar 13, 2013

Contributor

I agree. I didn't see that attribute before. I'll change that

@DayS

DayS Mar 13, 2013

Contributor

I agree. I didn't see that attribute before. I'll change that

- setIntentBody.invoke(_super(), setIntentMethod).arg(methodParam);
- setIntentBody.invoke(initIntentMethod);
+ // Override only if it's an activity
+ if (holder.classType == GeneratedClassType.ACTIVITY) {

This comment has been minimized.

@pyricau

pyricau Feb 28, 2013

Contributor

Shouldn't we check for the whole injectOrOverrideSetIntent method ? I don't get why you added "setIntent()" to Service, is this useful in any way ?

@pyricau

pyricau Feb 28, 2013

Contributor

Shouldn't we check for the whole injectOrOverrideSetIntent method ? I don't get why you added "setIntent()" to Service, is this useful in any way ?

@@ -78,6 +81,21 @@ public void process(Element element, JCodeModel codeModel, EBeansHolder eBeansHo
onCreateBody.invoke(JExpr._super(), onCreate);
}
+ holder.initStartCommand = holder.generatedClass.method(PRIVATE, codeModel.VOID, "initStartCommand_");

This comment has been minimized.

@pyricau

pyricau Feb 28, 2013

Contributor

I don't get the need for this initStartCommand_ method. Here is what's generated :

    @Override
    public int onStartCommand(Intent intent, int flag, int startId) {
        initStartCommand_(intent);
        return super.onStartCommand(intent, flag, startId);
    }

    private void initStartCommand_(Intent intent) {
        if (intent!= null) {
            injectExtras_(intent.getExtras());
        }
    }

What about this instead:

    @Override
    public int onStartCommand(Intent intent, int flag, int startId) {
        if (intent!= null) {
            injectExtras_(intent.getExtras());
        }
        return super.onStartCommand(intent, flag, startId);
    }
@pyricau

pyricau Feb 28, 2013

Contributor

I don't get the need for this initStartCommand_ method. Here is what's generated :

    @Override
    public int onStartCommand(Intent intent, int flag, int startId) {
        initStartCommand_(intent);
        return super.onStartCommand(intent, flag, startId);
    }

    private void initStartCommand_(Intent intent) {
        if (intent!= null) {
            injectExtras_(intent.getExtras());
        }
    }

What about this instead:

    @Override
    public int onStartCommand(Intent intent, int flag, int startId) {
        if (intent!= null) {
            injectExtras_(intent.getExtras());
        }
        return super.onStartCommand(intent, flag, startId);
    }
@@ -43,12 +43,7 @@ public ExtraValidator(ProcessingEnvironment processingEnv) {
public boolean validate(Element element, AnnotationElements validatedElements) {
IsValid valid = new IsValid();
- /*
- * TODO since we override setIntent(), we should check that the

This comment has been minimized.

@pyricau

pyricau Feb 28, 2013

Contributor

This TODO is still valid, even if @Override is removed. We can't override a final method, regardless of putting @Override.

@pyricau

pyricau Feb 28, 2013

Contributor

This TODO is still valid, even if @Override is removed. We can't override a final method, regardless of putting @Override.

@@ -44,6 +45,9 @@
@OrmLiteDao(helper = DatabaseHelper.class, model = User.class)
UserDao userDao;
+
+ @Extra
+ Long someId;

This comment has been minimized.

@pyricau

pyricau Feb 28, 2013

Contributor

Is that useful ? We already have ExtraInjectedService.

@pyricau

pyricau Feb 28, 2013

Contributor

Is that useful ? We already have ExtraInjectedService.

This comment has been minimized.

@DayS

DayS Feb 28, 2013

Contributor

This one is useless. I added this one before ExtraInjectedService and I forgot to remove it.

@DayS

DayS Feb 28, 2013

Contributor

This one is useless. I added this one before ExtraInjectedService and I forgot to remove it.

@pyricau

This comment has been minimized.

Show comment
Hide comment
@pyricau

pyricau Feb 28, 2013

Contributor

Most important note : I don't think this pull request actually solves #466.

@erickok wants to use @Extra in an IntentService. Let's read the onStartCommand() javadoc for IntentService:

You should not override this method for your IntentService. Instead, override onHandleIntent(Intent), which the system calls when the IntentService receives a start request.

And that's because onHandleIntent() executes in a background thread. onStartCommand() may be called multiple times while onHandleIntent() is executing.

So if the injected extras may change while executing onHandleIntent(). And you can expect concurrency issues too.

Contributor

pyricau commented Feb 28, 2013

Most important note : I don't think this pull request actually solves #466.

@erickok wants to use @Extra in an IntentService. Let's read the onStartCommand() javadoc for IntentService:

You should not override this method for your IntentService. Instead, override onHandleIntent(Intent), which the system calls when the IntentService receives a start request.

And that's because onHandleIntent() executes in a background thread. onStartCommand() may be called multiple times while onHandleIntent() is executing.

So if the injected extras may change while executing onHandleIntent(). And you can expect concurrency issues too.

@erickok

This comment has been minimized.

Show comment
Hide comment
@erickok

erickok Feb 28, 2013

I think @pyricau is right, I think. Luckily, intents are queued, that is, every Intent will be handled in serial (all executed on a single worker thread), so there should be no problem injecting the extras just before a new onHandleIntent() is started.

Here's a suggestion. Given the following simple annotated class:

@EService
public class ExtraInjectedService extends IntentService {

    @Extra("stringExtra")
    String stringExtra;

    @Override
    protected void onHandleIntent(Intent intent) {
        // Do some stuff...
        Log.d(ExtraInjectedService.class.getSimpleName(), "We now have access to injected String extra: " + stringExtra);
    }

}

Should generate something like:

public class ExtraInjectedService_ extends ExtraInjectedService {

    @Override
    protected void onHandleIntent(Intent intent) {
        extraString = intent.getStringExtra("stringExtra");
        super.onHandleIntent(Intent intent);
    }

}

And then also generale a Builder inner class in the ExtraInjectedService_. That should work right?

erickok commented Feb 28, 2013

I think @pyricau is right, I think. Luckily, intents are queued, that is, every Intent will be handled in serial (all executed on a single worker thread), so there should be no problem injecting the extras just before a new onHandleIntent() is started.

Here's a suggestion. Given the following simple annotated class:

@EService
public class ExtraInjectedService extends IntentService {

    @Extra("stringExtra")
    String stringExtra;

    @Override
    protected void onHandleIntent(Intent intent) {
        // Do some stuff...
        Log.d(ExtraInjectedService.class.getSimpleName(), "We now have access to injected String extra: " + stringExtra);
    }

}

Should generate something like:

public class ExtraInjectedService_ extends ExtraInjectedService {

    @Override
    protected void onHandleIntent(Intent intent) {
        extraString = intent.getStringExtra("stringExtra");
        super.onHandleIntent(Intent intent);
    }

}

And then also generale a Builder inner class in the ExtraInjectedService_. That should work right?

@pyricau

This comment has been minimized.

Show comment
Hide comment
@pyricau

pyricau Feb 28, 2013

Contributor

I started thinking like @erickok, but then something else occurred to me : we cannot reset the fields to their initial values.

Let's say we have this :

@EService
public class ExtraInjectedService extends IntentService {

    @Extra("stringExtra")
    String stringExtra = "foobar";

    @Override
    protected void onHandleIntent(Intent intent) {
        // Do some stuff...
        Log.d(ExtraInjectedService.class.getSimpleName(), "We now have access to injected String extra: " + stringExtra);
    }

}

If you start it twice, the first time with a "stringExtra", the second time without. Then the second time the field will be set to the stringExtra instead of "foobar" (default value).

I'm really started to think we shouldn't mix @Extra with services, because of their lifecycle.

Contributor

pyricau commented Feb 28, 2013

I started thinking like @erickok, but then something else occurred to me : we cannot reset the fields to their initial values.

Let's say we have this :

@EService
public class ExtraInjectedService extends IntentService {

    @Extra("stringExtra")
    String stringExtra = "foobar";

    @Override
    protected void onHandleIntent(Intent intent) {
        // Do some stuff...
        Log.d(ExtraInjectedService.class.getSimpleName(), "We now have access to injected String extra: " + stringExtra);
    }

}

If you start it twice, the first time with a "stringExtra", the second time without. Then the second time the field will be set to the stringExtra instead of "foobar" (default value).

I'm really started to think we shouldn't mix @Extra with services, because of their lifecycle.

@erickok

This comment has been minimized.

Show comment
Hide comment
@erickok

erickok Feb 28, 2013

I can see your point, but don't activities have the same issue? If new intents are set on an existing activity instead of creating a new activity?

erickok commented Feb 28, 2013

I can see your point, but don't activities have the same issue? If new intents are set on an existing activity instead of creating a new activity?

@pyricau

This comment has been minimized.

Show comment
Hide comment
@pyricau

pyricau Feb 28, 2013

Contributor

Damn it. We override setIntent() in activities and reinject extras there, but you're right, there's exactly the same problem. Hum..

Contributor

pyricau commented Feb 28, 2013

Damn it. We override setIntent() in activities and reinject extras there, but you're right, there's exactly the same problem. Hum..

@DayS

This comment has been minimized.

Show comment
Hide comment
@DayS

DayS Mar 13, 2013

Contributor

Well, it seems that this functionality is quite more complicated to implement than I expected...

What should we do about the reset field issue ?

Contributor

DayS commented Mar 13, 2013

Well, it seems that this functionality is quite more complicated to implement than I expected...

What should we do about the reset field issue ?

@DayS

This comment has been minimized.

Show comment
Hide comment
@DayS

DayS May 21, 2013

Contributor

I'm closing this PR as we don't have any good solution.

Contributor

DayS commented May 21, 2013

I'm closing this PR as we don't have any good solution.

@DayS DayS closed this May 21, 2013

@WonderCsabo WonderCsabo referenced this pull request Sep 2, 2014

Closed

Inject local services #1114

@WonderCsabo WonderCsabo deleted the 466_AddExtraInService branch Dec 10, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment