Cabal Therapy Window

[mark]

Hi, I have two suggestions for the window that opens for the Cabal Therapy effect:
1) The cardlist was sorted alphabetically but now all the cardnames are shuffled, so it is impossible to find a certain cardname in this list (in finite time ^^). Please restore the sorting of the list.
2) Please add a search field where I can enter the name of the card (or add other filters), because scrolling through 12k names is a little hard.

I don't know if there are some extended rules but in a real game I usually don't have a list of all cards available and should know the name by myself to use Cabal Therapy... maybe a window without a list and a (case insensitive, more or less fault-tolerant) inputbox is more realistic. Or you could enter a checkbox which enables/disables the list so every player can decide for himself.

Thanks and keep up the good work.

[friarsol]

I don't know if there are some extended rules but in a real game I usually don't have a list of all cards available and should know the name by myself to use Cabal Therapy... maybe a window without a list and a (case insensitive, more or less fault-tolerant) inputbox is more realistic. Or you could enter a checkbox which enables/disables the list so every player can decide for himself.

201.3. If an effect instructs a player to name a card, the player must choose the name of a card that exists in the Oracle card reference (see rule 108.1) and is legal in the format of the game the player is playing. (See rule 100.6.) If the player wants to name a split card, the player must name both halves of the split card. (See rule 708.) If the player wants to name a flip card’s alternative name, the player may do so. (See rule 709.) If the player wants to name the back face of a double-faced card, the player may do so. (See rule 711.) A player may not choose the name of a token unless it’s also the name of a card.

[myk]

Is there ever a case where we /don't/ want the list sorted in a call to GuiChoose.getChoices()? If we change the signature of the call to take a List instead of a Collection (only 3 or 4 usages send sets instead of lists, and they can easily be converted), we can sort the list before displaying it for all usages.
Edit: I see it also involves implementing the Comparable interface for a few more classes, but this does not seem difficult.

[Max mtg]

Is there ever a case where we /don't/ want the list sorted in a call to GuiChoose.getChoices()? If we change the signature of the call to take a List instead of a Collection (only 3 or 4 usages send sets instead of lists, and they can easily be converted), we can sort the list before displaying it for all usages.
Edit: I see it also involves implementing the Comparable interface for a few more classes, but this does not seem difficult.

Yes, this same dialog is used to show cards from all graveyards under string sub-headers, so it must not be re-arranged.
For that particular issue, a sorting should be done in something like 'NameCardEffect', it actually was there, but I removed (to save collection reallocation). Will put it back if it's needed.

No need in lists overloads. There is already an overload with parameters of type Collection<T> - lists prefectly fit in there.

[myk]

The overload change would be so the templated parameter could be passed to Collections.sort(). It would change from Collection<T> to List<T extends Comparable<? super T>>. I have actually gone through and got that change working, and it didn't require very many modifications to callers -- most passed lists of Comparables already. So, assuming that it is desirable to have at least /some/ (seems, most) of the calls to getChoices() present sorted lists, which is preferable, sort them before calling oneOrMany, oneOrNone, etc. (close to 300 references), or change the template (done, but not yet checked in) and pass a boolean (and perhaps a Comparator<T>). In the name of maintainability, I'd choose the latter.

[Max mtg]

There was a single request to sort the options list, so it would be enough to sort the incoming data there without influencing on the rest of callers. Moreover, in some scenarios sorting is undesirable.

[myk]

That's a fair point. Lots of changes means lots of potential for bugs. However, if the number of non-sortable calls is few, the changes should also be few. I had noticed the lack of sorting as well, especially when choosing from lists of cards on the battlefield or when interacting with the sideboard, and this was something I was going to get done anyway. Wouldn't it be an improvement for user experience in general to have a consistent presentation of data?

[Max mtg]

That's a fair point. Lots of changes means lots of potential for bugs. However, if the number of non-sortable calls is few, the changes should also be few. I had noticed the lack of sorting as well, especially when choosing from lists of cards on the battlefield or when interacting with the sideboard, and this was something I was going to get done anyway. Wouldn't it be an improvement for user experience in general to have a consistent presentation of data?

