From 58ee5bd110e4f8a4307b3270a9e10f8ab5456478 Mon Sep 17 00:00:00 2001 From: Guille Date: Thu, 29 May 2014 16:24:11 +0200 Subject: [PATCH 1/2] Some fixes to the JavaDoc's New register & unregister methods in Bus for explicit (not by reflection) EventHandler definition Added a generic Callback interface to be extended (implemented) by third parties Added a specific implementation of MethodCallingCallback with the orginal implementation of an EventHandler created by reflection of @Subscribe annotations in objects Added tests to cover added features Added example of explicir registration&un-registration of Callbacks in sample project --- library/pom.xml | 2 +- .../src/main/java/com/squareup/otto/Bus.java | 46 ++++++++++ .../main/java/com/squareup/otto/Callback.java | 19 +++++ .../java/com/squareup/otto/EventHandler.java | 56 ++++++------ .../squareup/otto/MethodInvokingCallback.java | 85 +++++++++++++++++++ .../test/java/com/squareup/otto/BusTest.java | 61 +++++++++++++ .../com/squareup/otto/EventHandlerTest.java | 10 +++ pom.xml | 2 +- sample/pom.xml | 2 +- .../otto/sample/LocationHistoryFragment.java | 23 +++-- 10 files changed, 270 insertions(+), 36 deletions(-) create mode 100644 library/src/main/java/com/squareup/otto/Callback.java create mode 100644 library/src/main/java/com/squareup/otto/MethodInvokingCallback.java diff --git a/library/pom.xml b/library/pom.xml index de325a0..610e6c6 100644 --- a/library/pom.xml +++ b/library/pom.xml @@ -20,7 +20,7 @@ com.squareup otto-parent - 1.3.5-SNAPSHOT + 1.3.6-SNAPSHOT ../pom.xml diff --git a/library/src/main/java/com/squareup/otto/Bus.java b/library/src/main/java/com/squareup/otto/Bus.java index f86e374..8cc42c3 100644 --- a/library/src/main/java/com/squareup/otto/Bus.java +++ b/library/src/main/java/com/squareup/otto/Bus.java @@ -239,6 +239,27 @@ public void register(Object object) { } } + /** + * Registers a new EventHandler for events of the provided type that will execute + * the provided callback whenever it is posted. + * + * @param type event class to subscribe to. + * @param callback Callback instance that will be executed when an event of the + * provided type is posted. + */ + public void register(Class type, Callback callback) { + Set handlers = handlersByType.get(type); + if (handlers == null) { + //concurrent put if absent + Set handlersCreation = new CopyOnWriteArraySet(); + handlers = handlersByType.putIfAbsent(type, handlersCreation); + if (handlers == null) { + handlers = handlersCreation; + } + } + handlers.add(new EventHandler(callback)); + } + private void dispatchProducerResultToHandler(EventHandler handler, EventProducer producer) { Object event = null; try { @@ -299,6 +320,31 @@ public void unregister(Object object) { } } + /** + * Unregisters a Callback for a particular event type + * + * @param type event class to unsubscribe from. + * @param callback Callback instance that would be executed when an event of the + * provided type was posted. + */ + public void unregister(Class type, Callback callback) { + Set currentHandlers = getHandlersForEventType(type); + if (currentHandlers == null) { + throw new IllegalArgumentException("There are no EventHandlers for type " + type); + } + EventHandler eventHandler = null; + for (EventHandler candidate : currentHandlers) { + if (candidate.hasCallback(callback)) { + eventHandler = candidate; + } + } + if (eventHandler == null) { + throw new IllegalArgumentException("There is no EventHandler for type " + type + " with callback " + callback); + } + eventHandler.invalidate(); + currentHandlers.remove(eventHandler); + } + /** * Posts an event to all registered handlers. This method will return successfully after the event has been posted to * all handlers, and regardless of any exceptions thrown by handlers. diff --git a/library/src/main/java/com/squareup/otto/Callback.java b/library/src/main/java/com/squareup/otto/Callback.java new file mode 100644 index 0000000..cbda95b --- /dev/null +++ b/library/src/main/java/com/squareup/otto/Callback.java @@ -0,0 +1,19 @@ +package com.squareup.otto; + +import java.lang.reflect.InvocationTargetException; + +/** + *

This interface represents something that an EventHandler can call whenever it is posted an Event.

