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

Posting instance of subclass calls subscribers of its superclass #83

Closed
venator85 opened this Issue Apr 30, 2013 · 4 comments

Comments

3 participants
@venator85
Copy link

venator85 commented Apr 30, 2013

Hi,
I noticed that when I post() an instance of subclass on the bus, the subscribers of its superclass are also invoked.
Also, there is an asymmetry between producing an instance of a subclass and of its superclass, which call exactly once their subscribers.

Here is a "test case": https://gist.github.com/venator85/5487144

Is this intended behaviour? While the producing behaviour is consistent with the sentence "Registering will only find methods on the immediate class type." on the web site, my understanding was that it would still hold for subscribers.
Thanks ;)

@JakeWharton

This comment has been minimized.

Copy link
Collaborator

JakeWharton commented Apr 30, 2013

Registering will only find methods on the immediate class type.

This is purely a discovery contract. @Produce and @Subscribe methods will only be located on the top-most class in the hierarchy of the instances passed to register. This is as much a performance optimization as it is an observation that usage of the event bus rarely required subscription on base classes. We will be removing this restriction in future versions of the library (without compromising the API by resorting to ugly things like subscription by convention).

On the other hand, a posted event object's class hierarchy is walked and subscribers for any of its types will be invoked. The event hierarchy is much more important than the registration hierarchy. For example, while I may be interested in LocationEvents, other aspects of the system might be communicating with WiFiLocationEvents, GpsLocationEvents, or even MockLocationEvents across the bus. As a subscriber, I shouldn't be forced to know of every subtype of the type that I care about. This is polymorphism at its finest.

Make sense?

@venator85

This comment has been minimized.

Copy link

venator85 commented Apr 30, 2013

Yeah thanks for the clarification.

@venator85 venator85 closed this Apr 30, 2013

@daj

This comment has been minimized.

Copy link

daj commented Jul 20, 2015

We will be removing this restriction in future versions of the library (without compromising the API by resorting to ugly things like subscription by convention).

Do you still have plans to remove this restriction? I'm assuming not as that could be disruptive (e.g. if somebody has accidentally @Subscribed in their superclasses, a new release would cause that code to run). FYI, I don't think greenrobot's EventBus has this restriction.

If you are going to keep the restriction, then it would be great if Otto gave a compilation warning for using @Subscribe in an abstract base class. That would have saved me some time. :-)

This is as much a performance optimization as it is an observation that usage of the event bus rarely required subscription on base classes.

FYI, two common situations where I've done this are:

  1. In a common base Fragment.
  2. In a common base REST integration test class (example highlights below).

I just hit the second case now - I've been used to using EventBus instead of Otto, so I hit this limitation without expecting it.

public abstract class BaseIntegrationTest {

    public void setUp() throws Exception {
        super.setUp();
        bus.register(this);
    }

    public void tearDown() throws Exception {
        super.tearDown();

        if (bus != null) {
            bus.unregister(this);
        }
    }

    protected void resetLatch() {
        latch = new CountDownLatch(1);
    }

    protected void assertBusEventReceived() {
        try {
            logger.debug("Start waiting for event");

            if (latch.await(30, TimeUnit.SECONDS)) {
                logger.debug("Bus event received");
            } else {
                fail("Timeout waiting for bus event");
            }
        } catch (InterruptedException e) {
            fail("InterruptedException while waiting " + e.getMessage());
        }
    }

    protected void login() {
        // <my login code>
        assertBusEventReceived();
    }

    @Subscribe
    public void authenticationCallback(LoginResponse response) {
        if (response.isError()) {
            fail("Request failed");
        } else {
            latch.countDown();
        }
    }
}
@JakeWharton

This comment has been minimized.

Copy link
Collaborator

JakeWharton commented Jul 21, 2015

No one is working on Otto, and I doubt it'll see many more releases, if
any. So I wouldn't expect this to change. If you want to register in a base
class register a nested class.

On Mon, Jul 20, 2015 at 4:05 PM Dan J notifications@github.com wrote:

We will be removing this restriction in future versions of the library
(without compromising the API by resorting to ugly things like subscription
by convention).

Do you still have plans to remove this restriction? I'm assuming not as
that could be disruptive (e.g. if somebody has accidentally @subscribed
in their superclasses, a new release would cause that code to run). FYI, I
don't think greenrobot's EventBus https://github.com/greenrobot/EventBus
has this restriction.

If you are going to keep the restriction, then it would be great if Otto
gave a compilation warning for using @subscribe in an abstract base
class. That would have saved me some time. :-)

This is as much a performance optimization as it is an observation that
usage of the event bus rarely required subscription on base classes.

FYI, two common situations where I've done this are:

  1. In a common base Fragment.
  2. In a common base REST integration test class (example highlights below).

I just hit the second case now - I've been used to using EventBus instead
of Otto, so I hit this limitation without expecting it.

public abstract class BaseIntegrationTest {

public void setUp() throws Exception {
    super.setUp();
    bus.register(this);
}

public void tearDown() throws Exception {
    super.tearDown();

    if (bus != null) {
        bus.unregister(this);
    }
}

protected void resetLatch() {
    latch = new CountDownLatch(1);
}

protected void assertBusEventReceived() {
    try {
        logger.debug("Start waiting for event");

        if (latch.await(30, TimeUnit.SECONDS)) {
            logger.debug("Bus event received");
        } else {
            fail("Timeout waiting for bus event");
        }
    } catch (InterruptedException e) {
        fail("InterruptedException while waiting " + e.getMessage());
    }
}

protected void login() {
    // <my login code>
    assertBusEventReceived();
}

@Subscribe
public void authenticationCallback(LoginResponse response) {
    if (response.isError()) {
        fail("Request failed");
    } else {
        latch.countDown();
    }
}

}


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

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