Fixes & improvements to action parsing

core/src/main/java/tc/oc/pgm/action/ActionParser.java

@@ -10,10 +10,12 @@
 import tc.oc.pgm.action.actions.KillEntitiesAction;
 import tc.oc.pgm.action.actions.ScopeSwitchAction;
 import tc.oc.pgm.action.actions.SetVariableAction;
+import tc.oc.pgm.api.feature.FeatureValidation;
 import tc.oc.pgm.api.filter.Filter;
 import tc.oc.pgm.api.filter.Filterables;
 import tc.oc.pgm.api.map.factory.MapFactory;
 import tc.oc.pgm.features.FeatureDefinitionContext;
+import tc.oc.pgm.features.XMLFeatureReference;
 import tc.oc.pgm.filters.Filterable;
 import tc.oc.pgm.filters.matcher.StaticFilter;
 import tc.oc.pgm.filters.parse.FilterParser;
@@ -48,23 +50,13 @@ public <B extends Filterable<?>> Action<? super B> parse(Element el, @Nullable C
       throws InvalidXMLException {
     String id = FeatureDefinitionContext.parseId(el);
 
+    Node node = new Node(el);
     if (id != null && maybeReference(el)) {
-      return parseReference(new Node(el), id, bound);
+      return parseReference(node, id, bound);
     }
 
     Action<? super B> result = parseDynamic(el, bound);
-
-    Class<?> childBound = result.getScope();
-    if (bound != null && !childBound.isAssignableFrom(bound)) {
-      throw new InvalidXMLException(
-          "Wrong trigger target, expected "
-              + bound.getSimpleName()
-              + " rather than "
-              + childBound.getSimpleName(),
-          new Node(el));
-    }
-
-    // We don't need to add references, they should already be added by whoever created them.
+    if (bound != null) validate(result, ActionScopeValidation.of(bound), node);
     if (result instanceof ActionDefinition)
       features.addFeature(el, (ActionDefinition<? super B>) result);
     return result;
@@ -75,7 +67,7 @@ public boolean isAction(Element el) {
   }
 
   private boolean maybeReference(Element el) {
-    return "trigger".equals(el.getName()) && el.getChildren().isEmpty();
+    return "action".equals(el.getName()) && el.getChildren().isEmpty();
   }
 
   public <B> Action<? super B> parseReference(Node node, Class<B> bound)
@@ -85,25 +77,41 @@ public <B> Action<? super B> parseReference(Node node, Class<B> bound)
 
   public <B> Action<? super B> parseReference(Node node, String id, Class<B> bound)
       throws InvalidXMLException {
-    return features.addReference(new XMLActionReference<>(factory.getFeatures(), node, id, bound));
+    Action<? super B> action = features.addReference(new XMLActionReference<>(features, node, id));
+    validate(action, ActionScopeValidation.of(bound), node);
+    return action;
+  }
+
+  @SuppressWarnings("rawtypes, unchecked")
+  public void validate(
+      Action<?> action, FeatureValidation<ActionDefinition<?>> validation, Node node)
+      throws InvalidXMLException {
+    if (action instanceof XMLFeatureReference) {
+      factory.getFeatures().validate((XMLFeatureReference) action, validation);
+    } else if (action instanceof ActionDefinition) {
+      factory.getFeatures().validate((ActionDefinition) action, validation, node);
+    } else {
+      throw new IllegalStateException(
+          "Attempted validation on an action which is neither definition nor reference.");
+    }
   }
 
   protected Method getParserFor(Element el) {
     return methodParsers.get(el.getName().toLowerCase());
   }
 
   @SuppressWarnings("unchecked")
-  private <T, B extends Filterable<?>> Action<T> parseDynamic(Element el, Class<B> bound)
+  private <T, B extends Filterable<?>> Action<T> parseDynamic(Element el, Class<B> scope)
       throws InvalidXMLException {
     Method parser = getParserFor(el);
     if (parser != null) {
       try {
-        return (Action<T>) parser.invoke(this, el, bound);
+        return (Action<T>) parser.invoke(this, el, scope);
       } catch (Exception e) {
         throw InvalidXMLException.coerce(e, new Node(el));
       }
     } else {
-      throw new InvalidXMLException("Unknown trigger type: " + el.getName(), el);
+      throw new InvalidXMLException("Unknown action type: " + el.getName(), el);
     }
   }
 
@@ -112,31 +120,48 @@ public <T extends Filterable<?>> Trigger<T> parseTrigger(Element el) throws Inva
     return new Trigger<>(
         cls,
         filters.parseReference(Node.fromRequiredAttr(el, "filter")),
-        parseReference(Node.fromRequiredAttr(el, "trigger"), cls));
+        parseReference(Node.fromRequiredAttr(el, "trigger", "action"), cls));
+  }
+
+  private <B extends Filterable<?>> Class<B> parseScope(Element el, Class<B> scope)
+      throws InvalidXMLException {
+    return parseScope(el, scope, "scope");
+  }
+
+  private <B extends Filterable<?>> Class<B> parseScope(Element el, Class<B> scope, String attr)
+      throws InvalidXMLException {
+    if (scope == null) return Filterables.parse(Node.fromRequiredAttr(el, attr));
+
+    Node node = Node.fromAttr(el, attr);
+    if (node != null && Filterables.parse(node) != scope)
+      throw new InvalidXMLException(
+          "Wrong scope defined for action, scope must be " + scope.getSimpleName(), el);
+    return scope;
   }
 
   @MethodParser("action")
