Issue 40 - Weapon & Armour comparison

[griff]

I've decided to take on Issue 40 as my first code contribution.

I chose to display the equipped item below the target item. If the target item happens to be equipped, then its name is updated to indicate that and the item is not repeated for compare.
The comparison will be displayed any time info is requested on an item (eg: in the shop buy screen, or in the player's inventory).



What do you think?

Technical Questions before I even consider a pull-request:

1) How do I move the word, " (Equipped)" into a translatable resource? (It is currently hard-coded in the ItemInfoActivity source file.)

2) I started with simplicity in implementation -- the iteminfo.xml file now has a second layout for the equipped item (I call it compare item) that is identical to the original. The ItemInfoActivity class has a method for filling the content of the compare item. This is a copy of the original code to fill the target item, except for the ids, thus breaking DRY.
* Is it worth creating a single fill method that takes a list of ids?
* OR is it worth changing the XML to be a collection of items (just for 2?) and thus simplifying making a single fill method?

[griff]

Figured out how easy it is to add the (Equipped) text as translatable resource.

As for DRY, I realized that creating a static method for each property simplifies the code tremendously. Each method takes a view object (mostly TextView) and an ItemType instance (among other things if needed).
(e.g.)

Code: Select all

        fillTitle(world, resources, (TextView) findViewById(R.id.iteminfo_title), itemType, (itemType == equippedType));
        fillDescription((TextView) findViewById(R.id.iteminfo_description), itemType);
        fillCategory((TextView) findViewById(R.id.iteminfo_category), itemType);
        fillItemEffects((ItemEffectsView) findViewById(R.id.iteminfo_effects), itemType);
        fillDisplayType(resources, (TextView) findViewById(R.id.iteminfo_displaytype), itemType);

        if (equippedType != null && equippedType != itemType){
            findViewById(R.id.compareinfo).setVisibility(View.VISIBLE);
            fillTitle(world, resources, (TextView) findViewById(R.id.compareinfo_title), equippedType, true);
            fillDescription((TextView) findViewById(R.id.compareinfo_description), equippedType);
            fillCategory((TextView) findViewById(R.id.compareinfo_category), equippedType);
            fillItemEffects((ItemEffectsView) findViewById(R.id.compareinfo_effects), equippedType);
            fillDisplayType(resources, (TextView) findViewById(R.id.compareinfo_displaytype), equippedType);
        } else {
            findViewById(R.id.compareinfo).setVisibility(View.GONE);
        }

I can still see a future request to compare a weapon against both equipped hands, in case the player is dual-wielding, or considering dual-wielding and wants to look at the current shield.

[ace]

it will hugely improve the UI. and make it item specific i.e., if one is buying a weapon he should be able to compare b/w these and if he is buying a shield then he must be comparing with the equipped shield and same for headgears, armors, shoes and gloves. just suggesting .

[rijackson741]

Don't forget that when dual wielding there can be two equipped weapons.

[Zukero]

Sounds like a good idea.
I'll check your code soon.
However, you should base your changes on my fork : https://github.com/Zukero/andors-trail
Thanks for working on it !

[griff]

As I've written it:
* It will show the corresponding equipped item -- eg helmet to helmet, boots to boots, etc.
* If you've dual-wielded and you look a shield, it will compare to the weapon in the off-hand.
* If you look at the weapon in your shield hand, it will compare to the weapon in your weapon hand.

All of this is a function of using the item code already in place.

For dual wielding scenarios, it might be useful to show what is equipped in both hands whenever looking at a weapon or shield. As far as I can tell, that will require a little bit of hackery in the code, since everything currently works from item definitions - that is no checks are made concerning what type of item you are looking at.


@Zukero: I already created a pull request on oskarwiksten's fork. Should I get your fork instead?

[Zukero]

Griff, oskar's github is deprecated, and quite a few commits behind.
Rebasing your changes on my latest "master" branch should be no problem ,as the files you changed haven't moved that much. You could then open your pr against my repo, as it is the reference currently.

[griff]

Done.

[Duke]

This will be a nice improvement to the user interface (UI), so for that I say thank you.

[Mayweed]

That looks great. Just one question:
What happens, if both items have a lot of attributes and the box is larger than the screen?
Or when you play in landscape mode? Is the text (or the box) scrollable?