Problems with Target.choose() and Player.choose() : clicking "Done" after choosing less than max amount of cards doesn't behave as expected.
- Dominant language
- Java
- Stars
- 2.4k
- Forks
- 940
- Avg merge
- 2d 12h
- Merged PRs (30d)
- 160
Description
While coding [WHO] [The Five Doctors](https://scryfall.com/card/who/101/the-five-doctors) (see #13691) I found two problems with choosing cards. They happen when you choose less cards than the max number of targets and then click "Done" to finish choosing. I believe they are caused by coding inconsistencies in methods `Target/TargetCardInLibrary.choose()` and `HumanPlayer.choose()`.
Here is what I found out :
1. This first problem concerns methods `TargetCardInLibrary.choose()` and `Target.choose() `(and probably `Target.chooseTarget()` and similar methods from the Targets class).
**Here is the problem** : when you select less cards than the max number via these methods and then click "Done", nothing happens. You can continue selecting cards. Choosing only stops when you click "Done" after having selected the same number of cards twice in a row (they don't have to be the same cards).
For example :
- if you select card A, then click "Done" twice, choosing stops
- if you select card A, then click "Done", then add card B and remove card A, then click "Done", it stops.
- if you select card A, then click "Done", then add card B, then click "Done", it doesn't stop (because the number of selected cards change).
This should affect all calls to `Player.searchLibrary()`, which calls `TargetCardInLibrary.choose()`. It can be checked using e.g. [[Burnished Hart]]. It doesn't exactly break the game flow, since you can still select whatever you want, but it is clearly not the expected behaviour.
This is the relevant code from `Target.choose()` (file `TargetImpl.java`) :
```
// stop by cancel/done
if (!targetController.choose(outcome, this, source, game)) {
chosen = isChosen(game);
return chosen;
break;
}
chosen = isChosen(game);
// stop by nothing to use (actual for human and done button)
if (prevTargetsCount == this.getTargets().size()) {
break;
}
```
The first condition ("stop by cancel/done") is actually never reached if you choose at least one card. The second is what allows the player to stop choosing by using "Done", but it doesn't work the first time it is used, and it doesn't seem that this is what it is for.
The thing is, unless I am missing something, **the `do{}while(true); `loops in `Target/TargetCardInLibrary.choose()` are actually useless**. They seem to be here to let the player choose several targets in succession, but the `HumanPlayer.choose/chooseTarget()` method called inside already does that.
Therefore there seems to be a rather easy fix in just removing the loop and the "nothing to use" condition. I have tried that, and it does seem to resolve the problem.
2. The second problem is simpler and concerns method `HumanPlayer.choose(outcome, cards, target, ability, game)`. Contrary to the way `HumanPlayer.choose(outcome, card, ability, game) `and other similar methods are coded, it returns `false` after "Done" is clicked. This is the relevant code :
```
if (responseId != null) {
...
} else {
// done or cancel button pressed
if (target.isChosen(game)) {
// try to finish
return false;
} else {
if (!required) {
...
```
This means that if less cards than max are chosen, the choose method will simply not return the correct answer (this was mentioned in #13691).
3. As mentioned in #11134, and event after the recent work in #13638, there are a lot of redundant methods that let a player choose cards. The `chooseTarget()` variants are, if I understand correctly, now outdated because that distinction is managed with `isNotTarget()`, but there is also the variants with the additional `Cards` argument, which seems to me strictly better than the one without. All of these are often slightly differently coded…
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.