Refactor: clean filter logic

Mage.Sets/src/mage/cards/b/BrutalExpulsion.java

@@ -10,7 +10,6 @@
 import mage.cards.CardSetInfo;
 import mage.constants.CardType;
 import mage.constants.Duration;
-import mage.filter.common.FilterCreaturePermanent;
 import mage.filter.common.FilterSpellOrPermanent;
 import mage.target.common.TargetCreatureOrPlaneswalker;
 import mage.target.common.TargetSpellOrPermanent;
@@ -26,7 +25,7 @@ public final class BrutalExpulsion extends CardImpl {
     private static final FilterSpellOrPermanent filter = new FilterSpellOrPermanent("spell or creature");
 
     static {
-        filter.setPermanentFilter(new FilterCreaturePermanent());
+        filter.getPermanentFilter().add(CardType.CREATURE.getPredicate());
     }
 
     public BrutalExpulsion(UUID ownerId, CardSetInfo setInfo) {

Mage.Sets/src/mage/cards/c/CommitMemory.java

@@ -1,7 +1,6 @@
 
 package mage.cards.c;
 
-import java.util.UUID;
 import mage.abilities.Ability;
 import mage.abilities.effects.Effect;
 import mage.abilities.effects.OneShotEffect;
@@ -13,24 +12,25 @@
 import mage.constants.CardType;
 import mage.constants.Outcome;
 import mage.constants.SpellAbilityType;
-import mage.filter.common.FilterNonlandPermanent;
 import mage.filter.common.FilterSpellOrPermanent;
+import mage.filter.predicate.Predicates;
 import mage.game.Game;
 import mage.game.permanent.Permanent;
 import mage.game.stack.Spell;
 import mage.players.Player;
 import mage.target.common.TargetSpellOrPermanent;
 
+import java.util.UUID;
+
 /**
- *
  * @author fireshoes
  */
 public final class CommitMemory extends SplitCard {
 
     private static final FilterSpellOrPermanent filter = new FilterSpellOrPermanent("spell or nonland permanent");
 
     static {
-        filter.setPermanentFilter(new FilterNonlandPermanent());
+        filter.getPermanentFilter().add(Predicates.not(CardType.LAND.getPredicate()));
     }
 
     public CommitMemory(UUID ownerId, CardSetInfo setInfo) {

Mage.Sets/src/mage/cards/u/Unsubstantiate.java

@@ -1,29 +1,28 @@
 
 package mage.cards.u;
 
-import java.util.UUID;
 import mage.abilities.effects.common.ReturnToHandTargetEffect;
 import mage.cards.CardImpl;
 import mage.cards.CardSetInfo;
 import mage.constants.CardType;
-import mage.filter.common.FilterCreaturePermanent;
 import mage.filter.common.FilterSpellOrPermanent;
 import mage.target.common.TargetSpellOrPermanent;
 
+import java.util.UUID;
+
 /**
- *
  * @author fireshoes
  */
 public final class Unsubstantiate extends CardImpl {
 
     private static final FilterSpellOrPermanent filter = new FilterSpellOrPermanent("spell or creature");
 
     static {
-        filter.setPermanentFilter(new FilterCreaturePermanent());
+        filter.getPermanentFilter().add(CardType.CREATURE.getPredicate());
     }
 
     public Unsubstantiate(UUID ownerId, CardSetInfo setInfo) {
-        super(ownerId,setInfo,new CardType[]{CardType.INSTANT},"{1}{U}");
+        super(ownerId, setInfo, new CardType[]{CardType.INSTANT}, "{1}{U}");
 
         // Return target spell or creature to its owner's hand.
         this.getSpellAbility().addTarget(new TargetSpellOrPermanent(1, 1, filter, false));

Mage/src/main/java/mage/filter/Filter.java

@@ -1,12 +1,15 @@
 package mage.filter;
 
+import mage.abilities.Ability;
+import mage.filter.predicate.ObjectSourcePlayerPredicate;
 import mage.filter.predicate.Predicate;
 import mage.game.Game;
 import mage.util.Copyable;
 
 import java.io.Serializable;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.UUID;
 
 /**
  * @param <E>
@@ -21,8 +24,25 @@ enum ComparisonScope {
 
     boolean match(E o, Game game);
 
+    boolean match(E object, UUID sourceControllerId, Ability source, Game game);
+
     Filter<E> add(Predicate<? super E> predicate);
 
+    /**
+     * Make sure on setting a new Filter that you overwrite this method
+     * and call Predicates.makeSurePredicateCompatibleWithFilter
+     * to check that the filter is able to process objects
+     * of the right kind. Helps with checks the Compiler can't do
+     * due to ObjectSourcePlayer casting in the this.match(4 arguments).
+     * <p>
+     * (method should then call this.addExtra(predicate) after verify checks)
+     */
+    Filter<E> add(ObjectSourcePlayerPredicate predicate);
+
+    // TODO: if someone can find a way to not have to add this
+    //       Compiler was confused between overloads
+    void addExtra(ObjectSourcePlayerPredicate predicate);
+
     boolean checkObjectClass(Object object);
 
     String getMessage();
@@ -31,13 +51,13 @@ enum ComparisonScope {
 
     Filter<E> copy();
 
-    public boolean isLockedFilter();
+    boolean isLockedFilter();
 
-    public void setLockedFilter(boolean lockedFilter);
+    void setLockedFilter(boolean lockedFilter);
 
     List<Predicate<? super E>> getPredicates();
 
-    default List<Predicate> getExtraPredicates() {
+    default List<ObjectSourcePlayerPredicate<E>> getExtraPredicates() {
         return new ArrayList<>();
     }
 }

Mage/src/main/java/mage/filter/FilterAbility.java

@@ -4,11 +4,13 @@
 import mage.abilities.Ability;
 import mage.constants.AbilityType;
 import mage.constants.Zone;
+import mage.filter.predicate.ObjectSourcePlayerPredicate;
 import mage.filter.predicate.Predicate;
+import mage.filter.predicate.Predicates;
 import mage.game.Game;
+import mage.game.stack.StackObject;
 
 /**
- *
  * @author North
  */
 public class FilterAbility extends FilterImpl<Ability> {
@@ -38,6 +40,14 @@ public static Predicate<Ability> type(AbilityType type) {
         return new AbilityTypePredicate(type);
     }
 
+    @Override
+    public FilterAbility add(ObjectSourcePlayerPredicate predicate) {
+        // Verify Checks
+        Predicates.makeSurePredicateCompatibleWithFilter(predicate, Ability.class, StackObject.class);
+        this.addExtra(predicate);
+        return this;
+    }
+
     @Override
     public boolean checkObjectClass(Object object) {
         return object instanceof Ability;

Mage/src/main/java/mage/filter/FilterCard.java

@@ -1,17 +1,13 @@
 package mage.filter;
 
-import mage.abilities.Ability;
 import mage.cards.Card;
 import mage.constants.TargetController;
-import mage.filter.predicate.ObjectSourcePlayer;
 import mage.filter.predicate.ObjectSourcePlayerPredicate;
 import mage.filter.predicate.Predicate;
 import mage.filter.predicate.Predicates;
-import mage.game.Game;
 
 import java.util.ArrayList;
 import java.util.List;
-import java.util.UUID;
 
 /**
  * Works with cards only. For objects like commanders you must override your canTarget method.
@@ -22,7 +18,6 @@
 public class FilterCard extends FilterObject<Card> {
 
     private static final long serialVersionUID = 1L;
-    protected final List<ObjectSourcePlayerPredicate<Card>> extraPredicates = new ArrayList<>();
 
     public FilterCard() {
         super("card");
@@ -34,71 +29,42 @@ public FilterCard(String name) {
 
     protected FilterCard(final FilterCard filter) {
         super(filter);
-        this.extraPredicates.addAll(filter.extraPredicates);
-    }
-
-    //20130711 708.6c
-    /* If anything performs a comparison involving multiple characteristics or
-     * values of one or more split cards in any zone other than the stack or
-     * involving multiple characteristics or values of one or more fused split
-     * spells, each characteristic or value is compared separately. If each of
-     * the individual comparisons would return a “yes” answer, the whole
-     * comparison returns a “yes” answer. The individual comparisons may involve
-     * different halves of the same split card.
-     */
-    @Override
-    public boolean match(Card card, Game game) {
-        if (card == null) {
-            return false;
-        }
-        return super.match(card, game);
-    }
-
-    public boolean match(Card card, UUID playerId, Ability source, Game game) {
-        if (!this.match(card, game)) {
-            return false;
-        }
-        ObjectSourcePlayer<Card> osp = new ObjectSourcePlayer<>(card, playerId, source);
-        return extraPredicates.stream().allMatch(p -> p.apply(osp, game));
-    }
-
-    public final void add(ObjectSourcePlayerPredicate predicate) {
-        if (isLockedFilter()) {
-            throw new UnsupportedOperationException("You may not modify a locked filter");
-        }
-
-        // verify check
-        checkPredicateIsSuitableForCardFilter(predicate);
-        Predicates.makeSurePredicateCompatibleWithFilter(predicate, Card.class);
-
-        extraPredicates.add(predicate);
-    }
-
-    public boolean hasPredicates() {
-        return !predicates.isEmpty() || !extraPredicates.isEmpty();
     }
 
     @Override
     public FilterCard copy() {
         return new FilterCard(this);
     }
 
-    @Override
-    public List<Predicate> getExtraPredicates() {
-        return new ArrayList<>(extraPredicates);
-    }
-
     public static void checkPredicateIsSuitableForCardFilter(Predicate predicate) {
-        // card filter can't contain controller predicate (only permanents on battlefield have controller)
+        // card filter can't contain controller predicate (only permanents on battlefield and StackObjects have controller)
         List<Predicate> list = new ArrayList<>();
         Predicates.collectAllComponents(predicate, list);
         if (list.stream().anyMatch(TargetController.ControllerPredicate.class::isInstance)) {
             throw new IllegalArgumentException("Wrong code usage: card filter doesn't support controller predicate");
         }
     }
 
+
     public FilterCard withMessage(String message) {
         this.setMessage(message);
         return this;
     }
+
+    @Override
+    public FilterCard add(ObjectSourcePlayerPredicate predicate) {
+        // verify checks
+        checkPredicateIsSuitableForCardFilter(predicate);
+        Predicates.makeSurePredicateCompatibleWithFilter(predicate, Card.class);
+        this.addExtra(predicate);
+        return this;
+    }
+
+    @Override
+    public boolean checkObjectClass(Object object) {
+        // TODO: investigate if we can/should exclude Permanent here.
+        //       as it does extend Card (if so do cleanup the
+        //       MultiFilterImpl that match Permanent and Card/Spell)
+        return object instanceof Card;
+    }
 }

Mage/src/main/java/mage/filter/FilterImpl.java

@@ -1,11 +1,15 @@
 package mage.filter;
 
+import mage.abilities.Ability;
+import mage.filter.predicate.ObjectSourcePlayer;
+import mage.filter.predicate.ObjectSourcePlayerPredicate;
 import mage.filter.predicate.Predicate;
 import mage.filter.predicate.Predicates;
 import mage.game.Game;
 
 import java.util.ArrayList;
 import java.util.List;
+import java.util.UUID;
 
 /**
  * @param <E>
@@ -14,7 +18,8 @@
  */
 public abstract class FilterImpl<E> implements Filter<E> {
 
-    protected List<Predicate<? super E>> predicates = new ArrayList<>();
+    private List<Predicate<? super E>> predicates = new ArrayList<>();
+    private List<ObjectSourcePlayerPredicate<E>> extraPredicates = new ArrayList<>();
     protected String message;
     protected boolean lockedFilter; // Helps to prevent "accidentally" modifying the StaticFilters objects
 
@@ -29,6 +34,7 @@ public FilterImpl(String name) {
     protected FilterImpl(final FilterImpl<E> filter) {
         this.message = filter.message;
         this.predicates = new ArrayList<>(filter.predicates);
+        this.extraPredicates = new ArrayList<>(filter.extraPredicates);
         this.lockedFilter = false;// After copying a filter it's allowed to modify
     }
 
@@ -41,25 +47,42 @@ public boolean match(E e, Game game) {
     }
 
     @Override
-    public final Filter<E> add(Predicate<? super E> predicate) {
-        if (isLockedFilter()) {
-            throw new UnsupportedOperationException("You may not modify a locked filter");
+    public boolean match(E object, UUID sourceControllerId, Ability source, Game game) {
+        if (!this.match(object, game)) {
+            return false;
         }
+        ObjectSourcePlayer osp = new ObjectSourcePlayer<>(object, sourceControllerId, source);
+        return extraPredicates.stream().allMatch(p -> p.apply(osp, game));
+    }
+
+    @Override
+    public Filter<E> add(Predicate<? super E> predicate) {
+        checkUnlockedFilter();
         predicates.add(predicate);
         return this;
     }
 
+    @Override
+    public final void addExtra(ObjectSourcePlayerPredicate predicate) {
+        checkUnlockedFilter();
+        extraPredicates.add(predicate);
+    }
+
     @Override
     public String getMessage() {
         return message;
     }
 
     @Override
     public final void setMessage(String message) {
+        checkUnlockedFilter();
+        this.message = message;
+    }
+
+    protected void checkUnlockedFilter() {
         if (isLockedFilter()) {
             throw new UnsupportedOperationException("You may not modify a locked filter");
         }
-        this.message = message;
     }
 
     @Override
@@ -80,4 +103,12 @@ public void setLockedFilter(boolean lockedFilter) {
     public List<Predicate<? super E>> getPredicates() {
         return predicates;
     }
+
+    public List<ObjectSourcePlayerPredicate<E>> getExtraPredicates() {
+        return new ArrayList<>(extraPredicates);
+    }
+
+    public boolean hasPredicates() {
+        return !predicates.isEmpty() || !extraPredicates.isEmpty();
+    }
 }