myk's code contributions

[myk]

Thanks for considering my help! Here are some of the things I thought I could usefully work on:

1) add tooltips to filter checkboxes in deck editor (bug 657)
2) stabilize order of filter checkboxes (currently they are populated by iterating over a HashMap, whose order changes between program runs due to random choice of hash function); change order of icons to "standard" as listed in the mtg rules book
3) make deck editor columns resizable and save/restore column widths
4) fix filter checkboxes not wrapping on initial display (bug 653)
5) save/restore state of quest plant/pet widgets

I have a patch ready for (1) and (2)

[friarsol]

Are you able to link to your diff somewhere or do the forums think you are too new to post a URL?

[myk]

sure thing -- try this: patch 1
Files modified:

• src/main/java/forge/gui/deckeditor/SFilterUtil.java
• src/main/java/forge/gui/deckeditor/views/VCardCatalog.java
• src/main/java/forge/gui/deckeditor/views/VCurrentDeck.java

the patch addresses a few minor items in the deck editor:
- adds tooltips for the filter checkboxes (issue 657)
- ensures the filter checkboxes are always in the same order
- puts all labels and checkboxes in "official" order from the mtg rules book:

• color order: WHITE, BLUE, BLACK, RED, GREEN, COLORLESS, MULTICOLOR
• type order: LAND, ARTIFACT, CREATURE, ENCHANTMENT, PLANESWALKER, INSTANT, SORCERY

[Max mtg]

Your changes look pretty good. Commited as 19148.

If you want to improve the code a little further, you may add some fields to enums ColorFilterProperty and TypeFilterProperty.
Those fields may include ImageIcon for this checkbox, Strings for tooltip of that very checkbox and the Predicate to apply when the checkbox is selected.
By adding fields to enum I mean something like what was done in forge.deck.DeckFormat enum.

This would allow to get rid of sheets like this one:

Code: Select all

        MAP_TYPE_CHECKBOXES.put(TypeFilterProperty.LAND,
                new ChbPnl(SEditorUtil.ICO_LAND.getImage(), "Land Cards"));
        MAP_TYPE_CHECKBOXES.put(TypeFilterProperty.ARTIFACT,
                new ChbPnl(SEditorUtil.ICO_ARTIFACT.getImage(), "Artifact Cards"));
        MAP_TYPE_CHECKBOXES.put(TypeFilterProperty.CREATURE,
                new ChbPnl(SEditorUtil.ICO_CREATURE.getImage(), "Creature Cards"));
        MAP_TYPE_CHECKBOXES.put(TypeFilterProperty.ENCHANTMENT,
                new ChbPnl(SEditorUtil.ICO_ENCHANTMENT.getImage(), "Enchantment Cards"));
        MAP_TYPE_CHECKBOXES.put(TypeFilterProperty.PLANESWALKER,
                new ChbPnl(SEditorUtil.ICO_PLANESWALKER.getImage(), "Planeswalker Cards"));
        MAP_TYPE_CHECKBOXES.put(TypeFilterProperty.INSTANT,
                new ChbPnl(SEditorUtil.ICO_INSTANT.getImage(), "Instant Cards"));
        MAP_TYPE_CHECKBOXES.put(TypeFilterProperty.SORCERY,
                new ChbPnl(SEditorUtil.ICO_SORCERY.getImage(), "Sorcery Cards"));


and replace them with something like

Code: Select all

for( TypeFilterProperty type : TypeFilterProperty.values() ) {
  MAP_TYPE_CHECKBOXES.put(type, new ChbPnl(type.getIcon().getImage(), type.getTooltip()));
}


[myk]

Awesome! Thanks! That's also a solid idea about moving the metadata into the enum. I'll see if I can get that done too.

[myk]

it looks like there was a slight merge discrepancy in VCurrentDeck that causes a panel to be non-opaque -- I'll fix it up in the next patch.

[myk]

patch 1.1: code improvements to filter enums
Files modified:

• src/main/java/forge/gui/deckeditor/SFilterUtil.java
• src/main/java/forge/gui/deckeditor/views/VCurrentDeck.java

implemented suggested improvements and fixed small merge error from last commit

[Max mtg]

Looks like you've produced some duplicate code in that patch (r19153) - the two enums are different classes, yet use the same fields
How are you going to handle that?

[myk]

that is true that there is some duplication. normally, they'd inherit from a base class, but Enums in Java can't do that. I'd have to think on it a bit to see if there's a more elegant solution.

[myk]

I played with a couple alternate representations of the data, but frankly, while they avoided code duplication, the supporting structures were far more complex and made for overall uglier code. There is an interesting technique called the extensible enum pattern (simpler explanation here), but, again, it made for more complex code than what is there already. I am definitely open to suggestions on cleaner ways to do it.

In the meantime, I picked up some of the other items I thought I could work on. Most were very simple changes:

patch 2: reapply card filters on deck editor controller change (issue 652)
Files modified:

• src/main/java/forge/gui/deckeditor/CDeckEditorUI.java

patch 3: make deck editor columns resizable
Files modified:

• src/main/java/forge/gui/deckeditor/tables/EditorTableModel.java
• src/main/java/forge/gui/deckeditor/tables/TableColumnInfo.java

- I looked into making the widths savable, and realized that they already are!
- I also fixed a NPE that occurs when the table is empty and the user clicks a column header

patch 4: fix filter checkboxes not wrapping on initial display (issue 653)
Files modified:

• src/main/java/forge/gui/WrapLayout.java

patch 5: save/restore state of quest plant/pet widgets
Files modified:

• src/main/java/forge/gui/home/quest/CSubmenuChallenges.java
• src/main/java/forge/gui/home/quest/CSubmenuDuels.java
• src/main/java/forge/quest/QuestController.java
• src/main/java/forge/quest/data/QuestData.java
• src/main/java/forge/quest/io/QuestDataIO.java

this one was a bit more involved:
- saved quest data when pet settings are modified
- moved the selectedPets map from QuestController to QuestData (since it now gets persisted); also renamed it petSlots for clarity
- incremented the quest file format version
- added code to bring older file formats up to date
- finally, while researching how the data is saved, I noticed that saveData() can be called from multiple threads. I made the method synchronized to prevent file corruption

[Max mtg]

Keep it simple! They both are the same entity, there's no real need for separate enums.
The dupliaction solution is: use a single enumeration, but declare 2 arrays filled with appropriate enum members for types and colors, then instead of iteration over all enum values, use the suitable array.

Patch 5: quest is saved on exit, so no need to save it in selected pet change.

For the rest I recommend to give you a commit permission.

UPD: you have to file a request at ucp.php?i=167

[myk]

patch 1.2 is available for your perusal : ) (though if you plan to give me commit access I'd be happy to apply it myself)

As for saving the quest on item selection in patch 5, according to my testing, it appears that the quest is not saved on exit, but rather on completion of actions that change its state (such as buying something in the bazaar). For example, if you launch forge, change the state of the 'plant' checkbox, exit forge, then launch forge again, you can see that the checkbox state was not preserved. I added the quest.save() calls in order to follow the existing code patterns. I presumed it was done this way to protect data in case of an unexpected program exit. Another code path is loading another quest, which also does not save the current quest.

[silly freak]

I'm a little late to this game, but just a tip for the future: In this particular use case, an Action might be what you want. It basically encapsulates data about a button (or checkbox), including text, icon, tooltip and more; and it is an ActionListener, so it can define what the button should do at the same time.

That way, even with two enums, the only duplication that would be needed is the single field for the action, not all the separate fields for text etc.

But it looks like you already took care of this particular problem; just keep it in mind if you have a similar usecase in the future

[Max mtg]

silly freak, do we have "Actions" in Forge incapsulating both control and its behaviour? Or have I missed something?

[Chris H.]

Thank you for your contributions Myk.

Doublestrike burned out during the UI overhaul. Most of the UI work is done but there are a few areas where we can tweak the code.

I just gave you commit privs and it may take as much as a day or two before the SVN is updated.