Repository navigation
Always call #onNewIntent(), even without @AfterExtras method(s) #1578
Description
Activity
But
setIntent()is not called if there is no@AfterExtrasmethod. On the other hand, if there is any, it is called. So depending on having any@AfterExtrasmethods extras are or are not updated - it's inconsistency for me.The extras are injected in two places In
setIntent()and inonCreate(). So they will be injected even if there is no@AfterExtrasmethod, inonCreate(). If theActivityis not created, just a new intent reaches theActivity, then we only inject the extras if thesetIntent()method is called. Which seem to be correct to me, because if that is not called, the newIntentdoes not really belong to theActivity. The@AfterExtrasannotation clearly states the the user wants to work with the injected extras, so we callsetIntent()there.You are saying that you are just using
@AfterExtrasas dummy now. My question is: which lifecycle method you use to actually work with the newly injected extras?You are saying that you are just using @AfterExtras as dummy now. My question is: which lifecycle method you use to actually work with the newly injected extras?
In my (very uncommon I think ;-) ) case I'm only passing this particular extra (if exists) to the next Activity, without interacting with it. The case is this Intent comes from notification click and next Activity must be informed, what was in this notification.
Please consider following two example Activities:
@EActivity public class AbcActivity extends Activity { @Extra String firstExtra; @Extra String secondExtra; @AfterExtras void doSthWithSecondExtra() { if (secondExtra != null) { Log.i("Second extra is: " + secondExtra) } } @Click(R.id.my_button) void onMyButtonClick() { Log.i("First extra is: " + firstExtra) } }
@EActivity public class XyzActivity extends Activity { @Extra String firstExtra; @Click(R.id.my_button) void onMyButtonClick() { Log.i("First extra is: " + firstExtra) } }
And now passing new Intent with single top flag etc. (so without re-creating the Activity) and then clicking the button will result in different values logged.
I agree this is an uncommon case. In AA, we can always support the most common case, because we have to decide things in compile-time, which makes adding options really hard.
Can you just use the workaround? Or something wrong with that?Yea, it works with workaround (empty
@AfterExtrasmethod). But maybe at least add some info in wiki? I can do this if you want.And about making decisions at compile-time - from my point of view
onNewIntent()should be generated always if there is any@AfterExtrasmethod (as it is currently) OR@Extrafield. Looks like addingholder.getOnNewIntent();intoExtraHandlershould do the thing and I can try to do this as well, if you would like me to. But I understand it may be not so easy as it looks like at first glance.@WonderCsabo i think this makes sense and i see no reason not to support this. i even have a similar case where i use only one of my four extras inside
@AfterExtrasand the others in either click or EventBus events. here i have the@AfterExtrasbut that method could become deprecated in near future.OK, let's override
onNewIntent()and callsetIntent()always. This is maybe a breaking change, because some people may expect the originalIntentwithgetIntent()if only uses@Extra.@dodgex should we move extra injecton to
onNewIntent, or always callsetIntent?@WonderCsabo that is a difficult question. is
onNewIntentcalled when the activity is created?i think i would keep it in
setIntent.What about setting the extras in both
onNewIntent()andonCreate(). And only callsetIntent()with@AfterExtras(or not call that at all)?i think we should do it that way.
inject in
onCreate()andonNewIntent()but not callsetIntent(). the user has to overrideonNewIntent()if he needs the newIntentingetIntent()instead of the original one.but that might mean a silently breaking change. as one may rely on the
setIntent()call when usinggetIntent()somewhere.Yeah, that would be a breaking change, we will mark it in the release notes. I agree, this is a best solution, and gives the user maximum flexibility.
4 remaining items
But please remember that by default AA calls setIntent() in onNewIntent() right now in most of the cases - if there is any
@Extrafield.That is not true, only if there is a method annotated with
@AfterExtras.Sorry, you are right. But still - sometimes AA calls setIntent(), sometimes it does not.
After thinking this trough i came to this conclusion:
We should inject extras as we do currently, meaning in
onCreate()and insetIntent(). The injected extras are becoming the state of theActivity. If we would inject extras inonNewIntent(), the state would contain any lastIntentextras which may be not interesting to the object at all, and also makes the real last interesting extras inaccessible in the injected fields. Also, the current way stores the same values in the fields asgetIntent().getExtras()do, which is consistent.I think we should not override
onNewIntent(), ever, even with@AfterExtras. We could add a flag to@AfterExtrasbut what happens when there are multiple methods annotated with that, or after extras methods are coming from the super classes? So the user should have the liberty to decide whether anIntentshould be set to theActivityor not.to clarify how i understand your proposal:
- by default never override
onNewIntent()to callsetIntent() - inject in
onCreate()andsetIntent() - (maybe) add an option to
@AfterExtrasto yet again generate aonNewIntent()that callssetIntent()
this sounds ok, but i would not add the option to
@AfterExtrasbut to@EActivitythat bypasses the issue of having more than one@AfterExtrasand it is imo a better place for an activity wide option. also this allows to have the overridenonNewIntent()even when not using@AfterExtras.also i think if the user has a case where the parent and the child activity require diffrent behaviour related to
setIntent()the user has a bigger problem than we can take care of.- by default never override
I do not think
@EActivityis a good place for this, the purpose of it is different.If have a different idea: add processing option which decides whether do override
onNewIntent()or not. This could be true by default, which is okay for most of the projects.However this is also not really cool. This is a tough problem, and it seems there is no best-case scenario. :(
I'd prefer the
@EActivityover a processing option as this allows to have a per activity setting.i belive having the option on
@EActivityis the best solution. one can configure the behaviour on a per activity base and can haveonNewIntent()independently of@AfterExtrasand@Extra- this way he could even benefit from it when not using any of the extras related annotations.I think we should not add more properties to
@EActivity, but we may could introduce a new annotation for this.@OverrideOnNewIntent? :Da new annotation is ok too. at least it should be possible to decide that on a per activity base.
I would have injected extras only
onCreateandsetIntent. If the developer wants, he still can overrideonNewIntentto callsetIntent.
To help him doing so, we could add a new annotation on theActivityclass (@SetIntentOnNewIntent) but I don't think it helps a lot.OK, a decision has been made. So the only action for use is remove the
onNewIntentoverride. @dodgex can you do this as the original author of that code?I just broke my own app/code with this PR :/ :D
Implemented, thanks for the cooperation everybody.
@m-radzikowski please override
onNewIntent()manually when you need it.
If there is any method with
@AfterExtraspassing new Intent to activity causes callingonNewIntent(), from theresetIntent()and finallyinjectExtras_()where all new extras are set. And it's great.On the other hand if there is no any
@AfterExtrasmethod generated subclass has noonNewIntent()method, and in result extra is not updated.This is unfortunate if extra does not cause any instant action, but is used later - for example in my case there is only one extra that is passed by to next activity, without any operations on it. So currently I had to add blank method with
@AfterExtrasannotation just to make sure thatonNewIntent()is generated.