choose mode improves:

- fixed broken cards with once per turn choose (example: Galadriel, Light of Valinor, closes #11362);
- fixed cheat to skip required mode by cancel button (example: Black Market Connections, closes #11149, closes #10611);
- fixed empty modes list if nothing available to choose;
- improved compatibility with max modes and other modification effects;
- fixed that non-valid modes can be selected in some use cases;

Mage.Server.Plugins/Mage.Player.Human/src/mage/player/human/HumanPlayer.java

@@ -18,8 +18,6 @@ import mage.cards.decks.Deck;
 import mage.choices.Choice;
 import mage.choices.ChoiceImpl;
 import mage.constants.*;
-import static mage.constants.PlayerAction.REQUEST_AUTO_ANSWER_RESET_ALL;
-import static mage.constants.PlayerAction.TRIGGER_AUTO_ORDER_RESET_ALL;
 import mage.filter.StaticFilters;
 import mage.filter.common.FilterAttackingCreature;
 import mage.filter.common.FilterBlockingCreature;
@@ -56,6 +54,9 @@ import java.util.*;
 import java.util.concurrent.ConcurrentLinkedQueue;
 import java.util.stream.Collectors;
 
+import static mage.constants.PlayerAction.REQUEST_AUTO_ANSWER_RESET_ALL;
+import static mage.constants.PlayerAction.TRIGGER_AUTO_ORDER_RESET_ALL;
+
 /**
  * @author BetaSteward_at_googlemail.com
  */
@@ -2428,9 +2429,17 @@ public class HumanPlayer extends PlayerImpl {
             return null;
         }
 
-        if (modes.size() > 1) {
+        if (modes.size() == 0) {
-            // done option for up to choices
+            return null;
-            boolean canEndChoice = modes.getSelectedModes().size() >= modes.getMinModes() || modes.isMayChooseNone();
+        }
+
+        if (modes.size() == 1) {
+            return modes.getMode();
+        }
+
+        boolean done = false;
+        while (!done && canRespond()) {
+            // prepare modes list
             MageObject obj = game.getObject(source);
             Map<UUID, String> modeMap = new LinkedHashMap<>();
             int modeIndex = 0;
@@ -2468,65 +2477,61 @@ public class HumanPlayer extends PlayerImpl {
                 }
             }
 
-            if (!modeMap.isEmpty()) {
+            // done button for "for up" choices only
-
+            boolean canEndChoice = modes.getSelectedModes().size() >= modes.getMinModes() || modes.isMayChooseNone();
-                // can done for up to
+            if (canEndChoice) {
-                if (canEndChoice) {
+                modeMap.put(Modes.CHOOSE_OPTION_DONE_ID, "Done");
-                    modeMap.put(Modes.CHOOSE_OPTION_DONE_ID, "Done");
+            }
-                }
+            modeMap.put(Modes.CHOOSE_OPTION_CANCEL_ID, "Cancel");
-                modeMap.put(Modes.CHOOSE_OPTION_CANCEL_ID, "Cancel");
+
-
+            // prepare dialog
-                boolean done = false;
+            String message = "Choose mode (selected " + modes.getSelectedModes().size() + " of " + modes.getMaxModes(game, source)
-                while (!done && canRespond()) {
+                    + ", min " + modes.getMinModes() + ")";
-
+            if (obj != null) {
-                    String message = "Choose mode (selected " + modes.getSelectedModes().size() + " of " + modes.getMaxModes(game, source)
+                message = message + "<br>" + obj.getLogName();
-                            + ", min " + modes.getMinModes() + ")";
+            }
-                    if (obj != null) {
+
-                        message = message + "<br>" + obj.getLogName();
+            updateGameStatePriority("chooseMode", game);
-                    }
+            prepareForResponse(game);
-
+            if (!isExecutingMacro()) {
-                    updateGameStatePriority("chooseMode", game);
+                game.fireGetModeEvent(playerId, message, modeMap);
-                    prepareForResponse(game);
+            }
-                    if (!isExecutingMacro()) {
+            waitForResponse(game);
-                        game.fireGetModeEvent(playerId, message, modeMap);
+
-                    }
+            // process choice
-                    waitForResponse(game);
+            UUID responseId = getFixedResponseUUID(game);
-
+            if (responseId != null) {
-                    UUID responseId = getFixedResponseUUID(game);
+                for (Mode mode : modes.getAvailableModes(source, game)) {
-                    if (responseId != null) {
+                    if (mode.getId().equals(responseId)) {
-                        for (Mode mode : modes.getAvailableModes(source, game)) {
+                        // TODO: add checks on 2x selects (cheaters can rewrite client side code and select same mode multiple times)
-                            if (mode.getId().equals(responseId)) {
+                        // reason: wrong setup eachModeMoreThanOnce and eachModeOnlyOnce in many cards
-                                // TODO: add checks on 2x selects (cheaters can rewrite client side code and select same mode multiple times)
+                        return mode;
-                                // reason: wrong setup eachModeMoreThanOnce and eachModeOnlyOnce in many cards
+                    }
-                                return mode;
+                }
-                            }
+
-                        }
+                // end choice by done option in ability pickup dialog
-
+                if (canEndChoice && Modes.CHOOSE_OPTION_DONE_ID.equals(responseId)) {
-                        // end choice by done option in ability pickup dialog
+                    done = true;
-                        if (canEndChoice && Modes.CHOOSE_OPTION_DONE_ID.equals(responseId)) {
+                }
-                            done = true;
+
-                        }
+                // cancel choice (remove all selections)
-
+                if (Modes.CHOOSE_OPTION_CANCEL_ID.equals(responseId)) {
-                        // cancel choice (remove all selections)
+                    modes.clearSelectedModes();
-                        if (Modes.CHOOSE_OPTION_CANCEL_ID.equals(responseId)) {
+                }
-                            modes.clearSelectedModes();
+            } else if (canEndChoice) {
-                        }
+                // end choice by done button in feedback panel
-                    } else if (canEndChoice) {
+                // disable after done option implemented
-                        // end choice by done button in feedback panel
+                // done = true;
-                        // disable after done option implemented
+            }
-                        // done = true;
+
-                    }
+            // triggered abilities can't be skipped by cancel or wrong answer
-
+            if (source.getAbilityType() != AbilityType.TRIGGERED) {
-                    // triggered abilities can't be skipped by cancel or wrong answer
+                done = true;
-                    if (source.getAbilityType() != AbilityType.TRIGGERED) {
-                        done = true;
-                    }
-                }
             }
-            return null;
         }
 
-        return modes.getMode();
+        // user disconnected, press cancel, press done or something else
+        return null;
     }
 
     @Override

Mage.Tests/src/test/java/org/mage/test/cards/modal/OnlyOnceModeTest.java

@@ -0,0 +1,106 @@
+package org.mage.test.cards.modal;
+
+import mage.constants.PhaseStep;
+import mage.constants.Zone;
+import mage.counters.CounterType;
+import org.junit.Test;
+import org.mage.test.serverside.base.CardTestPlayerBase;
+
+/**
+ * @author JayDi85
+ */
+public class OnlyOnceModeTest extends CardTestPlayerBase {
+
+    @Test
+    public void test_OncePerGame() {
+        // {2}, {T}: Choose one that hasn't been chosen
+        // Three Bowls of Porridge deals 2 damage to target creature.
+        // Tap target creature.
+        // Sacrifice Three Bowls of Porridge. You gain 3 life.
+        addCard(Zone.BATTLEFIELD, playerA, "Three Bowls of Porridge");
+        //
+        addCard(Zone.BATTLEFIELD, playerA, "Spectral Bears", 1); // 3/3
+        addCard(Zone.BATTLEFIELD, playerA, "Forest", 2);
+
+        // each mode usage must be removed from a list, so all mode choices must be 1
+
+        // turn 1 - first mode available
+        // choose 1: damage 2 to target
+        checkDamage("before mode 1", 1, PhaseStep.PRECOMBAT_MAIN, playerA, "Spectral Bears", 0);
+        activateAbility(1, PhaseStep.PRECOMBAT_MAIN, playerA, "{2}, {T}");
+        setModeChoice(playerA, "1");
+        addTarget(playerA, "Spectral Bears"); // to damage
+        waitStackResolved(1, PhaseStep.PRECOMBAT_MAIN);
+        checkDamage("before mode 1", 1, PhaseStep.PRECOMBAT_MAIN, playerA, "Spectral Bears", 2);
+
+        // turn 3 - first mode doesn't restore, so mode 2 will be used
+        // choose 2: Tap target creature
+        checkPermanentTapped("before mode 2", 3, PhaseStep.PRECOMBAT_MAIN, playerA, "Spectral Bears", false, 1);
+        activateAbility(3, PhaseStep.PRECOMBAT_MAIN, playerA, "{2}, {T}");
+        setModeChoice(playerA, "1");
+        addTarget(playerA, "Spectral Bears"); // to tap
+        waitStackResolved(3, PhaseStep.PRECOMBAT_MAIN);
+        checkPermanentTapped("after mode 2", 3, PhaseStep.PRECOMBAT_MAIN, playerA, "Spectral Bears", true, 1);
+
+        setStrictChooseMode(true);
+        setStopAt(3, PhaseStep.END_TURN);
+        execute();
+    }
+
+    @Test
+    public void test_OncePerTurn() {
+        int triggerCalls = 5;
+
+        // Whenever another creature enters the battlefield under your control, choose one that hasn't been chosen this turn
+        // Add {G}{G}{G}.
+        // Put a +1/+1 counter on each creature you control.
+        // Scry 2, then draw a card.
+        addCard(Zone.BATTLEFIELD, playerA, "Galadriel, Light of Valinor");
+        //
+        addCard(Zone.HAND, playerA, "Grizzly Bears", triggerCalls); // {1}{G}
+        addCard(Zone.BATTLEFIELD, playerA, "Forest", 2 * triggerCalls);
+        //
+        addCard(Zone.LIBRARY, playerA, "Mountain", 10);
+
+        // each mode usage must be removed from a list, so all mode choices must be 1
+
+        // do trigger 1: Add {G}{G}{G}
+        checkManaPool("before mode 1", 1, PhaseStep.PRECOMBAT_MAIN, playerA, "G", 0);
+        castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Grizzly Bears");
+        setModeChoice(playerA, "1");
+        waitStackResolved(1, PhaseStep.PRECOMBAT_MAIN);
+        checkManaPool("after mode 1", 1, PhaseStep.PRECOMBAT_MAIN, playerA, "G", 3);
+
+        // do trigger 2: Put a +1/+1 counter
+        checkPermanentCounters("before mode 2", 1, PhaseStep.PRECOMBAT_MAIN, playerA, "Grizzly Bears", CounterType.P1P1, 0);
+        castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Grizzly Bears");
+        setModeChoice(playerA, "1");
+        waitStackResolved(1, PhaseStep.PRECOMBAT_MAIN);
+        checkPermanentCounters("after mode 2", 1, PhaseStep.PRECOMBAT_MAIN, playerA, "Grizzly Bears", CounterType.P1P1, 1);
+
+        // do trigger 3: Scry 2, then draw a card
+        checkHandCardCount("before mode 3", 1, PhaseStep.PRECOMBAT_MAIN, playerA, "Mountain", 0);
+        castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Grizzly Bears");
+        setModeChoice(playerA, "1");
+        addTarget(playerA, "Mountain"); // scry 2, one to bottom, one to top
+        waitStackResolved(1, PhaseStep.PRECOMBAT_MAIN);
+        checkHandCardCount("after mode 3", 1, PhaseStep.PRECOMBAT_MAIN, playerA, "Mountain", 1);
+
+        // do trigger 4: nothing
+        castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Grizzly Bears");
+
+        // next turn triggers works again
+        // do trigger 5: Add {G}{G}{G}
+        checkManaPool("before mode 1 on turn 3", 3, PhaseStep.PRECOMBAT_MAIN, playerA, "G", 0);
+        castSpell(3, PhaseStep.PRECOMBAT_MAIN, playerA, "Grizzly Bears");
+        setModeChoice(playerA, "1");
+        waitStackResolved(3, PhaseStep.PRECOMBAT_MAIN);
+        checkManaPool("after mode 1 on turn 3", 3, PhaseStep.PRECOMBAT_MAIN, playerA, "G", 3);
+
+        setStrictChooseMode(true);
+        setStopAt(3, PhaseStep.END_TURN);
+        execute();
+
+        assertPermanentCount(playerA, "Grizzly Bears", triggerCalls);
+    }
+}

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

@@ -45,7 +45,7 @@ public class Modes extends LinkedHashMap<UUID, Mode> implements Copyable<Modes>
     private TargetController chooseController;
     private boolean mayChooseSameModeMoreThanOnce = false; // example: choose three... you may choose the same mode more than once
     private boolean mayChooseNone = false;
-    private boolean isRandom = false;
+    private boolean isRandom = false; // random from available modes, not modes TODO: research rules of Cult of Skaro after WHO release (is it random from all modes or from available/valid)
 
     public Modes() {
         // add default mode
@@ -287,19 +287,41 @@ public class Modes extends LinkedHashMap<UUID, Mode> implements Copyable<Modes>
                 && getOnceTurnNum(game, source) != game.getTurnNum();
     }
 
+    private boolean isSelectedValid(Ability source, Game game) {
+        if (isLimitUsageByOnce()) {
+            setOnceSelectedModes(source, game);
+        }
+        return this.selectedModes.size() >= this.getMinModes()
+                || (this.selectedModes.size() == 0 && mayChooseNone);
+    }
+
     public boolean choose(Game game, Ability source) {
         if (isAlreadySelectedModesOutdated(game, source)) {
             this.clearAlreadySelectedModes(source, game);
         }
-        if (this.size() > 1) {
+        this.clearSelectedModes();
-            this.clearSelectedModes();
-            if (this.isRandom) {
-                List<Mode> modes = getAvailableModes(source, game);
-                this.addSelectedMode(modes.get(RandomUtil.nextInt(modes.size())).getId());
-                return true;
-            }
 
-            // check if mode modifying abilities exist
+        // runtime check
+        if (this.isRandom && limitUsageByOnce) {
+            // non-tested use case, if you catch this error then disable and manually test, if fine then that check can be removed
+            throw new IllegalStateException("Wrong code usage: random modes are not support with once usage");
+        }
+
+        // 700.2b
+        // The controller of a modal triggered ability chooses the mode(s) as part of putting that ability
+        // on the stack. If one of the modes would be illegal (due to an inability to choose legal targets, for
+        // example), that mode can’t be chosen. If no mode is chosen, the ability is removed from the
+        // stack. (See rule 603.3c.)
+        List<Mode> availableModes = getAvailableModes(source, game);
+        if (availableModes.size() == 0) {
+            return isSelectedValid(source, game);
+        }
+
+        // modal spells must show choose dialog even for 1 option, so check this.size instead evailableModes.size here
+        if (this.size() > 1) {
+            // multiple modes
+
+            // modes modifications, e.g. choose max modes instead single
             Card card = game.getCard(source.getSourceId());
             if (card != null) {
                 for (Ability modeModifyingAbility : card.getAbilities(game)) {
@@ -310,7 +332,14 @@ public class Modes extends LinkedHashMap<UUID, Mode> implements Copyable<Modes>
                 }
             }
 
-            // check if all modes can be activated automatically
+            // choose random
+            if (this.isRandom) {
+                // TODO: research rules of Cult of Skaro after WHO release (is it random from all modes or from available/valid)
+                this.addSelectedMode(availableModes.get(RandomUtil.nextInt(availableModes.size())).getId());
+                return isSelectedValid(source, game);
+            }
+
+            // UX: check if all modes can be activated automatically
             if (this.size() == this.getMinModes() && !isMayChooseSameModeMoreThanOnce()) {
                 Set<UUID> onceSelectedModes = null;
                 if (isLimitUsageByOnce()) {
@@ -322,10 +351,7 @@ public class Modes extends LinkedHashMap<UUID, Mode> implements Copyable<Modes>
                         this.addSelectedMode(mode.getId());
                     }
                 }
-                if (isLimitUsageByOnce()) {
+                return isSelectedValid(source, game);
-                    setOnceSelectedModes(source, game);
-                }
-                return !selectedModes.isEmpty();
             }
 
             // 700.2d
@@ -352,20 +378,15 @@ public class Modes extends LinkedHashMap<UUID, Mode> implements Copyable<Modes>
             while (this.selectedModes.size() < currentMaxModes) {
                 Mode choice = player.chooseMode(this, source, game);
                 if (choice == null) {
-                    if (isLimitUsageByOnce()) {
+                    // user press cancel/stop in choose dialog or nothing to choose
-                        setOnceSelectedModes(source, game);
+                    return isSelectedValid(source, game);
-                    }
-                    return this.selectedModes.size() >= this.getMinModes()
-                            || (this.selectedModes.size() == 0 && mayChooseNone);
                 }
                 this.addSelectedMode(choice.getId());
                 if (currentMode == null) {
                     currentMode = choice;
                 }
             }
-            if (isLimitUsageByOnce()) {
+            // effects helper (keep real choosing player)
-                setOnceSelectedModes(source, game);
-            }
             if (chooseController == TargetController.OPPONENT) {
                 selectedModes
                         .stream()
@@ -374,13 +395,13 @@ public class Modes extends LinkedHashMap<UUID, Mode> implements Copyable<Modes>
                         .forEach(effects -> effects.setValue("choosingPlayer", playerId));
             }
         } else {
-            // only one mode available
+            // only one mode
-            this.clearSelectedModes();
             Mode mode = this.values().iterator().next();
             this.addSelectedMode(mode.getId());
             this.setActiveMode(mode);
         }
-        return true;
+
+        return isSelectedValid(source, game);
     }
 
     /**
@@ -449,7 +470,7 @@ public class Modes extends LinkedHashMap<UUID, Mode> implements Copyable<Modes>
         Set<UUID> res = new HashSet<>();
 
         // if selected modes is not for current turn, so we ignore any value that may be there
-        if (!ignoreOutdatedData && isAlreadySelectedModesOutdated(game, source)) {
+        if (ignoreOutdatedData && isAlreadySelectedModesOutdated(game, source)) {
             return res;
         }