How will you estimate then the amount of calls affected and unaffected by the implicit sort you are suggesting?
How are you going to sort arrays of Objects? like the one coming from forge.game.GameActionPlay.getCostAfterConvoke(SpellAbility, ManaCostBeingPaid, SpellAbility)
How will you compare abilities, that are frequently passed to oneOrNone?
Which criteria will you use to sort QuestWorlds?

All these questions arise because sorting means a lot of things to do that might inluence some unexpected parts of project.

[myk]

Absolutely true. I don't want to cause bugs any more than you do. However, I don't want to shy away from useful functionality without reason either. So far I have prototyped the change from Collection<T> to List<T extends Comparable<? super T>> and so far it seems that this functionality can be introduced in a way that is low-risk. I have had to change only 5 effect classes, all of them trivially. The classes that needed to implement Comparable were:

1. ReplacementEffect
2. SpellAbility
3. GameFormat
4. QuestWorld

and Card needed a custom comparator to sort by name instead of its existing id comparison (which, of course, I left unchanged). The sort criteria I used for the four named classes was the name field (or the name of the related card, which would keep related items together). If future needs require a different implementation of compareTo that for whatever reason turns out to be unsuitable for sorting, a custom comparator can be written, such as what I have written for Card. getCostAfterConvoke() did give me pause. There is a string "Done" added to the list of cards. Selecting that string has the same effect as hitting cancel. For now, I just removed the string, but this is a place where a custom comparator would solve the problem as well.
The big question, and the one I could use some help answering, is whether the number of non-sortable calls is likely to be large. If we were to make this change, I would go through all of them, of course, but it would be nice to have a ballpark figure to consider before I start. I am hoping the existing API (with the template change) will service the grand majority of the calls, and would sort the incoming lists. A version of each method with two additional parameters: a Comparator and a boolean, would service the few calls that want special treatment. This way impact to the code will be kept low.

[Max mtg]

The classes that needed to implement Comparable were:

1. ReplacementEffect
2. SpellAbility
3. GameFormat
4. QuestWorld

and Card needed a custom comparator to sort by name instead of its existing id comparison (which, of course, I left unchanged). The sort criteria I used for the four named classes was the name field (or the name of the related card, which would keep related items together). If future needs require a different implementation of compareTo that for whatever reason turns out to be unsuitable for sorting, a custom comparator can be written, such as what I have written for Card. getCostAfterConvoke() did give me pause. There is a string "Done" added to the list of cards. Selecting that string has the same effect as hitting cancel. For now, I just removed the string, but this is a place where a custom comparator would solve the problem as well.
The big question, and the one I could use some help answering, is whether the number of non-sortable calls is likely to be large. If we were to make this change, I would go through all of them, of course, but it would be nice to have a ballpark figure to consider before I start. I am hoping the existing API (with the template change) will service the grand majority of the calls, and would sort the incoming lists. A version of each method with two additional parameters: a Comparator and a boolean, would service the few calls that want special treatment. This way impact to the code will be kept low.

I like the last suggestion - make an overload of each method that accepts the very comparator or boolean (to use default comparator when T extends Comparable<T>) as a parameter. This way you will be able to compare only the choices explicitly specified to be sorted, (having declared some static final Comparers to use). That overload might pre-sort the collection and then call a old method that does not sort objects.

As another example where sorting must not be applied is developer's feature "look at library". Library is ordered in its own way, it should be displayed in unchanged order.
Graveyard is also an ordered zone and it's avaliable to all players - so no ordering for that zone must be performed.

As for original subject of the topic - I have commited a change that orders cards in ChooseCardNameEffect - the what topicstarter asked for, they are even filtered to prevent invalid choices.

[myk]

I checked in a framework for sorting. I'll see if I can convert a few effects to use it -- mostly ones that ask you to choose a card from the battlefield, I think

[Max mtg]

Code: Select all

forge.gui.ListChooser.ListChooser(String, int, int, List<T>, boolean, Comparator<T>)

The lister chooser must not sort elements, as that is extra responsibility for that class.
Have them pre-sorted before showing the list, please.

That boolean parameter is redundant. It would be enough to supply null as comparator to disable sorting.

[friarsol]

It'd be nice if any popup asking you to choose a Format (most specifically the one that displays at the end of main world Quest win), would show in the same order they are listed in formats.txt. Most restrictive format to least restrictive format.

[Max mtg]

sol, I've made it for you.

[friarsol]

sol, I've made it for you.

Awesome thanks.