search

AEFeinstein / mtg-familiar

An Android app for all things Magic: The Gathering
Other
280 stars 72 forks source link

Won't return any results when searching for uncommon cards if android lang is set to Spanish (could also happen with other) #62

Closed gopat closed 9 years ago

gopat commented 9 years ago
gopat commented 9 years ago

Ok ladies and gentlemen, i think i may have found it; please take a look at:

Method parseForm is using first char of localized strings to search criteria ??? if thats whats happenning, it sounds broken. First char in "Infrecuente" is "I", not "U" (as in "Uncommon"). Looks like thats the only translation that differs, for now: if more tranlsations are added, this'll get bigger.

Trying to figure out a good way to fix this.

AEFeinstein commented 9 years ago

You are on the money with the root cause there. This code was written way before localization, which is why it's not smart. I believe the proper fix is to build a string in the CURMT style with logic that compares mRarityNames to getString(R.string.search_Common), getString(R.string.search_Uncommon), etc. That will iron out any localization spelling differences.

Awesome catch!

gopat commented 9 years ago

I don't really know, to be honest, i've never touched android developement before, and my java is waaaay old and basic. Here is what i had drafted:

diff --git a/MTGFamiliar/src/main/java/com/gelakinetic/mtgfam/fragments/SearchViewFragment.java b/MTGFamiliar/src/main/java/com/gelakinetic/mtgfam/fragments/SearchViewFragment.java
index c87c4ed..c5db8b5 100644
--- a/MTGFamiliar/src/main/java/com/gelakinetic/mtgfam/fragments/SearchViewFragment.java
+++ b/MTGFamiliar/src/main/java/com/gelakinetic/mtgfam/fragments/SearchViewFragment.java
@@ -63,7 +63,8 @@ public class SearchViewFragment extends FamiliarFragment {
    private boolean[] mSetChecked;
    private String[] mSetSymbols;
    private String[] mFormatNames;
-   private String[] mRarityNames;
+    private String[] mRarityCodes;
+    private String[] mRarityNames;
    private boolean[] mRarityChecked;
    private int mSelectedFormat;

@@ -144,7 +145,8 @@ public class SearchViewFragment extends FamiliarFragment {

         /* Get the different rarities out of resources to populate the list of choices with */
        Resources res = getResources();
-       mRarityNames = res.getStringArray(R.array.rarities);
+        mRarityCodes = res.getStringArray(R.array.rarity_codes);
+        mRarityNames = res.getStringArray(R.array.rarities);
        mRarityChecked = new boolean[mRarityNames.length];
    }

@@ -385,10 +387,10 @@ public class SearchViewFragment extends FamiliarFragment {
        for (int i = 0; i < mRarityChecked.length; i++) {
            if (mRarityChecked[i]) {
                if (searchCriteria.rarity == null) {
-                   searchCriteria.rarity = mRarityNames[i].charAt(0) + "";
+                   searchCriteria.rarity = mRarityCodes[i].charAt(0) + "";
                }
                else {
-                   searchCriteria.rarity += mRarityNames[i].charAt(0);
+                   searchCriteria.rarity += mRarityCodes[i].charAt(0);
                }
            }
        }
diff --git a/MTGFamiliar/src/main/res/values/arrays.xml b/MTGFamiliar/src/main/res/values/arrays.xml
index 8432f94..d472b05 100644
--- a/MTGFamiliar/src/main/res/values/arrays.xml
+++ b/MTGFamiliar/src/main/res/values/arrays.xml
@@ -26,6 +26,14 @@
         <item>&lt;</item>
         <item>=</item>
     </string-array>
+    <!-- do NOT localize this array, also, it must have the same length as "rarities", with matching elements, in same order -->
+    <string-array name="rarity_codes">
+        <item>C</item>
+        <item>U</item>
+        <item>R</item>
+        <item>M</item>
+        <item>T</item>
+    </string-array>
     <string-array name="rarities">
         <item>@string/search_Common</item>
         <item>@string/search_Uncommon</item>

But whatever... If it is fixed, i am happy ( else i'll cast a Worldfire! hahaha ).

Pros/Cons, correct way to do it? As a noob, i don't think i can ponder here.

I just don't want this to happen: homer_hunting_spiders

AEFeinstein commented 9 years ago

gopat, you're quickly becoming one of my favorite people on the internet. I like your solution, though it is a little overly complicated. You don't actually have to make a string array resource for C, U, R, M, T, since it should never be localized. It's simpler to just use:

private String[] mRarityCodes = {"C", "U", "R", "M", "T"}; /* This should stay in the same order as R.array.rarities */
gopat commented 9 years ago

That was my first thought, my second thought was that maybe those two pieces of data could have a "complicated relationship" if separated.

I considered that putting them in the same file would not add significant overhead, and would minimize the probability of disaster(IndexOutOfBoundsException or filter misaligned) would Hasbro/Wizards decide to add/remove rarity/ies in the future.

It results quite weird to me that there seems to be no API to define and get some kind of hash map in resources... Still diving in doc and investigating, don't know what getResourceName, getResourceEntryName getIdentifier really do....

AEFeinstein commented 9 years ago

Feel free to delve and learn. I think that if there are comments for each array mentioning the other, the probability of disaster would be low enough for me to be comfortable.

gopat commented 9 years ago

Well, after delving (good one) a little, stumbled upon res.obtainTypedArray(id) and this post wich shows a use case that's different from what we need here, but which still had pretty useful info for our evil plan.

With that knowledge and with res.getResourceEntryName(id) it's possible to get an array of corresponding resource names, so this is what i came up with : 

diff --git a/MTGFamiliar/src/main/java/com/gelakinetic/mtgfam/fragments/SearchViewFragment.java b/MTGFamiliar/src/main/java/com/gelakinetic/mtgfam/fragments/SearchViewFragment.java
index c87c4ed..0d04cb5 100644
--- a/MTGFamiliar/src/main/java/com/gelakinetic/mtgfam/fragments/SearchViewFragment.java
+++ b/MTGFamiliar/src/main/java/com/gelakinetic/mtgfam/fragments/SearchViewFragment.java
@@ -8,6 +8,7 @@ import android.content.res.Resources;
 import android.database.Cursor;
 import android.database.sqlite.SQLiteDatabase;
 import android.os.Bundle;
+import android.content.res.TypedArray;
 import android.view.KeyEvent;
 import android.view.LayoutInflater;
 import android.view.Menu;
@@ -63,6 +64,7 @@ public class SearchViewFragment extends FamiliarFragment {
    private boolean[] mSetChecked;
    private String[] mSetSymbols;
    private String[] mFormatNames;
+   private String[] mRarityCodes;
    private String[] mRarityNames;
    private boolean[] mRarityChecked;
    private int mSelectedFormat;
@@ -144,8 +146,21 @@ public class SearchViewFragment extends FamiliarFragment {

         /* Get the different rarities out of resources to populate the list of choices with */
        Resources res = getResources();
-       mRarityNames = res.getStringArray(R.array.rarities);
-       mRarityChecked = new boolean[mRarityNames.length];
+       TypedArray mRarityNamesTemp = res.obtainTypedArray(R.array.rarities);
+       int i = mRarityNamesTemp.length();
+       mRarityNames = new String[i];
+       mRarityCodes = new String[i];
+       mRarityChecked = new boolean[i];
+       while (i-- > 0) {
+           int resID = mRarityNamesTemp.peekValue(i).resourceId;
+           String resEntryName = res.getResourceEntryName(resID);
+           int p = resEntryName.lastIndexOf("_");
+           if (-1 != p && p + 1 < resEntryName.length())
+               mRarityCodes[i] = String.valueOf(resEntryName.charAt(p + 1));
+           else mRarityCodes[i] = "";
+           mRarityNames[i] = res.getString(resID);
+       }
+       mRarityNamesTemp.recycle();
    }

    /**
@@ -385,10 +400,10 @@ public class SearchViewFragment extends FamiliarFragment {
        for (int i = 0; i < mRarityChecked.length; i++) {
            if (mRarityChecked[i]) {
                if (searchCriteria.rarity == null) {
-                   searchCriteria.rarity = mRarityNames[i].charAt(0) + "";
+                   searchCriteria.rarity = mRarityCodes[i].charAt(0) + "";
                }
                else {
-                   searchCriteria.rarity += mRarityNames[i].charAt(0);
+                   searchCriteria.rarity += mRarityCodes[i].charAt(0);
                }
            }
        }
diff --git a/MTGFamiliar/src/main/res/values/arrays.xml b/MTGFamiliar/src/main/res/values/arrays.xml
index 8432f94..9f86403 100644
--- a/MTGFamiliar/src/main/res/values/arrays.xml
+++ b/MTGFamiliar/src/main/res/values/arrays.xml
@@ -26,6 +26,8 @@
         <item>&lt;</item>
         <item>=</item>
     </string-array>
+    <!-- Keep the character after last underscore in these references the same as the rarity code (C R U M T)
+         See strings.xml and onCreate method in SearchViewFragment -->
     <string-array name="rarities">
         <item>@string/search_Common</item>
         <item>@string/search_Uncommon</item>
diff --git a/MTGFamiliar/src/main/res/values/strings.xml b/MTGFamiliar/src/main/res/values/strings.xml
index 8cb0ba7..ff4725f 100644
--- a/MTGFamiliar/src/main/res/values/strings.xml
+++ b/MTGFamiliar/src/main/res/values/strings.xml
@@ -153,6 +153,8 @@ Media licenced under the &lt;a href="http://creativecommons.org/licenses/by-sa/3
    <string name="search_Must_include_all_colors">Must include all colors</string>
    <string name="search_No_other_colors">No other colors</string>
    <string name="search_Exact_all_selected_and_no_others">Exact (all selected and no others)</string>
+   <!-- Keep the character after last underscore in the name of rarities the same as the rarity code (C R U M T)
+        See arrays.xml and onCreate method in SearchViewFragment -->
    <string name="search_Common">Common</string>
    <string name="search_Uncommon">Uncommon</string>
    <string name="search_Rare">Rare</string>

A obviously more complex, but no extra data is required in java code nor in resource files ...as long as we can cope with strange requirements in the resource identifiers for those rarity strings.

Some thoughts: Initially i kept mRarityNames = res.getStringArray(R.array.rarities); and mRarityChecked = new boolean[mRarityNames.length]; but in the end i decided to refactor them: same data (rarities resource array length) is referenced and used the same for all arrays and unified source for length are good ideas, felt natural and will help keep consistency of array lengths over code changes.

Feedback, please.

AEFeinstein commented 9 years ago

I was going to mention that strange requirement in resource identifiers, but then i scrolled all the way down and you beat me to it. Nothing a can't fix.

The only other note I have is that mRarityCodes should probably be of type char[] instead of String[]. It's clean the code a little and save a trivial amount of space.

I'm ready for the pull request when you are.

gopat commented 9 years ago

Would it be else mRarityCodes[i] = ' '; ok?, using a space as the character value for the code to say: "ok, there's a error, may be in identifiers or something, but whatever i'm a piece of code, can't do wonders, take this meaningless data instead". Need a char (value) since strings can be empty but a char is a char.

AEFeinstein commented 9 years ago

' ' is how I would do it.

gopat commented 9 years ago

Ok i did not explain wery well on the last comment, and now i don't know exactly you will accept.

I'm on phone now, but i pushed the changes to my fork before, so i'll just make the pull request with what's in there so you can merge it if it is what you expected/meant. If it is not, i will correct it, push and do the pull request again when i arrive home.

AEFeinstein commented 9 years ago

Despite uncertainty, what was in the pull request was what I wanted. Thanks!

gopat commented 9 years ago

"One is happy to be of service". Also glad that i got it all ok, since my english is not what you'd call perfect, -and i'm not very good at explaining-.

PD: I'll try to take a look into a couple more things... but those are separate issues.

AEFeinstein commented 9 years ago

Your english is better than you think, and The Simpsons is a universal language. Glad to have you helping out!