Suppress support and location tracking for translation detector

This changeset adds support for suppressing lint errors on
translations. For a missing translation, place the suppress attribute
on the default language key. For strings defined in other languages
but not the default locale, place the attribute on the extra
translation.

This required adding better location tracking (which is also
beneficial in that the translation warnings will show up as editor
underlines in Eclipse etc). Instead of having errors generated for
each locale ("the following strings are missing from locale X") it now
generates a unique error for each string, listing which locales it's
missing from. This also solves a different issue where the list of
missing strings was truncated when large; there's no more truncation
now.

Change-Id: If63e92a6a750a5314af2f11441347cf9fc0b0a15

6 files changed, 224 insertions, 82 deletions

diff --git a/eclipse/plugins/com.android.ide.eclipse.adt/src/com/android/ide/eclipse/adt/internal/lint/AddSuppressAttribute.java b/eclipse/plugins/com.android.ide.eclipse.adt/src/com/android/ide/eclipse/adt/internal/lint/AddSuppressAttribute.java
index 46d8e38..fde228b 100644
--- a/eclipse/plugins/com.android.ide.eclipse.adt/src/com/android/ide/eclipse/adt/internal/lint/AddSuppressAttribute.java
+++ b/eclipse/plugins/com.android.ide.eclipse.adt/src/com/android/ide/eclipse/adt/internal/lint/AddSuppressAttribute.java
@@ -29,7 +29,6 @@ import com.android.ide.eclipse.adt.internal.editors.IconFactory;
 import com.android.ide.eclipse.adt.internal.editors.layout.gle2.DomUtilities;
 import com.android.ide.eclipse.adt.internal.editors.uimodel.UiElementNode;
 import com.android.tools.lint.checks.DuplicateIdDetector;
-import com.android.tools.lint.checks.TranslationDetector;
 
 import org.eclipse.core.resources.IMarker;
 import org.eclipse.core.runtime.CoreException;
@@ -48,6 +47,7 @@ import org.w3c.dom.Node;
 /**
  * Fix for adding {@code tools:ignore="id"} attributes in XML files.
  */
+@SuppressWarnings("restriction") // DOM model
 class AddSuppressAttribute implements ICompletionProposal {
     private final AndroidXmlEditor mEditor;
     private final String mId;
@@ -128,7 +128,6 @@ class AddSuppressAttribute implements ICompletionProposal {
                         Display display = AdtPlugin.getDisplay();
                         if (display != null) {
                             display.asyncExec(new Runnable() {
-                                @SuppressWarnings("restriction") // DOM model
                                 @Override
                                 public void run() {
                                     Node xmlNode = uiNode.getXmlNode();
@@ -202,9 +201,7 @@ class AddSuppressAttribute implements ICompletionProposal {
         // available). Until that's resolved, we need to filter these out such that
         // we don't add misleading annotations on individual elements; the fallback
         // path is the DOM document itself instead.
-        if (id.equals(DuplicateIdDetector.CROSS_LAYOUT.getId())
-                || id.equals(TranslationDetector.MISSING.getId())
-                || id.equals(TranslationDetector.EXTRA.getId())) {
+        if (id.equals(DuplicateIdDetector.CROSS_LAYOUT.getId())) {
             node = document.getDocumentElement();
         }
 
diff --git a/lint/libs/lint_checks/src/com/android/tools/lint/checks/TranslationDetector.java b/lint/libs/lint_checks/src/com/android/tools/lint/checks/TranslationDetector.java
index efa53ad..5ed783a 100644
--- a/lint/libs/lint_checks/src/com/android/tools/lint/checks/TranslationDetector.java
+++ b/lint/libs/lint_checks/src/com/android/tools/lint/checks/TranslationDetector.java
@@ -29,7 +29,6 @@ import com.android.resources.ResourceFolderType;
 import com.android.tools.lint.detector.api.Category;
 import com.android.tools.lint.detector.api.Context;
 import com.android.tools.lint.detector.api.Issue;
-import com.android.tools.lint.detector.api.LintUtils;
 import com.android.tools.lint.detector.api.Location;
 import com.android.tools.lint.detector.api.ResourceXmlDetector;
 import com.android.tools.lint.detector.api.Scope;
@@ -52,7 +51,6 @@ import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
-import java.util.Map.Entry;
 import java.util.Set;
 import java.util.regex.Pattern;
 
@@ -65,7 +63,7 @@ public class TranslationDetector extends ResourceXmlDetector {
     static boolean COMPLETE_REGIONS =
             System.getenv("ANDROID_LINT_COMPLETE_REGIONS") != null; //$NON-NLS-1$
 
-    private static final Pattern lANGUAGE_PATTERN = Pattern.compile("^[a-z]{2}$"); //$NON-NLS-1$
+    private static final Pattern LANGUAGE_PATTERN = Pattern.compile("^[a-z]{2}$"); //$NON-NLS-1$
     private static final Pattern REGION_PATTERN = Pattern.compile("^r([A-Z]{2})$"); //$NON-NLS-1$
 
     /** Are all translations complete? */
@@ -103,6 +101,15 @@ public class TranslationDetector extends ResourceXmlDetector {
     private boolean mIgnoreFile;
     private Map<File, Set<String>> mFileToNames;
 
+    /** Locations for each untranslated string name. Populated during phase 2, if necessary */
+    private Map<String, Location> mMissingLocations;
+
+    /** Locations for each extra translated string name. Populated during phase 2, if necessary */
+    private Map<String, Location> mExtraLocations;
+
+    /** Error messages for each untranslated string name. Populated during phase 2, if necessary */
+    private Map<String, String> mDescriptions;
+
     /** Constructs a new {@link TranslationDetector} */
     public TranslationDetector() {
     }
@@ -127,12 +134,16 @@ public class TranslationDetector extends ResourceXmlDetector {
 
     @Override
     public void beforeCheckProject(Context context) {
-        mFileToNames = new HashMap<File, Set<String>>();
+        if (context.getDriver().getPhase() == 1) {
+            mFileToNames = new HashMap<File, Set<String>>();
+        }
     }
 
     @Override
     public void beforeCheckFile(Context context) {
-        mNames = new HashSet<String>();
+        if (context.getPhase() == 1) {
+            mNames = new HashSet<String>();
+        }
 
         // Convention seen in various projects
         mIgnoreFile = context.file.getName().startsWith("donottranslate"); //$NON-NLS-1$
@@ -140,21 +151,52 @@ public class TranslationDetector extends ResourceXmlDetector {
 
     @Override
     public void afterCheckFile(Context context) {
-        // Store this layout's set of ids for full project analysis in afterCheckProject
-        mFileToNames.put(context.file, mNames);
+        if (context.getPhase() == 1) {
+            // Store this layout's set of ids for full project analysis in afterCheckProject
+            mFileToNames.put(context.file, mNames);
 
-        mNames = null;
+            mNames = null;
+        }
     }
 
     @Override
     public void afterCheckProject(Context context) {
-        // NOTE - this will look for the presence of translation strings.
-        // If you create a resource folder but don't actually place a file in it
-        // we won't detect that, but it seems like a smaller problem.
+        if (context.getPhase() == 1) {
+            // NOTE - this will look for the presence of translation strings.
+            // If you create a resource folder but don't actually place a file in it
+            // we won't detect that, but it seems like a smaller problem.
+
+            checkTranslations(context);
 
-        checkTranslations(context);
+            mFileToNames = null;
 
-        mFileToNames = null;
+            if (mMissingLocations != null || mExtraLocations != null) {
+                context.getDriver().requestRepeat(this, Scope.ALL_RESOURCES_SCOPE);
+            }
+        } else {
+            assert context.getPhase() == 2;
+
+            reportMap(context, MISSING, mMissingLocations);
+            reportMap(context, EXTRA, mExtraLocations);
+            mMissingLocations = null;
+            mExtraLocations = null;
+            mDescriptions = null;
+        }
+    }
+
+    private void reportMap(Context context, Issue issue, Map<String, Location> map) {
+        if (map != null) {
+            for (Map.Entry<String, Location> entry : map.entrySet()) {
+                Location location = entry.getValue();
+                String name = entry.getKey();
+                String message = mDescriptions.get(name);
+
+                // We were prepending locations, but we want to prefer the base folders
+                location = Location.reverse(location);
+
+                context.report(issue, location, message, null);
+            }
+        }
     }
 
     private void checkTranslations(Context context) {
@@ -184,33 +226,7 @@ public class TranslationDetector extends ResourceXmlDetector {
             String name = parent.getName();
 
             // Look up the language for this folder.
-
-            String[] segments = name.split("-"); //$NON-NLS-1$
-
-            // TODO: To get an accurate answer, this should later do a
-            //   FolderConfiguration.getConfig(String[] folderSegments)
-            // to obtain a FolderConfiguration, then call
-            // getLanguageQualifier() on it, and if not null, call getValue() to get the
-            // actual language value.
-            // However, we don't have ide_common on the build path for lint, so for now
-            // use a simple guess about what constitutes a language qualifier here:
-
-            String language = null;
-            for (String segment : segments) {
-                // Language
-                if (language == null && segment.length() == 2
-                        && lANGUAGE_PATTERN.matcher(segment).matches()) {
-                    language = segment;
-                }
-
-                // Add in region
-                if (language != null && segment.length() == 3
-                        && REGION_PATTERN.matcher(segment).matches()) {
-                    language = language + '-' + segment;
-                    break;
-                }
-            }
-
+            String language = getLanguage(name);
             if (language == null) {
                 language = defaultLanguage;
             }
@@ -311,44 +327,78 @@ public class TranslationDetector extends ResourceXmlDetector {
                 if (reportMissing) {
                     Set<String> difference = Sets.difference(defaultStrings, strings);
                     if (difference.size() > 0) {
-                        List<String> sorted = new ArrayList<String>(difference);
-                        Collections.sort(sorted);
-                        Location location = getLocation(language, parentFolderToLanguage);
-                        int maxCount = context.getDriver().isAbbreviating() ? 4 : -1;
-                        // TODO: Compute applicable node scope
-                        context.report(MISSING, location,
-                            String.format("Locale %1$s is missing translations for: %2$s",
-                                language, LintUtils.formatList(sorted, maxCount)), null);
+                        if (mMissingLocations == null) {
+                            mMissingLocations = new HashMap<String, Location>();
+                        }
+                        if (mDescriptions == null) {
+                            mDescriptions = new HashMap<String, String>();
+                        }
+
+                        for (String s : difference) {
+                            mMissingLocations.put(s, null);
+                            String message = mDescriptions.get(s);
+                            if (message == null) {
+                                message = String.format("\"%1$s\" is not translated in %2$s",
+                                        s, language);
+                            } else {
+                                message = message + ", " + language;
+                            }
+                            mDescriptions.put(s, message);
+                        }
                     }
                 }
 
                 if (reportExtra) {
                     Set<String> difference = Sets.difference(strings, defaultStrings);
                     if (difference.size() > 0) {
-                        List<String> sorted = new ArrayList<String>(difference);
-                        Collections.sort(sorted);
-                        Location location = getLocation(language, parentFolderToLanguage);
-                        int maxCount = context.getDriver().isAbbreviating() ? 4 : -1;
-                        // TODO: Compute applicable node scope
-                        context.report(EXTRA, location, String.format(
-                              "Locale %1$s is translating names not found in default locale: %2$s",
-                              language, LintUtils.formatList(sorted, maxCount)), null);
+                        if (mExtraLocations == null) {
+                            mExtraLocations = new HashMap<String, Location>();
+                        }
+                        if (mDescriptions == null) {
+                            mDescriptions = new HashMap<String, String>();
+                        }
+
+                        for (String s : difference) {
+                            mExtraLocations.put(s, null);
+                            String message = String.format(
+                                "\"%1$s\" is translated here but not found in default locale", s);
+                            mDescriptions.put(s, message);
+                        }
                     }
                 }
             }
         }
     }
 
-    private Location getLocation(String language, Map<File, String> parentFolderToLanguage) {
-        Location location = null;
-        for (Entry<File, String> e : parentFolderToLanguage.entrySet()) {
-            if (e.getValue().equals(language)) {
-                // Use the location of the parent folder for this language
-                location = Location.create(e.getKey());
+    /** Look up the language for the given folder name */
+    private static String getLanguage(String name) {
+        String[] segments = name.split("-"); //$NON-NLS-1$
+
+        // TODO: To get an accurate answer, this should later do a
+        //   FolderConfiguration.getConfig(String[] folderSegments)
+        // to obtain a FolderConfiguration, then call
+        // getLanguageQualifier() on it, and if not null, call getValue() to get the
+        // actual language value.
+        // However, we don't have ide_common on the build path for lint, so for now
+        // use a simple guess about what constitutes a language qualifier here:
+
+        String language = null;
+        for (String segment : segments) {
+            // Language
+            if (language == null && segment.length() == 2
+                    && LANGUAGE_PATTERN.matcher(segment).matches()) {
+                language = segment;
+            }
+
+            // Add in region
+            if (language != null && segment.length() == 3
+                    && REGION_PATTERN.matcher(segment).matches()) {
+                language = language + '-' + segment;
                 break;
             }
         }
-        return location;
+
+        return language;
     }
 
     @Override
@@ -358,6 +408,42 @@ public class TranslationDetector extends ResourceXmlDetector {
         }
 
         Attr attribute = element.getAttributeNode(ATTR_NAME);
+
+        if (context.getPhase() == 2) {
+            // Just locating names requested in the {@link #mLocations} map
+            if (attribute == null) {
+                return;
+            }
+            String name = attribute.getValue();
+            if (mMissingLocations.containsKey(name)) {
+                String language = getLanguage(context.file.getParentFile().getName());
+                if (language == null) {
+                    if (context.getDriver().isSuppressed(MISSING, element)) {
+                        mMissingLocations.remove(name);
+                        return;
+                    }
+
+                    Location location = context.getLocation(element);
+                    location.setClientData(element);
+                    location.setSecondary(mMissingLocations.get(name));
+                    mMissingLocations.put(name, location);
+                }
+            }
+            if (mExtraLocations.containsKey(name)) {
+                if (context.getDriver().isSuppressed(EXTRA, element)) {
+                    mExtraLocations.remove(name);
+                    return;
+                }
+                Location location = context.getLocation(element);
+                location.setClientData(element);
+                location.setMessage("Also translated here");
+                location.setSecondary(mExtraLocations.get(name));
+                mExtraLocations.put(name, location);
+            }
+            return;
+        }
+
+        assert context.getPhase() == 1;
         if (attribute == null || attribute.getValue().length() == 0) {
             context.report(MISSING, element, context.getLocation(element),
                     "Missing name attribute in <string> declaration", null);
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/AbstractCheckTest.java b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/AbstractCheckTest.java
index 7f21477..4555420 100644
--- a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/AbstractCheckTest.java
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/AbstractCheckTest.java
@@ -258,6 +258,10 @@ public abstract class AbstractCheckTest extends TestCase {
         return false;
     }
 
+    protected boolean includeParentPath() {
+        return false;
+    }
+
     public class TestLintClient extends Main {
         private List<String> mErrors = new ArrayList<String>();
 
@@ -270,12 +274,16 @@ public abstract class AbstractCheckTest extends TestCase {
                 Object data) {
             StringBuilder sb = new StringBuilder();
 
+            if (issue == IssueRegistry.LINT_ERROR) {
+                return;
+            }
+
             if (location != null && location.getFile() != null) {
                 // Include parent directory for locations that have alternates, since
                 // frequently the file name is the same across different resource folders
                 // and we want to make sure in the tests that we're indeed passing the
                 // right files in as secondary locations
-                if (location.getSecondary() != null) {
+                if (location.getSecondary() != null || includeParentPath()) {
                     sb.append(location.getFile().getParentFile().getName() + "/"
                             + location.getFile().getName());
                 } else {
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/TranslationDetectorTest.java b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/TranslationDetectorTest.java
index 6a9b85b..9be7896 100644
--- a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/TranslationDetectorTest.java
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/TranslationDetectorTest.java
@@ -25,18 +25,21 @@ public class TranslationDetectorTest extends AbstractCheckTest {
         return new TranslationDetector();
     }
 
+    @Override
+    protected boolean includeParentPath() {
+        return true;
+    }
+
     public void testTranslation() throws Exception {
         TranslationDetector.COMPLETE_REGIONS = false;
         assertEquals(
             // Sample files from the Home app
-            "values-cs: Error: Locale cs is missing translations for: menu_settings\n" +
-            "values-de-rDE: Error: Locale de-rDE is missing translations for: menu_settings\n" +
-            "values-de-rDE: Warning: Locale de-rDE is translating names not found in default locale: continue_skip_label\n" +
-            "values-es-rUS: Error: Locale es-rUS is missing translations for: menu_settings\n" +
-            "values-es-rUS: Warning: Locale es-rUS is translating names not found in default locale: security_questions\n" +
-            "values-es: Error: Locale es is missing translations for: menu_settings\n" +
-            "values-es: Warning: Locale es is translating names not found in default locale: security_questions\n" +
-            "values-nl-rNL: Error: Locale nl-rNL is missing translations for: menu_settings, menu_wallpaper, show_all_apps",
+            "values-cs/arrays.xml:3: Warning: \"security_questions\" is translated here but not found in default locale\n" +
+            "=> values-es/strings.xml:12: Also translated here\n" +
+            "values-de-rDE/strings.xml:11: Warning: \"continue_skip_label\" is translated here but not found in default locale\n" +
+            "values/strings.xml:20: Error: \"show_all_apps\" is not translated in nl-rNL\n" +
+            "values/strings.xml:23: Error: \"menu_wallpaper\" is not translated in nl-rNL\n" +
+            "values/strings.xml:25: Error: \"menu_settings\" is not translated in cs, de-rDE, es, es-rUS, nl-rNL",
 
             lintProject(
                  "res/values/strings.xml",
@@ -45,6 +48,7 @@ public class TranslationDetectorTest extends AbstractCheckTest {
                  "res/values-es/strings.xml",
                  "res/values-es-rUS/strings.xml",
                  "res/values-land/strings.xml",
+                 "res/values-cs/arrays.xml",
                  "res/values-es/donottranslate.xml",
                  "res/values-nl-rNL/strings.xml"));
     }
@@ -53,11 +57,12 @@ public class TranslationDetectorTest extends AbstractCheckTest {
         TranslationDetector.COMPLETE_REGIONS = true;
         assertEquals(
             // Sample files from the Home app
-            "values-cs: Error: Locale cs is missing translations for: menu_settings\n" +
-            "values-de-rDE: Error: Locale de-rDE is missing translations for: menu_settings\n" +
-            "values-de-rDE: Warning: Locale de-rDE is translating names not found in default locale: continue_skip_label\n" +
-            "values-es-rUS: Error: Locale es-rUS is missing translations for: home_title, menu_settings, menu_wallpaper, show_all_apps... (1 more)\n" +
-            "values-nl-rNL: Error: Locale nl-rNL is missing translations for: menu_settings, menu_wallpaper, show_all_apps",
+            "values-de-rDE/strings.xml:11: Warning: \"continue_skip_label\" is translated here but not found in default locale\n" +
+            "values/strings.xml:19: Error: \"home_title\" is not translated in es-rUS\n" +
+            "values/strings.xml:20: Error: \"show_all_apps\" is not translated in es-rUS, nl-rNL\n" +
+            "values/strings.xml:23: Error: \"menu_wallpaper\" is not translated in es-rUS, nl-rNL\n" +
+            "values/strings.xml:25: Error: \"menu_settings\" is not translated in cs, de-rDE, es-rUS, nl-rNL\n" +
+            "values/strings.xml:29: Error: \"wallpaper_instructions\" is not translated in es-rUS",
 
             lintProject(
                  "res/values/strings.xml",
@@ -88,4 +93,15 @@ public class TranslationDetectorTest extends AbstractCheckTest {
                  "res/values/translatedarrays.xml",
                  "res/values-cs/translatedarrays.xml"));
     }
+
+    public void testTranslationSuppresss() throws Exception {
+        TranslationDetector.COMPLETE_REGIONS = false;
+        assertEquals(
+            "No warnings.",
+
+            lintProject(
+                    "res/values/strings_ignore.xml=>res/values/strings.xml",
+                    "res/values-es/strings_ignore.xml=>res/values-es/strings.xml",
+                    "res/values-nl-rNL/strings.xml=>res/values-nl-rNL/strings.xml"));
+    }
 }
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/res/values-es/strings_ignore.xml b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/res/values-es/strings_ignore.xml
new file mode 100644
index 0000000..4358448
--- /dev/null
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/res/values-es/strings_ignore.xml
@@ -0,0 +1,20 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<resources xmlns:android="http://schemas.android.com/apk/res/android"
+    xmlns:tools="http://schemas.android.com/tools"
+    xmlns:xliff="urn:oasis:names:tc:xliff:document:1.2">
+    <string name="home_title">"Casa"</string>
+    <string name="show_all_apps">"Todo"</string>
+    <string name="menu_wallpaper">"Papel tapiz"</string>
+    <string name="menu_search">"Búsqueda"</string>
+    <!-- no translation found for menu_settings (1769059051084007158) -->
+    <skip />
+    <string name="wallpaper_instructions">"Puntee en la imagen para establecer papel tapiz vertical"</string>
+    <string name="other" tools:ignore="ExtraTranslation">"?"</string>
+
+  <string-array name="security_questions" tools:ignore="ExtraTranslation">
+    <item>"Comida favorita"</item>
+    <item>"Ciudad de nacimiento"</item>
+    <item>"Nombre de tu mejor amigo/a de la infancia"</item>
+    <item>"Nombre de tu colegio"</item>
+  </string-array>
+</resources>
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/res/values/strings_ignore.xml b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/res/values/strings_ignore.xml
new file mode 100644
index 0000000..a69d4c8
--- /dev/null
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/res/values/strings_ignore.xml
@@ -0,0 +1,15 @@
+<?xml version="1.0" encoding="utf-8"?>
+<resources xmlns:tools="http://schemas.android.com/tools">
+    <!-- Home -->
+    <string name="home_title">Home Sample</string>
+    <string name="show_all_apps" tools:ignore="MissingTranslation">All</string>
+
+    <!-- Home Menus -->
+    <string name="menu_wallpaper" tools:ignore="MissingTranslation">Wallpaper</string>
+    <string name="menu_search">Search</string>
+    <string name="menu_settings" tools:ignore="all">Settings</string>
+    <string name="dummy" translatable="false">Ignore Me</string>
+
+    <!-- Wallpaper -->
+    <string name="wallpaper_instructions">Tap picture to set portrait wallpaper</string>
+</resources>