Skip to content
This repository was archived by the owner on Feb 26, 2023. It is now read-only.

Conversation

@dodgex
Copy link
Member

@dodgex dodgex commented Nov 11, 2014

implements #1232

Copy link
Member

Choose a reason for hiding this comment

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

I do not think it is a good idea to catch intents with action as the method name.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, you are right, but that was the old behaviour. changing this might break existing apps.

Copy link
Member

Choose a reason for hiding this comment

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

Ooops, i see. We missed that in the initial review of this feature...
It should stay, then, but we will change the behavior in 4.0.

Copy link
Member Author

Choose a reason for hiding this comment

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

Shall i open an issue and prepare a PR (based on this one)?

Copy link
Member

Choose a reason for hiding this comment

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

Basically a PR should not depend on other PRs, but we can make an exception here. Thanks!

@WonderCsabo
Copy link
Member

I just realized @ReceiverAction adds a never used context_ field. Why that is needed? If that is really unnecessary, can you add a new commit which removes that. I also think this is not good idea to save the context param anyway, since we can leak Context that way!

Otherwise, this PR looks good to me.

@dodgex
Copy link
Member Author

dodgex commented Nov 14, 2014

currently it seems required for stuff aditional injection. e.g. i have a @Perf that gets injected in init_() using the context_.

and when i see the generated code, i'm afraid that the Broadcast receiver is potentially a (VERY) big source of unexpected behaviour/bugs.

injection happens for each recived intent... let asume we have a @Bean that may have some state.

  1. intent received
  2. bean injected
  3. @ReceiverAction called
    3.1) the method something with the bean
    3.2) method is still working...
  4. another intent
  5. [new] bean injected
  6. @ReceiverAction called for new intent
  7. BOOOOOOM the bean for the method call of the first intent is no longer available and the whole receiver is fckd up.

@WonderCsabo
Copy link
Member

You are right... init_() is called in onReceive. My questions is: why? All injection should happen in onCreate, and onReceive should only handle the incoming Intents. Am is missing sg?

@WonderCsabo
Copy link
Member

OK, i just realized. BroadcastReceiver do not have Context (nor onCreate...), so when @EReceiver was implemented they came up with this solution.

@dodgex
Copy link
Member Author

dodgex commented Nov 14, 2014

yeah. but i'm not sure if that is a good way.

@WonderCsabo
Copy link
Member

BTW that is another issue. Can you open a new ticket to discuss that?

Back to my original question: generating context_ is not "lazy enough", can you fix that in this PR.

@dodgex
Copy link
Member Author

dodgex commented Nov 14, 2014

i have no idea how to do that the best way. currently i'm testing if it is possible to just pass the context to init_() for @EReceiver

@dodgex
Copy link
Member Author

dodgex commented Nov 14, 2014

added commit + rebased on develop

@WonderCsabo
Copy link
Member

Thanks. The action and dataScheme local variables should also be generated lazily in onReceive.

@WonderCsabo
Copy link
Member

Can you add a commit which lazily generates the action and dataScheme local variables in onReceive?

@dodgex
Copy link
Member Author

dodgex commented Nov 15, 2014

@WonderCsabo i think the action is required in all cases.

for dataScheme the only idea is by adding it right before the first ifwhere it is needed. so the code might look like

String action = ...
if(action.equals(action)) {
}
if(action2.equals(action)) {
}
String dataSchemes  = ..
if(action3.equals(action) && dataschemes.equal(scheme)) {
}

is that ok?

@WonderCsabo
Copy link
Member

Action is not required if one not uses the @ReceiverAction annotation, but the local variables are still created. Check out the MyReceiver_ class in the test suite.

@WonderCsabo
Copy link
Member

@dodgex can you address my comments?

@dodgex
Copy link
Member Author

dodgex commented Nov 19, 2014

oh. sure. i have overseen the last comment with about action without @ReceiverAction. is there another comment that i'm missing?

@WonderCsabo
Copy link
Member

I think there is no other.

Copy link
Member

Choose a reason for hiding this comment

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

Move the strings into constants.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

WonderCsabo added a commit that referenced this pull request Nov 20, 2014
…eraction

1232 mutliple actions for receiveraction
@WonderCsabo WonderCsabo merged commit 5a6db4d into androidannotations:develop Nov 20, 2014
@WonderCsabo
Copy link
Member

Thanks for this contribution. Can you update the wiki?

@dodgex dodgex deleted the 1232_mutliple_actions_for_receiveraction branch November 21, 2014 23:24
@dodgex
Copy link
Member Author

dodgex commented Nov 21, 2014

@WonderCsabo
Copy link
Member

Wiki merged, thanks!

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants