Fix copying subabilities to no longer duplicate them (#11399)

* Fix Subability copy bug (fix #10526 )

* Cards which copy abilities of other cards should not copy subabilities.

* Enable previously-failing tests

* Find more addAbility that should be done without subabilities

* Add documentation to addAbility function

* Add warning about not using basic addAbility when copying from a source

* Invert withSubabilities to fromExistingObject

Mage/src/main/java/mage/abilities/Ability.java

@@ -332,6 +332,8 @@ public interface Ability extends Controllable, Serializable {
 
     /**
      * Gets the list of sub-abilities associated with this ability.
+     * When copying, subabilities are copied separately and thus the list is desynced.
+     * Do not interact with the subabilities list during a game!
      *
      * @return
      */

Mage/src/main/java/mage/abilities/effects/common/CopyEffect.java

@@ -119,11 +119,11 @@ public class CopyEffect extends ContinuousEffectImpl {
         permanent.removeAllAbilities(source.getSourceId(), game);
         if (copyFromObject instanceof Permanent) {
             for (Ability ability : ((Permanent) copyFromObject).getAbilities(game)) {
-                permanent.addAbility(ability, getSourceId(), game);
+                permanent.addAbility(ability, getSourceId(), game, true);
             }
         } else {
             for (Ability ability : copyFromObject.getAbilities()) {
-                permanent.addAbility(ability, getSourceId(), game);
+                permanent.addAbility(ability, getSourceId(), game, true);
             }
         }
 

Mage/src/main/java/mage/abilities/effects/common/CopyTokenEffect.java

@@ -39,7 +39,7 @@ public class CopyTokenEffect extends ContinuousEffectImpl {
         }
         permanent.getAbilities().clear();
         for (Ability ability : token.getAbilities()) {
-            permanent.addAbility(ability, source.getSourceId(), game);
+            permanent.addAbility(ability, source.getSourceId(), game, true);
         }
         permanent.getPower().setModifiedBaseValue(token.getPower().getModifiedBaseValue());
         permanent.getToughness().setModifiedBaseValue(token.getToughness().getModifiedBaseValue());

Mage/src/main/java/mage/abilities/effects/common/GainActivatedAbilitiesOfTopCardEffect.java

@@ -43,7 +43,7 @@ public class GainActivatedAbilitiesOfTopCardEffect extends ContinuousEffectImpl
                 if (permanent != null) {
                     for (Ability ability : card.getAbilities(game)) {
                         if (ability instanceof ActivatedAbility) {
-                            permanent.addAbility(ability, source.getSourceId(), game);
+                            permanent.addAbility(ability, source.getSourceId(), game, true);
                         }
                     }
                     return true;

Mage/src/main/java/mage/abilities/effects/common/continuous/BecomesCreatureAllEffect.java

@@ -128,7 +128,7 @@ public class BecomesCreatureAllEffect extends ContinuousEffectImpl {
                 case AbilityAddingRemovingEffects_6:
                     if (!token.getAbilities().isEmpty()) {
                         for (Ability ability : token.getAbilities()) {
-                            permanent.addAbility(ability, source.getSourceId(), game);
+                            permanent.addAbility(ability, source.getSourceId(), game, true);
                         }
                     }
                     break;

Mage/src/main/java/mage/abilities/effects/common/continuous/BecomesCreatureAttachedEffect.java

@@ -112,7 +112,7 @@ public class BecomesCreatureAttachedEffect extends ContinuousEffectImpl {
                         break;
                 }
                 for (Ability ability : token.getAbilities()) {
-                    permanent.addAbility(ability, source.getSourceId(), game);
+                    permanent.addAbility(ability, source.getSourceId(), game, true);
                 }
                 break;
 

Mage/src/main/java/mage/abilities/effects/common/continuous/BecomesCreatureSourceEffect.java

@@ -141,7 +141,7 @@ public class BecomesCreatureSourceEffect extends ContinuousEffectImpl {
                     permanent.removeAllAbilities(source.getSourceId(), game);
                 }
                 for (Ability ability : token.getAbilities()) {
-                    permanent.addAbility(ability, source.getSourceId(), game);
+                    permanent.addAbility(ability, source.getSourceId(), game, true);
                 }
                 break;
 

Mage/src/main/java/mage/abilities/effects/common/continuous/BecomesCreatureTargetEffect.java

@@ -133,7 +133,7 @@ public class BecomesCreatureTargetEffect extends ContinuousEffectImpl {
                     if (sublayer == SubLayer.NA) {
                         if (!token.getAbilities().isEmpty()) {
                             for (Ability ability : token.getAbilities()) {
-                                permanent.addAbility(ability, source.getSourceId(), game);
+                                permanent.addAbility(ability, source.getSourceId(), game, true);
                             }
                         }
                     }

Mage/src/main/java/mage/abilities/keyword/CrewAbility.java

@@ -51,6 +51,8 @@ public class CrewAbility extends SimpleActivatedAbility {
         this.addIcon(new CardIconImpl(CardIconType.ABILITY_CREW, "Crew " + value));
         this.value = value;
         if (altCost != null) {
+            //TODO: the entire alternative cost should be included in the subability, not just the hint text
+            // Heart of Kiran's alternative crew cost is a static ability, not part of the activated ability directly
             this.addSubAbility(new SimpleStaticAbility(Zone.ALL, new InfoEffect(
                     "you may " + CardUtil.addCostVerb(altCost.getText())
                             + " rather than pay {this}'s crew cost"

Mage/src/main/java/mage/abilities/keyword/TransformAbility.java

@@ -67,7 +67,7 @@ public class TransformAbility extends SimpleStaticAbility {
         for (Ability ability : sourceCard.getAbilities()) {
             // source == null -- call from init card (e.g. own abilities)
             // source != null -- from apply effect
-            permanent.addAbility(ability, source == null ? permanent.getId() : source.getSourceId(), game);
+            permanent.addAbility(ability, source == null ? permanent.getId() : source.getSourceId(), game, true);
         }
         permanent.getPower().setModifiedBaseValue(sourceCard.getPower().getValue());
         permanent.getToughness().setModifiedBaseValue(sourceCard.getToughness().getValue());

Mage/src/main/java/mage/game/permanent/Permanent.java

@@ -218,6 +218,7 @@ public interface Permanent extends Card, Controllable {
      * @return can be null for exists abilities
      */
     Ability addAbility(Ability ability, UUID sourceId, Game game);
+    Ability addAbility(Ability ability, UUID sourceId, Game game, boolean fromExistingObject);
 
     void removeAllAbilities(UUID sourceId, Game game);
 

Mage/src/main/java/mage/game/permanent/PermanentImpl.java

@@ -388,8 +388,29 @@ public abstract class PermanentImpl extends CardImpl implements Permanent {
         return super.getAbilities(game);
     }
 
+    /**
+     * Add an ability to the permanent. When copying from an existing source
+     * you should use the fromExistingObject variant of this function to prevent double-copying subabilities
+     * @param ability The ability to be added
+     * @param sourceId   id of the source doing the added (for the effect created to add it)
+     * @param game
+     * @return The newly added ability copy
+     */
     @Override
     public Ability addAbility(Ability ability, UUID sourceId, Game game) {
+        return addAbility(ability, sourceId, game, false);
+    }
+
+    /**
+     * @param ability The ability to be added
+     * @param sourceId   id of the source doing the added (for the effect created to add it)
+     * @param game
+     * @param fromExistingObject if copying abilities from an existing source then must ignore sub-abilities because they're already on the source object
+     *                         Otherwise sub-abilities will be added twice to the resulting object
+     * @return The newly added ability copy
+     */
+    @Override
+    public Ability addAbility(Ability ability, UUID sourceId, Game game, boolean fromExistingObject) {
         // singleton abilities -- only one instance
         // other abilities -- any amount of instances
         if (!abilities.containsKey(ability.getId())) {
@@ -404,7 +425,9 @@ public abstract class PermanentImpl extends CardImpl implements Permanent {
                 game.getState().addAbility(copyAbility, sourceId, this);
             }
             abilities.add(copyAbility);
-            abilities.addAll(ability.getSubAbilities());
+            if (!fromExistingObject) {
+                abilities.addAll(copyAbility.getSubAbilities());
+            }
             return copyAbility;
         }
         return null;

Mage/src/main/java/mage/game/permanent/PermanentToken.java

@@ -89,7 +89,8 @@ public class PermanentToken extends PermanentImpl {
             // first time -> create ContinuousEffects only once
             // so sourceId must be null (keep triggered abilities forever?)
             for (Ability ability : token.getAbilities()) {
-                this.addAbility(ability, null, game);
+                //Don't add subabilities since the original token already has them in its abilities list
+                this.addAbility(ability, null, game, true);
             }
         }
         this.abilities.setControllerId(this.controllerId);

Mage/src/main/java/mage/game/permanent/token/Token.java

@@ -22,6 +22,7 @@ public interface Token extends MageObject {
     List<UUID> getLastAddedTokenIds();
 
     void addAbility(Ability ability);
+    void addAbility(Ability ability, boolean fromExistingObject);
 
     void removeAbility(Ability abilityToRemove);
 

Mage/src/main/java/mage/game/permanent/token/TokenImpl.java

@@ -77,15 +77,33 @@ public abstract class TokenImpl extends MageObjectImpl implements Token {
         return new ArrayList<>(lastAddedTokenIds);
     }
 
+    /**
+     * Add an ability to the token. When copying from an existing source
+     * you should use the fromExistingObject variant of this function to prevent double-copying subabilities
+     * @param ability The ability to be added
+     */
     @Override
     public void addAbility(Ability ability) {
+        addAbility(ability, false);
+    }
+
+    /**
+     * @param ability The ability to be added
+     * @param fromExistingObject if copying abilities from an existing source then must ignore sub-abilities because they're already on the source object
+     *                         Otherwise sub-abilities will be added twice to the resulting object
+     */
+    @Override
+    public void addAbility(Ability ability, boolean fromExistingObject) {
         ability.setSourceId(this.getId());
         abilities.add(ability);
-        abilities.addAll(ability.getSubAbilities());
+        if (!fromExistingObject) {
+            abilities.addAll(ability.getSubAbilities());
+        }
 
         // TODO: remove all override and backFace changes (bug example: active transform ability in back face)
         if (backFace != null) {
             backFace.addAbility(ability);
+            // Maybe supposed to add subabilities here too?
         }
     }
 

Mage/src/main/java/mage/util/functions/CopyTokenFunction.java

@@ -140,8 +140,8 @@ public class CopyTokenFunction {
             // otherwise there are problems to check for created continuous effects to check if
             // the source (the Token) has still this ability
             ability.newOriginalId();
-
-            target.addAbility(ability);
+            //Don't re-add subabilities since they've already in sourceObj's abilities list
+            target.addAbility(ability, true);
         }
 
         target.setPower(sourceObj.getPower().getBaseValue());
@@ -152,7 +152,6 @@ public class CopyTokenFunction {
 
     private Token from(Card source, Game game, Spell spell) {
         apply(source, game);
-
         // token's ZCC must be synced with original card to keep abilities settings
         // Example: kicker ability and kicked status
         if (spell != null) {