CopySpellAbilityEffect refactored: fixed Precursor Golem (and hopefully all similiar spells), removed calls to isHuman/isComputer

PlayerController: added chooseSingleSpellForEffect

src/main/java/forge/card/ability/SpellAbilityAi.java

@@ -122,4 +122,9 @@ public abstract class SpellAbilityAi extends SaTargetRountines {
         System.err.println("Warning: default (ie. inherited from base class) implementation of chooseSinglePlayer is used for " + this.getClass().getName() + ". Consider declaring an overloaded method");
         return options.get(0);
     }
+
+    public SpellAbility chooseSingleSpellAbility(Player player, SpellAbility sa, List<SpellAbility> spells) {
+        System.err.println("Warning: default (ie. inherited from base class) implementation of chooseSingleSpellAbility is used for " + this.getClass().getName() + ". Consider declaring an overloaded method");
+        return spells.get(0);
+    }
 }

src/main/java/forge/card/ability/effects/CopySpellAbilityEffect.java

@@ -4,6 +4,8 @@ import java.util.ArrayList;
 import java.util.Iterator;
 import java.util.List;
 
+import com.google.common.collect.Iterables;
+
 import forge.Card;
 import forge.CardLists;
 import forge.ITargetable;
@@ -13,6 +15,7 @@ import forge.card.cardfactory.CardFactory;
 import forge.card.spellability.SpellAbility;
 import forge.game.player.Player;
 import forge.gui.GuiChoose;
