Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Annotation Support #126

Closed
rjbrock opened this issue Nov 12, 2014 · 23 comments
Closed

Annotation Support #126

rjbrock opened this issue Nov 12, 2014 · 23 comments
Labels
confirmed enhancement New feature or request
Milestone

Comments

@rjbrock
Copy link
Contributor

rjbrock commented Nov 12, 2014

Hey, this looks like a great library, and I have been exploring using it. I know you guys have quite done quite a bit of research regarding Anotations vs Method Naming, but overall I really feel like Annotations are a much cleaner and more flexible solution.

I added annotations to my local branch. You can create a new EventBus that uses annotations like this new EventBus(DeclarationType.ANNOTATIONS). Then you can subscribe to receive events on any method like this:

@Subscribe
public void onMyEvent(CustomEvent e)

This uses the caller chooses thread method. We can also change this by adding an argument.

@Subscribe(threadMode = ThreadMode.Background)
public void onMyEvent(CustomEvent e)

This uses the background thread and is definitely a more explicit syntax.

Here is a link to my repo where I implemented the changes.
https://github.com/rjbrock/EventBus

I would still need to add unit tests before submitting a pull request so I just wanted to see what your thoughts were first.

Rick

@greenrobot
Copy link
Owner

Awesome! Annotations were planned for EventBus version 3. It makes sense today, because Android 4 is adopted widely these days. With Android 4, the performance difference should not really matter anymore. You can verify with the performance app project. Looking forward to check your changes, however it might get December - so no hurry. We just did a release anyway. Thanks!

@rjbrock
Copy link
Contributor Author

rjbrock commented Nov 12, 2014

Sounds good! Here is a link to the actual changeset.

rjbrock@e42992a

If things look ok (so far) I can flesh this out with more comments and unit test coverage as well as docs.

@greenrobot
Copy link
Owner

I see it's possible to do either method names searching and Annotations? That's nice for transition. I'd suggest to move that switch to the builder. The forthcoming release (e.g. 3.1) can prune the method name option afterwards.

Let's think a bit about consequences of adopting annotations:

  • Options should move from registration to annotation (sticky, priority)
  • Lots of unit tests have to be adjusted
  • New unit tests are required, e.g. there may be multiple subscriber methods wired to the same subscriber, but with different options (sticky, priority)

What do you think?

PS.: Please ensure you are working on the latest version, there were some changes in this area...

@greenrobot greenrobot changed the title Question about Annotations Annotation Support Nov 12, 2014
@greenrobot greenrobot added enhancement New feature or request confirmed labels Nov 12, 2014
@greenrobot
Copy link
Owner

Giving it some more thought, maybe we should just drop method name searching completely? It will get messy supporting both. Forcing people to add an annotation to their onEvent method should not be a big problem, I think.

@rjbrock
Copy link
Contributor Author

rjbrock commented Nov 12, 2014

I am totally on board with dropping method name searching completely. My goal with the original changeset that I linked you to was to be as minimally intrusive as possible.

I will update my changes to remove method name searching altogether then.

Do you want me working off of my repo for now? I will create an annotations branch.

@greenrobot
Copy link
Owner

Sounds good. Next weeks will be busy for me, but I'm really looking forward to have a look.

@MariusVolkhart
Copy link

How is this going to work with inheritance? When using Otto for example, you can't put an annotation on a superclass and have the subclass get the message because Otto doesn't traverse the class hierarchy for performance reasons.

@rjbrock
Copy link
Contributor Author

rjbrock commented Dec 29, 2014

@MariusVolkhart This doesn't change any behavior. This only changes the way that reflection is used to find the methods that are subscribing.

@MariusVolkhart
Copy link

Ok I did some more reading. I remembered there being a limitation to inheriting annotations, but it's just that annotations can't come from interfaces, they have to come from super classes. Looking through #130 I see it does traverse the entire class hierarchy to find the annotations so that should work.

@greenrobot
Copy link
Owner

I merged the branch of @rjbrock into this repo's annotations branch:
https://github.com/greenrobot/EventBus/tree/annotations

To make it a bit faster, I also did some work on SubscriberMethodFinder. Registration got like 30-40% faster, but it's still much slower than without annotation-support (tested on Android 5.0). Maybe this needs some more work.

@greenrobot
Copy link
Owner

Update: added annotation processor to index @ Subscribe annotation during compilation. Performance is pretty good like this. Still alpha state.

@rjbrock
Copy link
Contributor Author

rjbrock commented Feb 24, 2015

@greenrobot awesome, thanks for taking a look at this. Any help you need from me? L

@greenrobot
Copy link
Owner

@rjbrock and everyone: it would be cool if you could give it a shot. I'd be especially curious about your feedback on annotation processing at compile time. Thanks!

@g-neykov
Copy link

g-neykov commented Apr 3, 2015

@greenrobot and @rjbrock: I have been extensively using EventBus in projects I work on and I have been tracking the progress and changes you're making to switch annotations. I see that anonymous class support may get dropped because they cannot be annotated. I assume many people have been registering anonymous classes to cope with Activity/Fragment lifecycle hell and ensure events get delivered only in proper state of components. Is it possible to add as a feature partial object subscription (eg. add something like EventBus.register(Object subscriber, Class eventTypes) and a corresponding unregister(Object subscriber, Class eventTypes)). Would adding some sort of filtering to SubscriberMethodFinder hurt performance too much?

The reason to suggest that is that it would make subscriptions more flexible and would reduce boilerplate when using with Activity/Fragments when there would be events requiring view interaction/updating or Fragment transactions which can be done only while some tricky conditions are met.
I am willing to help if you think that is feasible to add my suggestion as feature.

@greenrobot
Copy link
Owner

@g-neykov Agreed. I think there is no way to detect anonymous classes by the annotation processor. Anyone is welcome to verify this shortcoming, maybe there's some work around. Thanks!

I don't want to (re-)inroduce "EventBus.register(Object subscriber, Class<?> eventTypes)" to keep the API simple.

@g-neykov
Copy link

g-neykov commented Apr 7, 2015

I see. Anyway, a big "Thank you" to you and all other contributors for the
wonderful library.

On Tue, Apr 7, 2015 at 7:53 AM, greenrobot notifications@github.com wrote:

@g-neykov https://github.com/g-neykov Agreed. I think there is no way
to detect anonymous classes by the annotation processor. Anyone is welcome
to verify this shortcoming, maybe there's some work around. Thanks!

I don't want to (re-)inroduce "EventBus.register(Object subscriber,
Class<?> eventTypes)" to keep the API simple.


Reply to this email directly or view it on GitHub
#126 (comment).

@smorloc
Copy link

smorloc commented Apr 22, 2015

When will snapshot builds be available? They are not currently available on Sonatype's snapshot repository (as stated in README.md).

@WonderCsabo
Copy link

@greenrobot i can confirm that anonymous classes are not available for annotation processing.

Do you plan to release a snapshot which contains the annotations branch?

@BattleShipPark
Copy link

Could you build a release including this? Thanks.

@minhoryang
Copy link

+1 Awesome! Love it

@greenrobot
Copy link
Owner

Yesterday, I announced a beta version available on Maven Central: http://androiddevblog.com/eventbus-3-droidcon/

Looking forward to your feedback!

@greenrobot greenrobot mentioned this issue Oct 27, 2015
@jymot
Copy link

jymot commented Oct 27, 2015

Awesome!

@greenrobot greenrobot added this to the 3.0.0 milestone Nov 19, 2015
@lordcodes
Copy link

This feature is implemented and can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed enhancement New feature or request
Projects
None yet
Development

No branches or pull requests