-  public <B extends Filterable<?>> ActionDefinition<? super B> parseDefinition(
-      Element el, Class<B> bound) throws InvalidXMLException {
-    if (bound == null) bound = Filterables.parse(Node.fromRequiredAttr(el, "scope"));
+  public <B extends Filterable<?>> ActionNode<? super B> parseAction(Element el, Class<B> scope)
+      throws InvalidXMLException {
+    scope = parseScope(el, scope);
 
     ImmutableList.Builder<Action<? super B>> builder = ImmutableList.builder();
     for (Element child : el.getChildren()) {
-      builder.add(parse(child, bound));
+      builder.add(parse(child, scope));
     }
 
     Filter filter = filters.parseFilterProperty(el, "filter", StaticFilter.ALLOW);
+    Filter untriggerFilter = filters.parseFilterProperty(el, "untrigger-filter", StaticFilter.DENY);
 
-    return new ActionNode<>(builder.build(), filter, bound);
+    return new ActionNode<>(builder.build(), filter, untriggerFilter, scope);
   }
 
   @MethodParser("switch-scope")
   public <O extends Filterable<?>, I extends Filterable<?>> Action<? super O> parseSwitchScope(
       Element el, Class<O> outer) throws InvalidXMLException {
-    if (outer == null) outer = Filterables.parse(Node.fromRequiredAttr(el, "outer"));
-    Class<I> inner = Filterables.parse(Node.fromRequiredAttr(el, "inner"));
+    outer = parseScope(el, outer, "outer");
+    Class<I> inner = parseScope(el, null, "inner");
 
-    ActionDefinition<? super I> child = parseDefinition(el, inner);
+    ActionDefinition<? super I> child = parseAction(el, inner);
 
     Action<? super O> result = ScopeSwitchAction.of(child, outer, inner);
     if (result == null) {
@@ -161,7 +186,7 @@ public <T extends Filterable<?>> SetVariableAction<T> parseSetVariable(Element e
       throws InvalidXMLException {
     VariableDefinition<?> var =
         features.resolve(Node.fromRequiredAttr(el, "var"), VariableDefinition.class);
-    if (scope == null) scope = Filterables.parse(Node.fromRequiredAttr(el, "scope"));
+    scope = parseScope(el, scope);
 
     if (!Filterables.isAssignable(scope, var.getScope()))
       throw new InvalidXMLException(

core/src/main/java/tc/oc/pgm/action/ActionScopeValidation.java

@@ -0,0 +1,34 @@
+package tc.oc.pgm.action;
+
+import java.util.HashMap;
+import java.util.Map;
+import tc.oc.pgm.api.feature.FeatureValidation;
+import tc.oc.pgm.util.xml.InvalidXMLException;
+import tc.oc.pgm.util.xml.Node;
+
+public class ActionScopeValidation implements FeatureValidation<ActionDefinition<?>> {
+
+  private static final Map<Class<?>, ActionScopeValidation> INSTANCES = new HashMap<>();
+
+  public static ActionScopeValidation of(Class<?> scope) {
+    return INSTANCES.computeIfAbsent(scope, ActionScopeValidation::new);
+  }
+
+  private final Class<?> scope;
+
+  private ActionScopeValidation(Class<?> scope) {
+    this.scope = scope;
+  }
+
+  @Override
+  public void validate(ActionDefinition<?> definition, Node node) throws InvalidXMLException {
+    Class<?> scope = definition.getScope();
+    if (!scope.isAssignableFrom(this.scope))
+      throw new InvalidXMLException(
+          "Wrong action scope, got "
+              + scope.getSimpleName()
+              + " but expected "
+              + scope.getSimpleName(),
+          node);
+  }
+}

core/src/main/java/tc/oc/pgm/action/XMLActionReference.java

@@ -3,40 +3,14 @@
 import javax.annotation.Nullable;
 import tc.oc.pgm.features.FeatureDefinitionContext;
 import tc.oc.pgm.features.XMLFeatureReference;
-import tc.oc.pgm.util.xml.InvalidXMLException;
 import tc.oc.pgm.util.xml.Node;
 
 @SuppressWarnings({"rawtypes", "unchecked"})
 public class XMLActionReference<S> extends XMLFeatureReference<ActionDefinition>
     implements Action<S> {
 
-  private final Class<? super S> scope;
-
-  public XMLActionReference(
-      FeatureDefinitionContext context, Node node, @Nullable String id, Class<S> scope) {
+  public XMLActionReference(FeatureDefinitionContext context, Node node, @Nullable String id) {
     super(context, node, id, ActionDefinition.class);
-    this.scope = scope;
-  }
-
-  @Override
-  public void resolve() throws InvalidXMLException {
-    Node node = this.node; // In case we need to report an error
-
-    super.resolve();
-
-    if (super.referent == null) return;
-
-    Class<?> ref = super.referent.getScope();
-    if (!ref.isAssignableFrom(this.scope)) {
-      throw new InvalidXMLException(
-          "Wrong trigger target for ID '"
-              + id
-              + "': expected "
-              + scope.getSimpleName()
-              + " rather than "
-              + ref.getSimpleName(),
-          node);
-    }
   }
 
   @Override

core/src/main/java/tc/oc/pgm/action/actions/ActionNode.java

@@ -8,12 +8,18 @@
 public class ActionNode<B extends Filterable<?>> extends AbstractAction<B> {
   private final ImmutableList<Action<? super B>> actions;
   private final Filter filter;
+  private final Filter untrigerFilter;
   private final Class<B> bound;
 
-  public ActionNode(ImmutableList<Action<? super B>> actions, Filter filter, Class<B> bound) {
+  public ActionNode(
+      ImmutableList<Action<? super B>> actions,
+      Filter filter,
+      Filter untriggerFilter,
+      Class<B> bound) {
     super(bound);
     this.actions = actions;
     this.filter = filter;
+    this.untrigerFilter = untriggerFilter;
     this.bound = bound;
   }
 
@@ -30,4 +36,12 @@ public void trigger(B t) {
       }
     }
   }
+
+  public void untrigger(B t) {
+    if (untrigerFilter.query(t).isAllowed()) {
+      for (Action<? super B> action : actions) {
+        action.untrigger(t);
+      }
+    }
+  }
 }

core/src/main/java/tc/oc/pgm/filters/XMLFilterReference.java

@@ -15,17 +15,8 @@
 /** A {@link Filter} that delegates all methods to an XML reference */
 public class XMLFilterReference extends XMLFeatureReference<FilterDefinition> implements Filter {
 
-  public XMLFilterReference(
-      FeatureDefinitionContext context, Node node, Class<FilterDefinition> type) {
-    this(context, node, null, type);
-  }
-
-  public XMLFilterReference(
-      FeatureDefinitionContext context,
-      Node node,
-      @Nullable String id,
-      Class<FilterDefinition> type) {
-    super(context, node, id, type);
+  public XMLFilterReference(FeatureDefinitionContext context, Node node, @Nullable String id) {
+    super(context, node, id, FilterDefinition.class);
   }
 
   @Override

core/src/main/java/tc/oc/pgm/filters/parse/FeatureFilterParser.java

@@ -36,11 +36,8 @@ public Filter parse(Element el) throws InvalidXMLException {
   }
 
   @Override
-  public Filter parseReference(Node node, String value) throws InvalidXMLException {
-    return factory
-        .getFeatures()
-        .addReference(
-            new XMLFilterReference(factory.getFeatures(), node, value, FilterDefinition.class));
+  public Filter parseReference(Node node, String id) throws InvalidXMLException {
+    return features.addReference(new XMLFilterReference(factory.getFeatures(), node, id));
   }
 
   @Override

core/src/main/java/tc/oc/pgm/filters/parse/FilterParser.java

@@ -128,7 +128,7 @@ public Filter parsePropertyElement(Element property) throws InvalidXMLException
    * Return the filter referenced by the given name/id, and assume it appears in the given {@link
    * Node} for error reporting purposes.
    */
-  public abstract Filter parseReference(Node node, String value) throws InvalidXMLException;
+  public abstract Filter parseReference(Node node, String id) throws InvalidXMLException;
 
   public boolean isFilter(Element el) {
     return methodParsers.containsKey(el.getName()) || factory.getRegions().isRegion(el);

core/src/main/java/tc/oc/pgm/filters/parse/LegacyFilterParser.java

@@ -90,10 +90,10 @@ protected boolean isReference(Element el) {
   }
 
   @Override
-  public Filter parseReference(Node node, String value) throws InvalidXMLException {
-    Filter filter = this.filterContext.get(value);
+  public Filter parseReference(Node node, String id) throws InvalidXMLException {
+    Filter filter = this.filterContext.get(id);
     if (filter == null) {
-      throw new InvalidXMLException("No filter named '" + value + "'", node);
+      throw new InvalidXMLException("No filter named '" + id + "'", node);
     }
     return filter;
   }

core/src/main/java/tc/oc/pgm/kits/ActionKit.java

@@ -20,4 +20,11 @@ public void applyPostEvent(MatchPlayer player, boolean force, List<ItemStack> di
       t.trigger(player);
     }
   }
+
+  @Override
+  public void remove(MatchPlayer player) {
+    for (Action<? super MatchPlayer> t : actions) {
+      t.untrigger(player);
+    }
+  }
 }

core/src/main/java/tc/oc/pgm/kits/KitModule.java

@@ -10,6 +10,7 @@
 import org.bukkit.inventory.ItemStack;
 import org.jdom2.Document;
 import org.jdom2.Element;
+import tc.oc.pgm.action.ActionModule;
 import tc.oc.pgm.api.feature.FeatureDefinition;
 import tc.oc.pgm.api.filter.Filter;
 import tc.oc.pgm.api.map.MapModule;
@@ -54,7 +55,7 @@ public static class Factory implements MapModuleFactory<KitModule> {
 
     @Override
     public Collection<Class<? extends MapModule>> getWeakDependencies() {
-      return ImmutableList.of(TeamModule.class);
+      return ImmutableList.of(ActionModule.class, TeamModule.class);
     }
 
     @Override

core/src/main/java/tc/oc/pgm/regions/FeatureRegionParser.java

@@ -25,11 +25,11 @@ public Region parse(Element el) throws InvalidXMLException {
   }
 
   @Override
-  public Region parseReference(Node node, String val) throws InvalidXMLException {
+  public Region parseReference(Node node, String id) throws InvalidXMLException {
     return factory
         .getFeatures()
         .addReference(
-            new XMLRegionReference(factory.getFeatures(), node, val, RegionDefinition.class));
+            new XMLRegionReference(factory.getFeatures(), node, id, RegionDefinition.class));
   }
 
   @MethodParser("region")

core/src/main/java/tc/oc/pgm/regions/LegacyRegionParser.java

@@ -38,10 +38,10 @@ public Region parse(Element el) throws InvalidXMLException {
   }
 
   @Override
-  public Region parseReference(Node node, String name) throws InvalidXMLException {
-    Region region = this.regionContext.get(name);
+  public Region parseReference(Node node, String id) throws InvalidXMLException {
+    Region region = this.regionContext.get(id);
     if (region == null) {
-      throw new InvalidXMLException("Unknown region '" + name + "'", node);
+      throw new InvalidXMLException("Unknown region '" + id + "'", node);
     } else {
       return region;
     }

core/src/main/java/tc/oc/pgm/regions/RegionParser.java

@@ -39,7 +39,7 @@ public String type() {
    */
   public abstract Region parse(Element el) throws InvalidXMLException;
 
-  public abstract Region parseReference(Node node, String value) throws InvalidXMLException;
+  public abstract Region parseReference(Node node, String id) throws InvalidXMLException;
 
   public Region parseReference(Attribute attribute) throws InvalidXMLException {
     return parseReference(new Node(attribute));