+import forge.util.Lang;
 
 public class CopySpellAbilityEffect extends SpellAbilityEffect {
 
@@ -66,48 +69,23 @@ public class CopySpellAbilityEffect extends SpellAbilityEffect {
             return;
         }
 
+        boolean mayChoseNewTargets = true;
+        List<SpellAbility> copies = new ArrayList<SpellAbility>();
+        
         if (sa.hasParam("CopyMultipleSpells")) {
             final int spellCount = Integer.parseInt(sa.getParam("CopyMultipleSpells"));
             ArrayList<SpellAbility> chosenSAs = new ArrayList<SpellAbility>();
-            SpellAbility chosenSAtmp = null;
-            for (int multi = 0; multi < spellCount; multi++) {
-                if (tgtSpells.size() == 1) {
-                    chosenSAs.addAll(tgtSpells);
-                } else if (sa.getActivatingPlayer().isHuman()) {
-                    String num = "";
-                    if (multi == 1 - 1) {
-                        num = "first";
-                    }
-                    else if (multi == 2 - 1) {
-                        num = "second";
-                    }
-                    else if (multi == 3 - 1) {
-                        num = "third";
-                    } else {
-                        num = Integer.toString(multi - 1) + "th";
-                    }
-                    chosenSAtmp = GuiChoose.one("Select " + num + " spell to copy to stack", tgtSpells);
-                    chosenSAs.add(chosenSAtmp);
-                    tgtSpells.remove(chosenSAtmp);
-                } else {
-                    chosenSAs.add(tgtSpells.get(multi));
-                }
-            }
 
-            for (final SpellAbility chosenSAcopy : chosenSAs) {
-                chosenSAcopy.setActivatingPlayer(controller);
-                for (int i = 0; i < amount; i++) {
-                    CardFactory.copySpellontoStack(card, chosenSAcopy.getSourceCard(), chosenSAcopy, true, null);
-                }
+            for (int multi = 0; multi < spellCount && !tgtSpells.isEmpty(); multi++) {
+                String prompt = "Select " + Lang.getOrdinal(multi) + " spell to copy to stack";
+                SpellAbility chosen = controller.getController().chooseSingleSpellForEffect(tgtSpells, sa, prompt);
+                copies.add(CardFactory.copySpellAbilityAndSrcCard(card, chosen.getSourceCard(), chosen, true));
+                chosenSAs.add(chosen);
+                tgtSpells.remove(chosen);
             }
         }
         else if (sa.hasParam("CopyForEachCanTarget")) {
-            SpellAbility chosenSA = null;
-            if (tgtSpells.size() == 1 || sa.getActivatingPlayer().isComputer()) {
-                chosenSA = tgtSpells.get(0);
-            } else {
-                chosenSA = GuiChoose.one("Select a spell to copy", tgtSpells);
-            }
+            SpellAbility chosenSA = controller.getController().chooseSingleSpellForEffect(tgtSpells, sa, "Select a spell to copy");
             chosenSA.setActivatingPlayer(controller);
             List<ITargetable> candidates = chosenSA.getTargetRestrictions().getAllCandidates(chosenSA, true);
             if (sa.hasParam("CanTargetPlayer")) {
@@ -115,8 +93,13 @@ public class CopySpellAbilityEffect extends SpellAbilityEffect {
                 // Remove targeted players because getAllCandidates include all the valid players
                 for(Player p : chosenSA.getTargets().getTargetPlayers())
                     candidates.remove(p);
+                
+                mayChoseNewTargets = false;
                 for (ITargetable o : candidates) {
-                    CardFactory.copySpellontoStack(card, chosenSA.getSourceCard(), chosenSA, true, o);
+                    SpellAbility copy = CardFactory.copySpellAbilityAndSrcCard(card, chosenSA.getSourceCard(), chosenSA, true);
+                    copy.resetTargets();
+                    copy.getTargets().add(o);
+                    copies.add(copy);
                 }
             } else {// Precursor Golem, Ink-Treader Nephilim
                 final String type = sa.getParam("CopyForEachCanTarget");
@@ -126,28 +109,29 @@ public class CopySpellAbilityEffect extends SpellAbilityEffect {
                         valid.add((Card) o);
                     }
                 }
-                valid = CardLists.getValidCards(valid, type, chosenSA.getActivatingPlayer(),
-                        chosenSA.getSourceCard());
+                valid = CardLists.getValidCards(valid, type, chosenSA.getActivatingPlayer(), chosenSA.getSourceCard());
+                Card originalTarget = Iterables.getFirst(getTargetCards(chosenSA), null);
+                valid.remove(originalTarget);
+                mayChoseNewTargets = false;
                 for (Card c : valid) {
-                    CardFactory.copySpellontoStack(card, chosenSA.getSourceCard(), chosenSA, true, c);
+                    SpellAbility copy = CardFactory.copySpellAbilityAndSrcCard(card, chosenSA.getSourceCard(), chosenSA, true);
+                    copy.resetTargets();
+                    copy.getTargets().add(c);
+                    copies.add(copy);
                 }
             }
         }
         else {
-            SpellAbility chosenSA = null;
-            if (tgtSpells.size() == 1) {
-                chosenSA = tgtSpells.get(0);
-            } else if (sa.getActivatingPlayer().isHuman()) {
-                chosenSA = GuiChoose.one("Select a spell to copy", tgtSpells);
-            } else {
-                chosenSA = tgtSpells.get(0);
-            }
-
+            SpellAbility chosenSA = controller.getController().chooseSingleSpellForEffect(tgtSpells, sa, "Select a spell to copy");
             chosenSA.setActivatingPlayer(controller);
             for (int i = 0; i < amount; i++) {
-                CardFactory.copySpellontoStack(card, chosenSA.getSourceCard(), chosenSA, true, null);
+                copies.add(CardFactory.copySpellAbilityAndSrcCard(card, chosenSA.getSourceCard(), chosenSA, true));
             }
         }
+        
+        for(SpellAbility copySA : copies) {
+            controller.getController().playSpellAbilityForFree(copySA, mayChoseNewTargets);
+        }
     } // end resolve
 
 }

src/main/java/forge/card/cardfactory/CardFactory.java