+ * + * @author Guillermo Gutierrez + */ +public interface Callback { + /** + *

This method will be called whenever an Event is posted to the EventHandler that has an implementation of this + * interface.

+ * + * @param event event object that has been posted + * @throws InvocationTargetException + */ + void call(T event) throws InvocationTargetException; +} diff --git a/library/src/main/java/com/squareup/otto/EventHandler.java b/library/src/main/java/com/squareup/otto/EventHandler.java index 843a31b..8547a33 100644 --- a/library/src/main/java/com/squareup/otto/EventHandler.java +++ b/library/src/main/java/com/squareup/otto/EventHandler.java @@ -25,7 +25,7 @@ * *

This class only verifies the suitability of the method and event type if something fails. Callers are expected t * verify their uses of this class. - * + *

*

Two EventHandlers are equivalent when they refer to the same method on the same object (not class). This * property is used to ensure that no handler method is registered more than once. * @@ -33,10 +33,8 @@ */ class EventHandler { - /** Object sporting the handler method. */ - private final Object target; - /** Handler method. */ - private final Method method; + /** Handler Callback. */ + private final Callback callback; /** Object hash code. */ private final int hashCode; /** Should this handler receive events? */ @@ -50,14 +48,23 @@ class EventHandler { throw new NullPointerException("EventHandler method cannot be null."); } - this.target = target; - this.method = method; - method.setAccessible(true); + // Compute hash code eagerly since we know it will be used frequently and we cannot estimate the runtime of the + // target's hashCode call. + final int prime = 31; + this.callback = new MethodInvokingCallback(target, method); + this.hashCode = (prime + callback.hashCode()) * prime + target.hashCode(); + } + + EventHandler(final Callback callback) { + if (callback == null) { + throw new NullPointerException("EventHandler callback cannot be null."); + } // Compute hash code eagerly since we know it will be used frequently and we cannot estimate the runtime of the // target's hashCode call. final int prime = 31; - hashCode = (prime + method.hashCode()) * prime + target.hashCode(); + this.callback = callback; + this.hashCode = (prime + callback.hashCode()) * prime; } public boolean isValid() { @@ -74,31 +81,24 @@ public void invalidate() { } /** - * Invokes the wrapped handler method to handle {@code event}. + * Invokes the wrapped handler callback to handle {@code event}. * - * @param event event to handle - * @throws java.lang.IllegalStateException if previously invalidated. - * @throws java.lang.reflect.InvocationTargetException if the wrapped method throws any {@link Throwable} that is not - * an {@link Error} ({@code Error}s are propagated as-is). + * @param event event to handle + * @throws java.lang.reflect.InvocationTargetException if the wrapped callback throws any + * {@link Throwable} that is not + * an {@link Error} ({@code Error}s + * are propagated as-is). */ + @SuppressWarnings("unchecked") public void handleEvent(Object event) throws InvocationTargetException { if (!valid) { throw new IllegalStateException(toString() + " has been invalidated and can no longer handle events."); } - try { - method.invoke(target, event); - } catch (IllegalAccessException e) { - throw new AssertionError(e); - } catch (InvocationTargetException e) { - if (e.getCause() instanceof Error) { - throw (Error) e.getCause(); - } - throw e; - } + callback.call(event); } @Override public String toString() { - return "[EventHandler " + method + "]"; + return "[EventHandler " + callback.toString() + "]"; } @Override public int hashCode() { @@ -120,7 +120,11 @@ public void handleEvent(Object event) throws InvocationTargetException { final EventHandler other = (EventHandler) obj; - return method.equals(other.method) && target == other.target; + return callback.equals(other.callback); } + + public boolean hasCallback(Callback callback) { + return this.callback.equals(callback); + } } diff --git a/library/src/main/java/com/squareup/otto/MethodInvokingCallback.java b/library/src/main/java/com/squareup/otto/MethodInvokingCallback.java new file mode 100644 index 0000000..5554cfb --- /dev/null +++ b/library/src/main/java/com/squareup/otto/MethodInvokingCallback.java @@ -0,0 +1,85 @@ +package com.squareup.otto; + +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; + +/** + *

Extracted from EventHandler's original implementation

+ *

Wraps a Method in a Callback interface in order to be called whenever the EventHandler is posted an Event

+ *

This class only verifies the suitability of the method and event type if something fails. Callers are expected to + * verify their uses of this class.

+ *

Two MethodInvokingCallback are equivalent when they refer to the same method on the same object (not class). + * This property is used to ensure that no handler method is registered more than once.

+ * + * @author Guillermo Gutierrez + */ +public class MethodInvokingCallback implements Callback { + private final Object target; + private final Method method; + private final int hashCode; + + public MethodInvokingCallback(Object target, Method method) { + method.setAccessible(true); + this.target = target; + this.method = method; + // Compute hash code eagerly since we know it will be used frequently and we cannot estimate the runtime of the + // target's hashCode call. + final int prime = 31; + hashCode = (prime + method.hashCode()) * prime + target.hashCode(); + } + + /** + *

This method will be called whenever an Event is posted to the EventHandler that has an implementation of this + * interface.

+ * + * @param event event to handle + * @throws java.lang.reflect.InvocationTargetException if the wrapped method throws any {@link Throwable} that is not + * an {@link Error} ({@code Error}s are propagated as-is). + */ + @Override + public void call(Object event) throws InvocationTargetException { + try { + method.invoke(target, event); + } catch (IllegalAccessException e) { + throw new AssertionError(e); + } catch (InvocationTargetException e) { + if (e.getCause() instanceof Error) { + throw (Error) e.getCause(); + } + throw e; + } + } + + public Method getMethod() { + return method; + } + + @Override + public String toString() { + return method.toString(); + } + + @Override + public int hashCode() { + return hashCode; + } + + @Override + public boolean equals(Object obj) { + if (this == obj) { + return true; + } + + if (obj == null) { + return false; + } + + if (getClass() != obj.getClass()) { + return false; + } + + final MethodInvokingCallback other = (MethodInvokingCallback) obj; + + return method.equals(other.getMethod()) && target == other.target; + } +} diff --git a/library/src/test/java/com/squareup/otto/BusTest.java b/library/src/test/java/com/squareup/otto/BusTest.java index 5f42e9b..4ed98da 100644 --- a/library/src/test/java/com/squareup/otto/BusTest.java +++ b/library/src/test/java/com/squareup/otto/BusTest.java @@ -19,6 +19,7 @@ import org.junit.Before; import org.junit.Test; +import java.lang.reflect.InvocationTargetException; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; @@ -305,6 +306,66 @@ class VoidProducer { expectedEvents, catcher2.getEvents()); } + @Test public void unregisterCallback() { + final StringCatcher catcher1 = new StringCatcher(); + final StringCatcher catcher2 = new StringCatcher(); + Callback catcher1Callback = new Callback() { + @Override + public void call(String event) throws InvocationTargetException { + catcher1.hereHaveAString(event); + } + }; + Callback catcher2Callback = new Callback() { + @Override + public void call(String event) throws InvocationTargetException { + catcher2.hereHaveAString(event); + } + }; + try { + bus.unregister(String.class, catcher1Callback); + fail("Attempting to unregister an unregistered object succeeded"); + } catch (IllegalArgumentException expected) { + // OK. + } + + bus.register(String.class, catcher1Callback); + bus.post(EVENT); + bus.register(String.class, catcher2Callback); + bus.post(EVENT); + + List expectedEvents = new ArrayList(); + expectedEvents.add(EVENT); + expectedEvents.add(EVENT); + + assertEquals("Two correct events should be delivered.", + expectedEvents, catcher1.getEvents()); + + assertEquals("One correct event should be delivered.", + Arrays.asList(EVENT), catcher2.getEvents()); + + bus.unregister(String.class, catcher1Callback); + bus.post(EVENT); + + assertEquals("Shouldn't catch any more events when unregistered.", + expectedEvents, catcher1.getEvents()); + assertEquals("Two correct events should be delivered.", + expectedEvents, catcher2.getEvents()); + + try { + bus.unregister(String.class, catcher1Callback); + fail("Attempting to unregister an unregistered object succeeded"); + } catch (IllegalArgumentException expected) { + // OK. + } + + bus.unregister(String.class, catcher2Callback); + bus.post(EVENT); + assertEquals("Shouldn't catch any more events when unregistered.", + expectedEvents, catcher1.getEvents()); + assertEquals("Shouldn't catch any more events when unregistered.", + expectedEvents, catcher2.getEvents()); + } + @Test public void producingNullIsInvalid() { try { bus.register(new NullProducer()); diff --git a/library/src/test/java/com/squareup/otto/EventHandlerTest.java b/library/src/test/java/com/squareup/otto/EventHandlerTest.java index 98fd510..1cb901a 100644 --- a/library/src/test/java/com/squareup/otto/EventHandlerTest.java +++ b/library/src/test/java/com/squareup/otto/EventHandlerTest.java @@ -65,6 +65,16 @@ public class EventHandlerTest { } } + /** Checks that EventHandler's constructor disallows null callbacks. */ + @Test public void rejectionOfNullCallbacks() { + try { + new EventHandler(null); + fail("EventHandler must immediately reject null methods."); + } catch (NullPointerException expected) { + // Hooray! + } + } + /** Checks that EventHandler's constructor disallows null targets. */ @Test public void rejectionOfNullTargets() throws NoSuchMethodException { Method method = getRecordingMethod(); diff --git a/pom.xml b/pom.xml index 450c790..0f88464 100644 --- a/pom.xml +++ b/pom.xml @@ -25,7 +25,7 @@ com.squareup otto-parent - 1.3.5-SNAPSHOT + 1.3.6-SNAPSHOT pom Otto (Parent) diff --git a/sample/pom.xml b/sample/pom.xml index 5ac3560..3da5e19 100644 --- a/sample/pom.xml +++ b/sample/pom.xml @@ -20,7 +20,7 @@ com.squareup otto-parent - 1.3.5-SNAPSHOT + 1.3.6-SNAPSHOT ../pom.xml diff --git a/sample/src/main/java/com/squareup/otto/sample/LocationHistoryFragment.java b/sample/src/main/java/com/squareup/otto/sample/LocationHistoryFragment.java index ce44b42..857d24d 100644 --- a/sample/src/main/java/com/squareup/otto/sample/LocationHistoryFragment.java +++ b/sample/src/main/java/com/squareup/otto/sample/LocationHistoryFragment.java @@ -20,8 +20,10 @@ import android.support.v4.app.ListFragment; import android.view.View; import android.widget.ArrayAdapter; +import com.squareup.otto.Callback; import com.squareup.otto.Subscribe; +import java.lang.reflect.InvocationTargetException; import java.util.ArrayList; import java.util.List; @@ -29,15 +31,29 @@ public class LocationHistoryFragment extends ListFragment { private final List locationEvents = new ArrayList(); private ArrayAdapter adapter; + private Callback callback; @Override public void onResume() { super.onResume(); BusProvider.getInstance().register(this); + // Example of explicit registration of Callbacks + callback = new Callback() { + @Override + public void call(Object event) throws InvocationTargetException { + locationEvents.add(0, event.toString()); + if (adapter != null) { + adapter.notifyDataSetChanged(); + } + } + }; + BusProvider.getInstance().register(LocationChangedEvent.class, callback); } @Override public void onPause() { super.onPause(); BusProvider.getInstance().unregister(this); + // Example of explicit un-registration of Callbacks + BusProvider.getInstance().unregister(LocationHistoryFragment.class, callback); } @Override public void onViewCreated(View view, Bundle savedInstanceState) { @@ -46,13 +62,6 @@ public class LocationHistoryFragment extends ListFragment { setListAdapter(adapter); } - @Subscribe public void onLocationChanged(LocationChangedEvent event) { - locationEvents.add(0, event.toString()); - if (adapter != null) { - adapter.notifyDataSetChanged(); - } - } - @Subscribe public void onLocationCleared(LocationClearEvent event) { locationEvents.clear(); if (adapter != null) { From 2cce8a4b28a6c6ba6bb7cbcafa07ab501fa03e24 Mon Sep 17 00:00:00 2001 From: Guille Date: Thu, 29 May 2014 16:33:15 +0200 Subject: [PATCH 2/2] Lint fix on javadoc --- library/src/main/java/com/squareup/otto/Bus.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/src/main/java/com/squareup/otto/Bus.java b/library/src/main/java/com/squareup/otto/Bus.java index 8cc42c3..34cff88 100644 --- a/library/src/main/java/com/squareup/otto/Bus.java +++ b/library/src/main/java/com/squareup/otto/Bus.java @@ -321,7 +321,7 @@ public void unregister(Object object) { } /** - * Unregisters a Callback for a particular event type + * Unregisters a Callback for a particular event type. * * @param type event class to unsubscribe from. * @param callback Callback instance that would be executed when an event of the