Bail out of human mode selection when list empty

[matoro]

I originally wrote this by changing the engine to filter modes with no valid targets before even passing them to Player.chooseMode(), however that blew up the tests as they all assume that mode number selection is chosen from the complete list of nodes, and narrowing that down to only valid modes completely threw off the numbering scheme. So instead fix it in the human player plugin by simply bailing out early if all modes have been stripped out.

Fixes #14805

[JayDi85]

changing the engine to filter modes with no valid targets before even passing them to Player.chooseMode()

It's very important use case -- AI and other auto-games (e.g. with auto-selection) can freeze the choice dialog in infinite loop? It must be research before fix apply -- cause current fix will hide it forever from human games, but not AI (if AI buggy).

[matoro]

changing the engine to filter modes with no valid targets before even passing them to Player.chooseMode()

It's very important use case -- AI and other auto-games (e.g. with auto-selection) can freeze the choice dialog in infinite loop? It must be research before fix apply -- cause current fix will hide it forever from human games, but not AI (if AI buggy).

So, you would rather I go with the first solution and update all the mode selection numbers in the tests?

[JayDi85]

So, you would rather I go with the first solution and update all the mode selection numbers in the tests?

Nope, I recommend to test AI game and make sure it's not affected by that bug (AI game must not freeze in zero available choices). You can test it manually or by AI unit tests (see any usage of CardTestPlayerBaseWithAIHelps).

[JayDi85]

however that blew up the tests as they all assume that mode number selection is chosen from the complete list of nodes

btw that's not true -- it's choose from filtered options, e.g. already selected option will be hide and option number will be changed for unit tests.

Also one of the potential problem for such use cases (zero available options) -- human must see empty GUI dialog with done button on no more available options, e.g. for "up to" modes.

[matoro]

however that blew up the tests as they all assume that mode number selection is chosen from the complete list of nodes

btw that's not true -- it's choose from filtered options, e.g. already selected option will be hide and option number will be changed for unit tests.

yeah, but I meant that filtering out modes with invalid targets changed the expected numbers again on top of that.

[xenohedron]

Can open separate issue to test the scenario in AI games, sure, but that is NOT a good reason to delay this fix. Game-breaking bugs are very bad and fixes are high priority.

(I'd be skeptical of any fix that changes a lot of test behavior, unless we agree it's definitely desired improvement. Either way - separate scope from this PR.)

[matoro]

Fixed these. Added an additional condition to only bail out when mode selection is mandatory (i.e. not on "choose up to", manually confirmed this still fixes the issue) and also added an AI test to validate that it doesn't freeze in this scenario.