Separate phase-related functionality in an EventPhase class

fabric-api-base/src/main/java/net/fabricmc/fabric/api/event/Event.java

@@ -20,6 +20,8 @@ import org.jetbrains.annotations.ApiStatus;
 
 import net.minecraft.util.Identifier;
 
+import net.fabricmc.fabric.impl.base.event.UnsupportedEventPhase;
+
 /**
  * Base class for Fabric's event implementations.
  *
@@ -50,42 +52,19 @@ public abstract class Event<T> {
 
 	/**
 	 * Register a listener to the event, in the default phase.
-	 * Have a look at {@link #addPhaseOrdering} for an explanation of event phases.
 	 *
 	 * @param listener The desired listener.
 	 */
-	public abstract void register(T listener);
-
-	/**
-	 * The identifier of the default phase.
-	 * Have a look at {@link EventFactory#createWithPhases} for an explanation of event phases.
-	 */
-	public static final Identifier DEFAULT_PHASE = new Identifier("fabric", "default");
-
-	/**
-	 * Register a listener to the event for the specified phase.
-	 * Have a look at {@link EventFactory#createWithPhases} for an explanation of event phases.
-	 *
-	 * @param phase Identifier of the phase this listener should be registered for. It will be created if it didn't exist yet.
-	 * @param listener The desired listener.
-	 */
-	public void register(Identifier phase, T listener) {
-		// This is done to keep compatibility with existing Event subclasses, but they should really not be subclassing Event.
-		register(listener);
+	public void register(T listener) {
+		phase(EventPhase.DEFAULT).register(listener);
 	}
 
 	/**
-	 * Request that listeners registered for one phase be executed before listeners registered for another phase.
-	 * Relying on the default phases supplied to {@link EventFactory#createWithPhases} should be preferred over manually
-	 * registering phase ordering dependencies.
-	 *
-	 * <p>Incompatible ordering constraints such as cycles will lead to inconsistent behavior:
-	 * some constraints will be respected and some will be ignored. If this happens, a warning will be logged.
-	 *
-	 * @param firstPhase The identifier of the phase that should run before the other. It will be created if it didn't exist yet.
-	 * @param secondPhase The identifier of the phase that should run after the other. It will be created if it didn't exist yet.
+	 * Retrieve a phase for this event, or create it if it didn't exist yet.
+	 * @see EventPhase
 	 */
-	public void addPhaseOrdering(Identifier firstPhase, Identifier secondPhase) {
-		// This is not abstract to avoid breaking existing Event subclasses, but they should really not be subclassing Event.
+	public EventPhase<T> phase(Identifier phaseIdentifier) {
+		// This is done to keep compatibility with existing Event subclasses, but they should really not be subclassing Event.
+		return new UnsupportedEventPhase<>(this);
 	}
 }

fabric-api-base/src/main/java/net/fabricmc/fabric/api/event/EventFactory.java

@@ -96,33 +96,24 @@ public final class EventFactory {
 	}
 
 	/**
-	 * Create an array-backed event with a list of default phases that get invoked in order.
+	 * Create an array-backed event with a list of default {@linkplain EventPhase phases} that get invoked in order.
 	 * Exposing the identifiers of the default phases as {@code public static final} constants is encouraged.
 	 *
-	 * <p>An event phase is a named group of listeners, which may be ordered before or after other groups of listeners.
-	 * This allows some listeners to take priority over other listeners.
-	 * Adding separate events should be considered before making use of multiple event phases.
-	 *
-	 * <p>Phases may be freely added to events created with any of the factory functions,
-	 * however using this function is preferred for widely used event phases.
-	 * If more phases are necessary, discussion with the author of the Event is encouraged.
-	 *
-	 * <p>Refer to {@link Event#addPhaseOrdering} for an explanation of event phases.
-	 *
 	 * @param type           The listener class type.
 	 * @param invokerFactory The invoker factory, combining multiple listeners into one instance.
-	 * @param defaultPhases  The default phases of this event, in the correct order. Must contain {@link Event#DEFAULT_PHASE}.
+	 * @param defaultPhases  The default phases of this event, in the correct order. Must contain {@link EventPhase#DEFAULT}.
 	 * @param <T>            The listener type.
 	 * @return The Event instance.
+	 * @see EventPhase
 	 */
 	public static <T> Event<T> createWithPhases(Class<? super T> type, Function<T[], T> invokerFactory, Identifier... defaultPhases) {
-		EventFactoryImpl.ensureContainsDefault(defaultPhases);
-		EventFactoryImpl.ensureNoDuplicates(defaultPhases);
+		if (!EventFactoryImpl.contains(defaultPhases, EventPhase.DEFAULT)) throw new IllegalArgumentException("The default phases must contain EventPhase.DEFAULT");
+		EventFactoryImpl.ensureNoDuplicatesNoNull(defaultPhases);
 
 		Event<T> event = createArrayBacked(type, invokerFactory);
 
 		for (int i = 1; i < defaultPhases.length; ++i) {
-			event.addPhaseOrdering(defaultPhases[i-1], defaultPhases[i]);
+			event.phase(defaultPhases[i-1]).runBefore(defaultPhases[i]);
 		}
 
 		return event;

fabric-api-base/src/main/java/net/fabricmc/fabric/api/event/EventPhase.java

@@ -0,0 +1,61 @@
+/*
+ * Copyright (c) 2016, 2017, 2018, 2019 FabricMC
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package net.fabricmc.fabric.api.event;
+
+import net.minecraft.util.Identifier;
+
+/**
+ * An event phase is a named group of event listeners, which may be ordered before or after other groups of listeners.
+ * This allows some listeners to take priority over other listeners.
+ * Adding separate events should be considered before making use of multiple event phases.
+ *
+ * <p>Phases may be freely added to any event, however using the default phases passed to {@link EventFactory#createWithPhases}
+ * is preferred to manually adding them. If more phases are necessary, discussion with the author of the Event is encouraged.
+ */
+public interface EventPhase<T> {
+	/**
+	 * The identifier of the default phase.
+	 */
+	Identifier DEFAULT = new Identifier("fabric", "default");
+
+	/**
+	 * Register a listener to this event phase.
+	 *
+	 * @param listener The desired listener.
+	 */
+	void register(T listener);
+
+	/**
+	 * Request that listeners registered to this phase be ran before listeners registered to some other phases.
+	 * Relying on the default phases supplied to {@link EventFactory#createWithPhases} should be preferred over manually
+	 * registering phase ordering dependencies.
+	 *
+	 * <p>Incompatible ordering constraints such as cycles will lead to inconsistent ordering of listeners inside the cycle.
+	 * If this happens, a warning will be logged.
+	 *
+	 * @param subsequentPhases Identifiers of the phases that should run AFTER this one.
+	 */
+	void runBefore(Identifier... subsequentPhases);
+
+	/**
+	 * Request that listeners registered to this phase be ran after listeners registered to some other phases.
+	 *
+	 * @param previousPhases Identifiers of the phases that should run BEFORE this one.
+	 * @see #runBefore
+	 */
+	void runAfter(Identifier... previousPhases);
+}

fabric-api-base/src/main/java/net/fabricmc/fabric/impl/base/event/ArrayBackedEvent.java

@@ -32,6 +32,7 @@ import org.apache.logging.log4j.Logger;
 import net.minecraft.util.Identifier;
 
 import net.fabricmc.fabric.api.event.Event;
+import net.fabricmc.fabric.api.event.EventPhase;
 
 class ArrayBackedEvent<T> extends Event<T> {
 	private static final Logger LOGGER = LogManager.getLogger("fabric-api-base");
@@ -42,11 +43,11 @@ class ArrayBackedEvent<T> extends Event<T> {
 	/**
 	 * Registered event phases.
 	 */
-	private final Map<Identifier, EventPhaseData<T>> phases = new LinkedHashMap<>();
+	private final Map<Identifier, EventPhaseData> phases = new LinkedHashMap<>();
 	/**
 	 * Phases sorted in the correct dependency order.
 	 */
-	private final List<EventPhaseData<T>> sortedPhases = new ArrayList<>();
+	private final List<EventPhaseData> sortedPhases = new ArrayList<>();
 
 	@SuppressWarnings("unchecked")
 	ArrayBackedEvent(Class<? super T> type, Function<T[], T> invokerFactory) {
@@ -60,26 +61,19 @@ class ArrayBackedEvent<T> extends Event<T> {
 	}
 
 	@Override
-	public void register(T listener) {
-		register(DEFAULT_PHASE, listener);
-	}
-
-	@Override
-	public void register(Identifier phaseIdentifier, T listener) {
-		Objects.requireNonNull(phaseIdentifier, "Tried to register a listener for a null phase!");
-		Objects.requireNonNull(listener, "Tried to register a null listener!");
+	public EventPhase<T> phase(Identifier phaseIdentifier) {
+		Objects.requireNonNull(phaseIdentifier, "Tried to retrieve a null phase.");
 
 		synchronized (lock) {
-			getOrCreatePhase(phaseIdentifier).addListener(listener);
-			rebuildInvoker(handlers.length + 1);
+			return getOrCreatePhase(phaseIdentifier);
 		}
 	}
 
-	private EventPhaseData<T> getOrCreatePhase(Identifier id) {
-		EventPhaseData<T> phase = phases.get(id);
+	private EventPhaseData getOrCreatePhase(Identifier id) {
+		EventPhaseData phase = phases.get(id);
 
 		if (phase == null) {
-			phase = new EventPhaseData<>(id, handlers.getClass().getComponentType());
+			phase = new EventPhaseData(id, handlers.getClass().getComponentType());
 			phases.put(id, phase);
 			sortedPhases.add(phase);
 		}
@@ -97,7 +91,7 @@ class ArrayBackedEvent<T> extends Event<T> {
 			T[] newHandlers = (T[]) Array.newInstance(handlers.getClass().getComponentType(), newLength);
 			int newHandlersIndex = 0;
 
-			for (EventPhaseData<T> existingPhase : sortedPhases) {
+			for (EventPhaseData existingPhase : sortedPhases) {
 				for (T handler : existingPhase.listeners) {
 					newHandlers[newHandlersIndex++] = handler;
 				}
@@ -110,22 +104,6 @@ class ArrayBackedEvent<T> extends Event<T> {
 		update();
 	}
 
-	@Override
-	public void addPhaseOrdering(Identifier firstPhase, Identifier secondPhase) {
-		Objects.requireNonNull(firstPhase, "Tried to add an ordering for a null phase.");
-		Objects.requireNonNull(secondPhase, "Tried to add an ordering for a null phase.");
-		if (firstPhase.equals(secondPhase)) throw new IllegalArgumentException("Tried to add a phase that depends on itself.");
-
-		synchronized (lock) {
-			EventPhaseData<T> first = getOrCreatePhase(firstPhase);
-			EventPhaseData<T> second = getOrCreatePhase(secondPhase);
-			first.subsequentPhases.add(second);
-			second.previousPhases.add(first);
-			sortPhases();
-			rebuildInvoker(handlers.length);
-		}
-	}
-
 	/**
 	 * Uses a modified Kosaraju SCC to sort the phases.
 	 */
@@ -133,9 +111,9 @@ class ArrayBackedEvent<T> extends Event<T> {
 		sortedPhases.clear();
 
 		// FIRST VISIT
-		List<EventPhaseData<T>> toposort = new ArrayList<>(phases.size());
+		List<EventPhaseData> toposort = new ArrayList<>(phases.size());
 
-		for (EventPhaseData<T> phase : phases.values()) {
+		for (EventPhaseData phase : phases.values()) {
 			forwardVisit(phase, null, toposort);
 		}
 
@@ -143,19 +121,19 @@ class ArrayBackedEvent<T> extends Event<T> {
 		Collections.reverse(toposort);
 
 		// SECOND VISIT
-		for (EventPhaseData<T> phase : toposort) {
+		for (EventPhaseData phase : toposort) {
 			backwardVisit(phase);
 		}
 
 		clearStatus(toposort);
 	}
 
-	private void forwardVisit(EventPhaseData<T> phase, EventPhaseData<T> parent, List<EventPhaseData<T>> toposort) {
+	private void forwardVisit(EventPhaseData phase, EventPhaseData parent, List<EventPhaseData> toposort) {
 		if (phase.visitStatus == 0) {
 			// Not yet visited.
 			phase.visitStatus = 1;
 
-			for (EventPhaseData<T> data : phase.subsequentPhases) {
+			for (EventPhaseData data : phase.subsequentPhases) {
 				forwardVisit(data, phase, toposort);
 			}
 
@@ -171,28 +149,28 @@ class ArrayBackedEvent<T> extends Event<T> {
 		}
 	}
 
-	private void clearStatus(List<EventPhaseData<T>> phases) {
-		for (EventPhaseData<T> phase : phases) {
+	private void clearStatus(List<EventPhaseData> phases) {
+		for (EventPhaseData phase : phases) {
 			phase.visitStatus = 0;
 		}
 	}
 
-	private void backwardVisit(EventPhaseData<T> phase) {
+	private void backwardVisit(EventPhaseData phase) {
 		if (phase.visitStatus == 0) {
 			phase.visitStatus = 1;
 			sortedPhases.add(phase);
 
-			for (EventPhaseData<T> data : phase.previousPhases) {
+			for (EventPhaseData data : phase.previousPhases) {
 				backwardVisit(data);
 			}
 		}
 	}
 
-	private static class EventPhaseData<T> {
+	private class EventPhaseData implements EventPhase<T> {
 		final Identifier id;
 		T[] listeners;
-		final List<EventPhaseData<T>> subsequentPhases = new ArrayList<>();
-		final List<EventPhaseData<T>> previousPhases = new ArrayList<>();
+		final List<EventPhaseData> subsequentPhases = new ArrayList<>();
+		final List<EventPhaseData> previousPhases = new ArrayList<>();
 		int visitStatus = 0; // 0: not visited, 1: visiting, 2: visited
 
 		@SuppressWarnings("unchecked")
@@ -206,5 +184,51 @@ class ArrayBackedEvent<T> extends Event<T> {
 			listeners = Arrays.copyOf(listeners, oldLength + 1);
 			listeners[oldLength] = listener;
 		}
+
+		@Override
+		public void register(T listener) {
+			Objects.requireNonNull(listener, "Tried to register a null listener!");
+
+			synchronized (lock) {
+				addListener(listener);
+				rebuildInvoker(handlers.length + 1);
+			}
+		}
+
+		@Override
+		public void runBefore(Identifier... subsequentPhases) {
+			EventFactoryImpl.ensureNoDuplicatesNoNull(subsequentPhases);
+			if (subsequentPhases.length == 0) throw new IllegalArgumentException("Must register at least one subsequent phase.");
+			if (EventFactoryImpl.contains(subsequentPhases, id)) throw new IllegalArgumentException("Event phase may not depend on itself.");
+
+			synchronized (lock) {
+				for (Identifier other : subsequentPhases) {
+					EventPhaseData second = getOrCreatePhase(other);
+					this.subsequentPhases.add(second);
+					second.previousPhases.add(this);
+				}
+
+				sortPhases();
+				rebuildInvoker(handlers.length);
+			}
+		}
+
+		@Override
+		public void runAfter(Identifier... previousPhases) {
+			EventFactoryImpl.ensureNoDuplicatesNoNull(previousPhases);
+			if (previousPhases.length == 0) throw new IllegalArgumentException("Must register at least one previous phase.");
+			if (EventFactoryImpl.contains(previousPhases, id)) throw new IllegalArgumentException("Event phase may not depend on itself.");
+
+			synchronized (lock) {
+				for (Identifier other : previousPhases) {
+					EventPhaseData second = getOrCreatePhase(other);
+					this.previousPhases.add(second);
+					second.subsequentPhases.add(this);
+				}
+
+				sortPhases();
+				rebuildInvoker(handlers.length);
+			}
+		}
 	}
 }

fabric-api-base/src/main/java/net/fabricmc/fabric/impl/base/event/EventFactoryImpl.java

@@ -25,6 +25,7 @@ import java.lang.reflect.Modifier;
 import java.lang.reflect.Proxy;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Objects;
 import java.util.function.Function;
 
 import net.minecraft.util.Identifier;
@@ -46,21 +47,23 @@ public final class EventFactoryImpl {
 		return event;
 	}
 
-	public static void ensureContainsDefault(Identifier[] defaultPhases) {
-		for (Identifier id : defaultPhases) {
-			if (id.equals(Event.DEFAULT_PHASE)) {
-				return;
+	public static boolean contains(Identifier[] phases, Identifier query) {
+		for (Identifier id : phases) {
+			if (id.equals(query)) {
+				return true;
 			}
 		}
 
-		throw new IllegalArgumentException("The event phases must contain Event.DEFAULT_PHASE.");
+		return false;
 	}
 
-	public static void ensureNoDuplicates(Identifier[] defaultPhases) {
-		for (int i = 0; i < defaultPhases.length; ++i) {
-			for (int j = i+1; j < defaultPhases.length; ++j) {
-				if (defaultPhases[i].equals(defaultPhases[j])) {
-					throw new IllegalArgumentException("Duplicate event phase: " + defaultPhases[i]);
+	public static void ensureNoDuplicatesNoNull(Identifier[] phases) {
+		for (int i = 0; i < phases.length; ++i) {
+			Objects.requireNonNull(phases[i], "Encountered null phase.");
+
+			for (int j = i+1; j < phases.length; ++j) {
+				if (phases[i].equals(phases[j])) {
+					throw new IllegalArgumentException("Duplicate event phase: " + phases[i]);
 				}
 			}
 		}

fabric-api-base/src/main/java/net/fabricmc/fabric/impl/base/event/UnsupportedEventPhase.java

@@ -0,0 +1,46 @@
+/*
+ * Copyright (c) 2016, 2017, 2018, 2019 FabricMC
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package net.fabricmc.fabric.impl.base.event;
+
+import net.minecraft.util.Identifier;
+
+import net.fabricmc.fabric.api.event.Event;
+import net.fabricmc.fabric.api.event.EventPhase;
+
+/**
+ * Base phase implementation for foreign events that don't support phases.
+ */
+public class UnsupportedEventPhase<T> implements EventPhase<T> {
+	private final Event<T> event;
+
+	public UnsupportedEventPhase(Event<T> event) {
+		this.event = event;
+	}
+
+	@Override
+	public void register(T listener) {
+		event.register(listener);
+	}
+
+	@Override
+	public void runBefore(Identifier... subsequentPhases) {
+	}
+
+	@Override
+	public void runAfter(Identifier... previousPhases) {
+	}
+}

fabric-api-base/src/testmod/java/net/fabricmc/fabric/test/base/EventTests.java

@@ -25,6 +25,7 @@ import net.minecraft.util.Identifier;
 
 import net.fabricmc.fabric.api.event.Event;
 import net.fabricmc.fabric.api.event.EventFactory;
+import net.fabricmc.fabric.api.event.EventPhase;
 
 public class EventTests {
 	public static void run() {
@@ -58,7 +59,7 @@ public class EventTests {
 		Event<Test> event = createEvent();
 
 		event.register(ensureOrder(0));
-		event.register(Event.DEFAULT_PHASE, ensureOrder(1));
+		event.phase(EventPhase.DEFAULT).register(ensureOrder(1));
 		event.register(ensureOrder(2));
 
 		event.invoker().onTest();
@@ -69,11 +70,11 @@ public class EventTests {
 	private static void testMultipleDefaultPhases() {
 		Identifier first = new Identifier("fabric", "first");
 		Identifier second = new Identifier("fabric", "second");
-		Event<Test> event = EventFactory.createWithPhases(Test.class, INVOKER_FACTORY, first, second, Event.DEFAULT_PHASE);
+		Event<Test> event = EventFactory.createWithPhases(Test.class, INVOKER_FACTORY, first, second, EventPhase.DEFAULT);
 
-		event.register(second, ensureOrder(1));
+		event.phase(second).register(ensureOrder(1));
 		event.register(ensureOrder(2));
-		event.register(first, ensureOrder(0));
+		event.phase(first).register(ensureOrder(0));
 
 		for (int i = 0; i < 5; ++i) {
 			event.invoker().onTest();
@@ -90,21 +91,21 @@ public class EventTests {
 		Identifier late = new Identifier("fabric", "late");
 		Identifier veryLate = new Identifier("fabric", "very_late");
 
-		event.addPhaseOrdering(veryEarly, early);
-		event.addPhaseOrdering(early, Event.DEFAULT_PHASE);
-		event.addPhaseOrdering(Event.DEFAULT_PHASE, late);
-		event.addPhaseOrdering(late, veryLate);
+		event.phase(veryEarly).runBefore(early);
+		event.phase(early).runBefore(EventPhase.DEFAULT);
+		event.phase(late).runAfter(EventPhase.DEFAULT);
+		event.phase(late).runBefore(veryLate);
 
 		event.register(ensureOrder(4));
 		event.register(ensureOrder(5));
-		event.register(veryEarly, ensureOrder(0));
-		event.register(early, ensureOrder(2));
-		event.register(late, ensureOrder(6));
-		event.register(veryLate, ensureOrder(8));
-		event.register(veryEarly, ensureOrder(1));
-		event.register(veryLate, ensureOrder(9));
-		event.register(late, ensureOrder(7));
-		event.register(early, ensureOrder(3));
+		event.phase(veryEarly).register(ensureOrder(0));
+		event.phase(early).register(ensureOrder(2));
+		event.phase(late).register(ensureOrder(6));
+		event.phase(veryLate).register(ensureOrder(8));
+		event.phase(veryEarly).register(ensureOrder(1));
+		event.phase(veryLate).register(ensureOrder(9));
+		event.phase(late).register(ensureOrder(7));
+		event.phase(early).register(ensureOrder(3));
 
 		for (int i = 0; i < 5; ++i) {
 			event.invoker().onTest();
@@ -120,23 +121,23 @@ public class EventTests {
 		Identifier b1 = new Identifier("fabric", "b1");
 		Identifier b2 = new Identifier("fabric", "b2");
 		Identifier b3 = new Identifier("fabric", "b3");
-		Identifier c = Event.DEFAULT_PHASE;
+		Identifier c = EventPhase.DEFAULT;
 
 		// A always first and C always last.
-		event.register(a, ensureOrder(0));
-		event.register(c, ensureOrder(4));
-		event.register(b1, ensureOrder(1));
-		event.register(b1, ensureOrder(2));
-		event.register(b1, ensureOrder(3));
+		event.phase(a).register(ensureOrder(0));
+		event.phase(c).register(ensureOrder(4));
+		event.phase(b1).register(ensureOrder(1));
+		event.phase(b1).register(ensureOrder(2));
+		event.phase(b1).register(ensureOrder(3));
 
 		// A -> B
-		event.addPhaseOrdering(a, b1);
+		event.phase(a).runBefore(b1);
 		// B -> C
-		event.addPhaseOrdering(b3, c);
+		event.phase(b3).runBefore(c);
 		// loop
-		event.addPhaseOrdering(b1, b2);
-		event.addPhaseOrdering(b2, b3);
-		event.addPhaseOrdering(b3, b1);
+		event.phase(b1).runBefore(b2);
+		event.phase(b2).runBefore(b3);
+		event.phase(b3).runBefore(b1);
 
 		for (int i = 0; i < 5; ++i) {
 			event.invoker().onTest();