@@ -138,8 +138,7 @@ public class CardFactory {
      * @param bCopyDetails
      *            a boolean.
      */
-    public final static void copySpellontoStack(final Card source, final Card original, final SpellAbility sa,
-            final boolean bCopyDetails, final ITargetable definedTarget) {
+    public final static SpellAbility copySpellAbilityAndSrcCard(final Card source, final Card original, final SpellAbility sa, final boolean bCopyDetails) {
         //Player originalController = original.getController();
         Player controller = sa.getActivatingPlayer();
         final Card c = copyCard(original);
@@ -204,10 +203,7 @@ public class CardFactory {
             }
             copySA.setPaidHash(sa.getPaidHash());
         }
-
-        controller.getController().playSpellAbilityForFree(copySA);
-
-        //c.addController(originalController);
+        return copySA;
     }
 
     /**

src/main/java/forge/game/ai/ComputerUtil.java

@@ -264,7 +264,7 @@ public class ComputerUtil {
      * @param sa
      *            a {@link forge.card.spellability.SpellAbility} object.
      */
-    public static final void playStackFree(final Player ai, final SpellAbility sa) {
+    public static final void playSpellAbilityForFree(final Player ai, final SpellAbility sa) {
         sa.setActivatingPlayer(ai);
 
         final Card source = sa.getSourceCard();

src/main/java/forge/game/player/HumanPlay.java

@@ -184,7 +184,7 @@ public class HumanPlay {
     
         if (sa != null) {
             sa.setActivatingPlayer(player);
-            playSaWithoutPayingManaCost(player.getGame(), sa);
+            playSaWithoutPayingManaCost(player.getGame(), sa, true);
         }
     }
 
@@ -196,7 +196,7 @@ public class HumanPlay {
      * @param sa
      *            a {@link forge.card.spellability.SpellAbility} object.
      */
-    public static final void playSaWithoutPayingManaCost(final Game game, final SpellAbility sa) {
+    public static final void playSaWithoutPayingManaCost(final Game game, final SpellAbility sa, boolean mayChooseNewTargets) {
         FThreads.assertExecutedByEdt(false);
         final Card source = sa.getSourceCard();
         
@@ -209,7 +209,7 @@ public class HumanPlay {
             final CostPayment payment = new CostPayment(sa.getPayCosts(), sa);
     
             final HumanPlaySpellAbility req = new HumanPlaySpellAbility(sa, payment);
-            req.fillRequirements(false, true, false);
+            req.fillRequirements(!mayChooseNewTargets, true, false);
         } else {
             if (sa.isSpell()) {
                 final Card c = sa.getSourceCard();

src/main/java/forge/game/player/PlayerController.java

@@ -94,7 +94,7 @@ public abstract class PlayerController {
      */
     //public abstract void playFromSuspend(Card c);
     public abstract boolean playCascade(Card cascadedCard, Card sourceCard);
-    public abstract void playSpellAbilityForFree(SpellAbility copySA);
+    public abstract void playSpellAbilityForFree(SpellAbility copySA, boolean mayChoseNewTargets);
 
     public abstract Deck sideboard(final Deck deck, GameType gameType);
 
@@ -112,6 +112,8 @@ public abstract class PlayerController {
     public Card chooseSingleCardForEffect(List<Card> sourceList, SpellAbility sa, String title) { return chooseSingleCardForEffect(sourceList, sa, title, false); }
     public abstract Card chooseSingleCardForEffect(List<Card> sourceList, SpellAbility sa, String title, boolean isOptional);
     public abstract Player chooseSinglePlayerForEffect(List<Player> options, SpellAbility sa, String title);
+    public abstract SpellAbility chooseSingleSpellForEffect(List<SpellAbility> spells, SpellAbility sa, String title);
+
     public abstract boolean confirmAction(SpellAbility sa, PlayerActionConfirmMode mode, String message);
     public abstract boolean getWillPlayOnFirstTurn(boolean isFirstGame);
     public abstract boolean confirmStaticApplication(Card hostCard, GameEntity affected, String logic, String message);

src/main/java/forge/game/player/PlayerControllerAi.java

@@ -94,22 +94,6 @@ public class PlayerControllerAi extends PlayerController {
         return brains;
     }
 
-    /* (non-Javadoc)
-     * @see forge.game.player.PlayerController#mayPlaySpellAbilityForFree(forge.card.spellability.SpellAbility)
-     */
-    @Override
-    public void playSpellAbilityForFree(SpellAbility copySA) {
-        if (copySA instanceof Spell) {
-            Spell spell = (Spell) copySA;
-            if (spell.canPlayFromEffectAI(true, true)) {
-                ComputerUtil.playStackFree(player, copySA);
-            }
-        } else {
-            copySA.canPlayAI();
-            ComputerUtil.playStackFree(player, copySA);
-        }
-    }
-
     @Override
     public Deck sideboard(Deck deck, GameType gameType) {
         // AI does not know how to sideboard
@@ -156,6 +140,15 @@ public class PlayerControllerAi extends PlayerController {
         return api.getAi().chooseSinglePlayer(player, sa, options);
     }
 
+    @Override
+    public SpellAbility chooseSingleSpellForEffect(java.util.List<SpellAbility> spells, SpellAbility sa, String title) {
+        ApiType api = sa.getApi();
+        if ( null == api ) {
+            throw new InvalidParameterException("SA is not api-based, this is not supported yet");
+        }
+        return api.getAi().chooseSingleSpellAbility(player, sa, spells);
+    }
+    
     
     @Override
     public boolean confirmAction(SpellAbility sa, PlayerActionConfirmMode mode, String message) {
@@ -235,6 +228,25 @@ public class PlayerControllerAi extends PlayerController {
         return getAi().chooseCardToDredge(dredgers);
     }
 
+    /* (non-Javadoc)
+     * @see forge.game.player.PlayerController#mayPlaySpellAbilityForFree(forge.card.spellability.SpellAbility)
+     */
+    @Override
+    public void playSpellAbilityForFree(SpellAbility copySA, boolean mayChooseNewTargets) {
+        // Ai is known to set targets in doTrigger, so if it cannot choose new targets, we won't call canPlays
+        if( mayChooseNewTargets ) {
+            if (copySA instanceof Spell) {
+                Spell spell = (Spell) copySA;
+                if (!spell.canPlayFromEffectAI(true, true)) {
+                    return; // is this legal at all?
+                }
+            } else {
+                copySA.canPlayAI();
+            }
+        }
+        ComputerUtil.playSpellAbilityForFree(player, copySA);
+    }
+
     @Override
     public void playMiracle(SpellAbility miracle, Card card) {
         getAi().chooseAndPlaySa(false, false, miracle);

src/main/java/forge/game/player/PlayerControllerHuman.java

@@ -116,8 +116,8 @@ public class PlayerControllerHuman extends PlayerController {
      * @see forge.game.player.PlayerController#mayPlaySpellAbilityForFree(forge.card.spellability.SpellAbility)
      */
     @Override
-    public void playSpellAbilityForFree(SpellAbility copySA) {
-        HumanPlay.playSaWithoutPayingManaCost(player.getGame(), copySA);
+    public void playSpellAbilityForFree(SpellAbility copySA, boolean mayChoseNewTargets) {
+        HumanPlay.playSaWithoutPayingManaCost(player.getGame(), copySA, mayChoseNewTargets);
     }
 
     /* (non-Javadoc)
@@ -278,22 +278,24 @@ public class PlayerControllerHuman extends PlayerController {
 
     @Override
     public int chooseNumber(SpellAbility sa, String title, int min, int max) {
-        final String[] choices = new String[max + 1];
+        final Integer[] choices = new Integer[max + 1];
         for (int i = min; i <= max; i++) {
-            choices[i] = Integer.toString(i);
+            choices[i] = Integer.valueOf(i);
         }
-        return Integer.parseInt(GuiChoose.one(title, choices));
+        return GuiChoose.one(title, choices).intValue();
     }
 
     @Override
     public Player chooseSinglePlayerForEffect(List<Player> options, SpellAbility sa, String title) {
         // Human is supposed to read the message and understand from it what to choose
-        if (options.size() > 1)
-            return GuiChoose.one(title, options);
-        else 
-            return options.get(0);
+        return options.size() < 2 ? options.get(0) : GuiChoose.one(title, options);
     }
 
+    @Override
+    public SpellAbility chooseSingleSpellForEffect(java.util.List<SpellAbility> spells, SpellAbility sa, String title) {
+        // Human is supposed to read the message and understand from it what to choose
+        return spells.size() < 2 ? spells.get(0) : GuiChoose.one(title, spells);
+    }
 
     /* (non-Javadoc)
      * @see forge.game.player.PlayerController#confirmAction(forge.card.spellability.SpellAbility, java.lang.String, java.lang.String)

src/main/java/forge/game/zone/MagicStack.java

@@ -360,7 +360,8 @@ public class MagicStack /* extends MyObservable */ implements Iterable<SpellAbil
                     } while( hasPaid );
 
                     for (int i = 0; i < magnitude; i++) {
-                        CardFactory.copySpellontoStack(sp.getSourceCard(), sp.getSourceCard(), sp, false, null);
+                        SpellAbility copy = CardFactory.copySpellAbilityAndSrcCard(sp.getSourceCard(), sp.getSourceCard(), sp, false);
+                        activating.getController().playSpellAbilityForFree(copy, true);
                     }
                 }
 

src/main/java/forge/gui/GuiDisplayUtil.java

@@ -429,7 +429,7 @@ public final class GuiDisplayUtil {
                     game.getAction().moveToHand(forgeCard); // this is really needed (for rollbacks at least) 
                     // Human player is choosing targets for an ability controlled by chosen player. 
                     sa.setActivatingPlayer(p);
-                    HumanPlay.playSaWithoutPayingManaCost(game, sa);
+                    HumanPlay.playSaWithoutPayingManaCost(game, sa, true);
                 }
             }
         });