Fix ChangeZoneEffect ordering to library; Aetherspouts prompt #5798
Conversation
Fix cards not showing up for Aetherspouts.
| final CardCollection reversed = new CardCollection(tgtCards); | ||
| Collections.reverse(reversed); | ||
| tgtCards = reversed; |
There was a problem hiding this comment.
@tool4ever i think the extra Reverse seems a bit ugly ... can we do something about this?
There was a problem hiding this comment.
Wasn't too happy with that part either, but I couldn't find an elegant way to reverse a CardCollectionView. If there's a better option though I'll swap it out.
There was a problem hiding this comment.
@tool4ever probably needs to look into the orderMoveToZoneList and orderCardsByTheirOwners functions, that for destination=Library already reverses the CardCollection
There was a problem hiding this comment.
Since this seems more of a convenience thing to select the order opposite from the zone changes it should probably belong to GUI code (PlayerControllerHuman)
or just switch the "bottom" + "top" labels around? :p
either way then all places in effect classes could be cleaned up
There was a problem hiding this comment.
It's a question of whether you want the output of orderMoveToZoneList to mean "the player wants the cards in this order" versus "this is the order you should move cards one-by-one into this zone and position". Whatever is settled on, it would probably be good idea to add a Javadoc comment explaining the function's behavior when ordering cards for the top of a deck.
There was a problem hiding this comment.
Second one makes more sense?
So all loops can be for each
There was a problem hiding this comment.
i don't know, we probably need more info like @tehdiplomat
There was a problem hiding this comment.
Ideally, we would have a different UI element thats a Scry type. Where you can reorder a library AND send to a different zone (including bottom of the library). I know people have had similar issues with Sylvan Library, where the ordering and the place on library didn't up matching their expectations. It seems weird to reverse the array when we already have labels to say which order things will be in.
Resolves #5466.
While looking for cards to test that issue, I ran into Aetherspouts, which I think badly needs a rework for UX purposes. Currently, it prompts you for the ordering of the cards, then prompts you one card at a time whether to send the card to the top or bottom. This causes the order on top to be reversed, much like Brainsurge was doing here, since it puts cards on top of the deck one at a time.
It also didn't specify which card you were moving for each individual prompt, meaning you had to memorize the order you put them in if you want to decide case by case. I fixed this last issue by adding support for
ShowCurrentCardto generic choices, but I think the implementation probably needs to be redone at some point. Possibly to one where you split into piles for the top and bottom first, then order them.