Add issue explanations to lint text output

This CL adds a new option, explainIssues (true by default in the Gradle plugin)
which causes the plain text output of lint (which for example is used by the
lintVital task) to include full issue explanations in the error output.

This helps solve the problem that the one-line error message is often not
sufficient to explain a new issue to a developer, and it may not have been
obvious where to look for more info.

The output can be turned off with

android {
    lintOptions {
        explainIssues false
    }
}

Change-Id: I63190e893dfe366ed053d8705a94d7b73014670a
(cherry picked from commit 7ed600a5c0d7dba7aae23c14f965fe666367c78c)

9 files changed, 221 insertions, 9 deletions

diff --git a/build-system/builder-model/src/main/java/com/android/builder/model/LintOptions.java b/build-system/builder-model/src/main/java/com/android/builder/model/LintOptions.java
index c5ea2cc60b..df2863ed24 100644
--- a/build-system/builder-model/src/main/java/com/android/builder/model/LintOptions.java
+++ b/build-system/builder-model/src/main/java/com/android/builder/model/LintOptions.java
@@ -56,6 +56,8 @@ import java.util.Set;
  *          noLines true
  *          // if true, show all locations for an error, do not truncate lists, etc.
  *          showAll true
+ *          // whether lint should include full issue explanations in the text error output
+ *          explainIssues false
  *          // Fallback lint configuration (default severities, etc.)
  *          lintConfig file("default-lint.xml")
  *          // if true, generate a text report of issues (false by default)
@@ -137,6 +139,10 @@ public interface LintOptions {
     /** Returns whether lint should treat all warnings as errors */
     public boolean isWarningsAsErrors();
 
+    /** Returns whether lint should include explanations for issue errors. (Note that
+     * HTML and XML reports intentionally do this unconditionally, ignoring this setting.) */
+    public boolean isExplainIssues();
+
     /**
      * Returns whether lint should include all output (e.g. include all alternate
      * locations, not truncating long messages, etc.)
diff --git a/build-system/gradle/src/main/groovy/com/android/build/gradle/internal/dsl/LintOptionsImpl.groovy b/build-system/gradle/src/main/groovy/com/android/build/gradle/internal/dsl/LintOptionsImpl.groovy
index 123a7ba229..26b3348d92 100644
--- a/build-system/gradle/src/main/groovy/com/android/build/gradle/internal/dsl/LintOptionsImpl.groovy
+++ b/build-system/gradle/src/main/groovy/com/android/build/gradle/internal/dsl/LintOptionsImpl.groovy
@@ -63,6 +63,8 @@ public class LintOptionsImpl implements LintOptions, Serializable {
     private boolean showAll
     @Input
     private boolean checkReleaseBuilds = true;
+    @Input
+    private boolean explainIssues = true;
     @InputFile
     private File lintConfig
     @Input
@@ -102,6 +104,7 @@ public class LintOptionsImpl implements LintOptions, Serializable {
             boolean ignoreWarnings,
             boolean warningsAsErrors,
             boolean showAll,
+            boolean explainIssues,
             boolean checkReleaseBuilds,
             Map<String,LintOptions.Severity> severityOverrides) {
         this.disable = disable
@@ -122,6 +125,7 @@ public class LintOptionsImpl implements LintOptions, Serializable {
         this.ignoreWarnings = ignoreWarnings
         this.warningsAsErrors = warningsAsErrors
         this.showAll = showAll
+        this.explainIssues = explainIssues
         this.checkReleaseBuilds = checkReleaseBuilds
 
         if (severityOverrides != null) {
@@ -152,6 +156,7 @@ public class LintOptionsImpl implements LintOptions, Serializable {
                 source.isIgnoreWarnings(),
                 source.isWarningsAsErrors(),
                 source.isShowAll(),
+                source.isExplainIssues(),
                 source.isCheckReleaseBuilds(),
                 source.getSeverityOverrides()
         )
@@ -296,6 +301,14 @@ public class LintOptionsImpl implements LintOptions, Serializable {
         this.warningsAsErrors = allErrors
     }
 
+    public boolean isExplainIssues() {
+        return explainIssues
+    }
+
+    public void setExplainIssues(boolean explainIssues) {
+        this.explainIssues = explainIssues
+    }
+
     /**
      * Returns whether lint should include all output (e.g. include all alternate
      * locations, not truncating long messages, etc.)
@@ -415,6 +428,7 @@ public class LintOptionsImpl implements LintOptions, Serializable {
         flags.setShowEverything(showAll)
         flags.setDefaultConfiguration(lintConfig)
         flags.setSeverityOverrides(severities)
+        flags.setExplainIssues(explainIssues)
 
         if (report || flags.isFatalOnly()) {
             if (textReport || flags.isFatalOnly()) {
diff --git a/lint/cli/src/main/java/com/android/tools/lint/LintCliFlags.java b/lint/cli/src/main/java/com/android/tools/lint/LintCliFlags.java
index 64d1d03b05..0dcae04f71 100644
--- a/lint/cli/src/main/java/com/android/tools/lint/LintCliFlags.java
+++ b/lint/cli/src/main/java/com/android/tools/lint/LintCliFlags.java
@@ -52,6 +52,7 @@ public class LintCliFlags {
     private boolean mNoWarnings;
     private boolean mAllErrors;
     private boolean mFatalOnly;
+    private boolean mExplainIssues;
     private List<File> mSources;
     private List<File> mClasses;
     private List<File> mLibraries;
@@ -374,4 +375,24 @@ public class LintCliFlags {
     public void setSeverityOverrides(@NonNull Map<String, Severity> severities) {
         mSeverities = severities;
     }
+
+    /**
+     * Whether text reports should include full explanation texts. (HTML and XML reports always
+     * do, unconditionally.)
+     *
+     * @return true if text reports should include explanation text
+     */
+    public boolean isExplainIssues() {
+        return mExplainIssues;
+    }
+
+    /**
+     * Sets whether text reports should include full explanation texts. (HTML and XML reports
+     * always do, unconditionally.)
+     *
+     * @param explainText true if text reports should include explanation text
+     */
+    public void setExplainIssues(boolean explainText) {
+        mExplainIssues = explainText;
+    }
 }
diff --git a/lint/cli/src/main/java/com/android/tools/lint/TextReporter.java b/lint/cli/src/main/java/com/android/tools/lint/TextReporter.java
index 19e480d850..5afab495b6 100644
--- a/lint/cli/src/main/java/com/android/tools/lint/TextReporter.java
+++ b/lint/cli/src/main/java/com/android/tools/lint/TextReporter.java
@@ -16,11 +16,16 @@
 
 package com.android.tools.lint;
 
+import com.android.annotations.NonNull;
+import com.android.annotations.Nullable;
+import com.android.tools.lint.detector.api.Issue;
 import com.android.tools.lint.detector.api.Location;
 import com.android.tools.lint.detector.api.Position;
 import com.android.tools.lint.detector.api.Severity;
+import com.android.utils.SdkUtils;
 import com.google.common.annotations.Beta;
 import com.google.common.base.Joiner;
+import com.google.common.base.Splitter;
 
 import java.io.File;
 import java.io.IOException;
@@ -47,7 +52,8 @@ public class TextReporter extends Reporter {
      * @param writer the writer to write into
      * @param close whether the writer should be closed when done
      */
-    public TextReporter(LintCliClient client, LintCliFlags flags, Writer writer, boolean close) {
+    public TextReporter(@NonNull LintCliClient client, @NonNull LintCliFlags flags,
+            @NonNull Writer writer, boolean close) {
         this(client, flags, null, writer, close);
     }
 
@@ -60,8 +66,8 @@ public class TextReporter extends Reporter {
      * @param writer the writer to write into
      * @param close whether the writer should be closed when done
      */
-    public TextReporter(LintCliClient client, LintCliFlags flags, File file, Writer writer,
-            boolean close) {
+    public TextReporter(@NonNull LintCliClient client, @NonNull LintCliFlags flags,
+            @Nullable File file, @NonNull Writer writer, boolean close) {
         super(client, file);
         mFlags = flags;
         mWriter = writer;
@@ -80,7 +86,12 @@ public class TextReporter extends Reporter {
                 mWriter.flush();
             }
         } else {
+            Issue lastIssue = null;
             for (Warning warning : issues) {
+                if (warning.issue != lastIssue) {
+                    explainIssue(output, lastIssue);
+                    lastIssue = warning.issue;
+                }
                 int startLength = output.length();
 
                 if (warning.path != null) {
@@ -200,6 +211,7 @@ public class TextReporter extends Reporter {
                     output.append('\n');
                 }
             }
+            explainIssue(output, lastIssue);
 
             mWriter.write(output.toString());
 
@@ -218,6 +230,43 @@ public class TextReporter extends Reporter {
         }
     }
 
+    private void explainIssue(@NonNull StringBuilder output, @Nullable Issue issue)
+            throws IOException {
+        if (issue == null || !mFlags.isExplainIssues()) {
+            return;
+        }
+
+        String explanation = issue.getExplanation(Issue.OutputFormat.TEXT);
+        if (explanation.trim().isEmpty()) {
+            return;
+        }
+
+        String indent = "   ";
+        String formatted = SdkUtils.wrap(explanation, Main.MAX_LINE_WIDTH - indent.length(), null);
+        output.append('\n');
+        output.append(indent);
+        output.append("Explanation for issues of type \"").append(issue.getId()).append("\":\n");
+        for (String line : Splitter.on('\n').split(formatted)) {
+            if (!line.isEmpty()) {
+                output.append(indent);
+                output.append(line);
+            }
+            output.append('\n');
+        }
+        List<String> moreInfo = issue.getMoreInfo();
+        if (!moreInfo.isEmpty()) {
+            for (String url : moreInfo) {
+                if (formatted.contains(url)) {
+                    continue;
+                }
+                output.append(indent);
+                output.append(url);
+                output.append('\n');
+            }
+            output.append('\n');
+        }
+    }
+
     boolean isWriteToConsole() {
         return mOutput == null;
     }
diff --git a/lint/cli/src/test/java/com/android/tools/lint/HtmlReporterTest.java b/lint/cli/src/test/java/com/android/tools/lint/HtmlReporterTest.java
index f7b094cfc7..3ab4470e85 100644
--- a/lint/cli/src/test/java/com/android/tools/lint/HtmlReporterTest.java
+++ b/lint/cli/src/test/java/com/android/tools/lint/HtmlReporterTest.java
@@ -195,7 +195,7 @@ public class HtmlReporterTest extends AbstractCheckTest {
                     + "<br/>\n"
                     + "* The application cannot be translated to other languages by just adding new translations for existing string resources.<br/>\n"
                     + "<br/>\n"
-                    + "In Eclipse there is a quickfix to automatically extract this hardcoded string into a resource lookup.\n"
+                    + "In Android Studio and Eclipse there are quickfixes to automatically extract this hardcoded string into a resource lookup.\n"
                     + "</div>\n"
                     + "<br/><div class=\"moreinfo\">More info: </div><br/>To suppress this error, use the issue id \"HardcodedText\" as explained in the <a href=\"#SuppressInfo\">Suppressing Warnings and Errors</a> section.<br/>\n"
                     + "</div>\n"
diff --git a/lint/cli/src/test/java/com/android/tools/lint/TextReporterTest.java b/lint/cli/src/test/java/com/android/tools/lint/TextReporterTest.java
index 0f55173ecf..9726908f8c 100644
--- a/lint/cli/src/test/java/com/android/tools/lint/TextReporterTest.java
+++ b/lint/cli/src/test/java/com/android/tools/lint/TextReporterTest.java
@@ -30,6 +30,7 @@ import com.google.common.io.Files;
 import java.io.File;
 import java.io.FileWriter;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.List;
 
 public class TextReporterTest extends AbstractCheckTest {
@@ -87,6 +88,7 @@ public class TextReporterTest extends AbstractCheckTest {
             List<Warning> warnings = new ArrayList<Warning>();
             warnings.add(warning1);
             warnings.add(warning2);
+            Collections.sort(warnings);
 
             reporter.write(0, 2, warnings);
 
@@ -109,6 +111,126 @@ public class TextReporterTest extends AbstractCheckTest {
         }
     }
 
+    public void testWithExplanations() throws Exception {
+        File file = new File(getTargetDir(), "report");
+        try {
+            LintCliClient client = new LintCliClient() {
+                @Override
+                String getRevision() {
+                    return "unittest"; // Hardcode version to keep unit test output stable
+                }
+            };
+            //noinspection ResultOfMethodCallIgnored
+            file.getParentFile().mkdirs();
+            FileWriter writer = new FileWriter(file);
+            TextReporter reporter = new TextReporter(client, client.mFlags, file, writer, true);
+            client.mFlags.setExplainIssues(true);
+            Project project = Project.create(client, new File("/foo/bar/Foo"),
+                    new File("/foo/bar/Foo"));
+            client.mFlags.setShowEverything(true);
+
+            Warning warning1 = new Warning(ManifestDetector.USES_SDK,
+                    "<uses-sdk> tag should specify a target API level (the highest verified " +
+                            "version; when running on later versions, compatibility behaviors may " +
+                            "be enabled) with android:targetSdkVersion=\"?\"",
+                    Severity.WARNING, project, null);
+            warning1.line = 6;
+            warning1.file = new File("/foo/bar/Foo/AndroidManifest.xml");
+            warning1.errorLine = "    <uses-sdk android:minSdkVersion=\"8\" />\n    ^\n";
+            warning1.path = "AndroidManifest.xml";
+            warning1.location = Location.create(warning1.file,
+                    new DefaultPosition(6, 4, 198), new DefaultPosition(6, 42, 236));
+            Location secondary = Location.create(warning1.file,
+                    new DefaultPosition(7, 4, 198), new DefaultPosition(7, 42, 236));
+            secondary.setMessage("Secondary location");
+            warning1.location.setSecondary(secondary);
+
+            Warning warning2 = new Warning(HardcodedValuesDetector.ISSUE,
+                    "[I18N] Hardcoded string \"Fooo\", should use @string resource",
+                    Severity.WARNING, project, null);
+            warning2.line = 11;
+            warning2.file = new File("/foo/bar/Foo/res/layout/main.xml");
+            warning2.errorLine = "        android:text=\"Fooo\" />\n" +
+                    "        ~~~~~~~~~~~~~~~~~~~\n";
+            warning2.path = "res/layout/main.xml";
+            warning2.location = Location.create(warning2.file,
+                    new DefaultPosition(11, 8, 377), new DefaultPosition(11, 27, 396));
+            secondary = Location.create(warning1.file,
+                    new DefaultPosition(7, 4, 198), new DefaultPosition(7, 42, 236));
+            secondary.setMessage("Secondary location");
+            warning2.location.setSecondary(secondary);
+            Location tertiary = Location.create(warning2.file,
+                    new DefaultPosition(5, 4, 198), new DefaultPosition(5, 42, 236));
+            secondary.setSecondary(tertiary);
+
+            // Add another warning of the same type as warning 1 to make sure we
+            // (1) sort the warnings of the same issue together and (2) only print
+            // the explanation twice1
+            Warning warning3 = new Warning(ManifestDetector.USES_SDK,
+                    "<uses-sdk> tag should specify a target API level (the highest verified " +
+                            "version; when running on later versions, compatibility behaviors may " +
+                            "be enabled) with android:targetSdkVersion=\"?\"",
+                    Severity.WARNING, project, null);
+            warning3.line = 8;
+            warning3.file = new File("/foo/bar/Foo/AndroidManifest.xml");
+            warning3.errorLine = "    <uses-sdk android:minSdkVersion=\"8\" />\n    ^\n";
+            warning3.path = "AndroidManifest.xml";
+            warning3.location = Location.create(warning3.file,
+                    new DefaultPosition(8, 4, 198), new DefaultPosition(8, 42, 236));
+
+            List<Warning> warnings = new ArrayList<Warning>();
+            warnings.add(warning1);
+            warnings.add(warning2);
+            warnings.add(warning3);
+            Collections.sort(warnings);
+
+            reporter.write(0, 3, warnings);
+
+            String report = Files.toString(file, Charsets.UTF_8);
+            assertEquals(""
+                    + "AndroidManifest.xml:7: Warning: <uses-sdk> tag should specify a target API level (the highest verified version; when running on later versions, compatibility behaviors may be enabled) with android:targetSdkVersion=\"?\" [UsesMinSdkAttributes]\n"
+                    + "    <uses-sdk android:minSdkVersion=\"8\" />\n"
+                    + "    ^\n"
+                    + "    AndroidManifest.xml:8: Secondary location\n"
+                    + "AndroidManifest.xml:9: Warning: <uses-sdk> tag should specify a target API level (the highest verified version; when running on later versions, compatibility behaviors may be enabled) with android:targetSdkVersion=\"?\" [UsesMinSdkAttributes]\n"
+                    + "    <uses-sdk android:minSdkVersion=\"8\" />\n"
+                    + "    ^\n"
+                    + "\n"
+                    + "   Explanation for issues of type \"UsesMinSdkAttributes\":\n"
+                    + "   The manifest should contain a <uses-sdk> element which defines the minimum\n"
+                    + "   API Level required for the application to run, as well as the target\n"
+                    + "   version (the highest API level you have tested the version for.)\n"
+                    + "\n"
+                    + "   http://developer.android.com/guide/topics/manifest/uses-sdk-element.html\n"
+                    + "\n"
+                    + "res/layout/main.xml:12: Warning: [I18N] Hardcoded string \"Fooo\", should use @string resource [HardcodedText]\n"
+                    + "        android:text=\"Fooo\" />\n"
+                    + "        ~~~~~~~~~~~~~~~~~~~\n"
+                    + "    AndroidManifest.xml:8: Secondary location\n"
+                    + "Also affects: res/layout/main.xml:6\n"
+                    + "\n"
+                    + "   Explanation for issues of type \"HardcodedText\":\n"
+                    + "   Hardcoding text attributes directly in layout files is bad for several\n"
+                    + "   reasons:\n"
+                    + "\n"
+                    + "   * When creating configuration variations (for example for landscape or\n"
+                    + "   portrait)you have to repeat the actual text (and keep it up to date when\n"
+                    + "   making changes)\n"
+                    + "\n"
+                    + "   * The application cannot be translated to other languages by just adding\n"
+                    + "   new translations for existing string resources.\n"
+                    + "\n"
+                    + "   In Android Studio and Eclipse there are quickfixes to automatically extract\n"
+                    + "   this hardcoded string into a resource lookup.\n"
+                    + "\n"
+                    + "0 errors, 3 warnings\n",
+                    report);
+        } finally {
+            //noinspection ResultOfMethodCallIgnored
+            file.delete();
+        }
+    }
+
     @Override
     protected Detector getDetector() {
         fail("Not used in this test");
diff --git a/lint/cli/src/test/java/com/android/tools/lint/XmlReporterTest.java b/lint/cli/src/test/java/com/android/tools/lint/XmlReporterTest.java
index 4c17bfaceb..99d01c638d 100644
--- a/lint/cli/src/test/java/com/android/tools/lint/XmlReporterTest.java
+++ b/lint/cli/src/test/java/com/android/tools/lint/XmlReporterTest.java
@@ -119,7 +119,7 @@ public class XmlReporterTest extends AbstractCheckTest {
                 "\n" +
                 "* The application cannot be translated to other languages by just adding new translations for existing string resources.\n" +
                 "\n" +
-                "In Eclipse there is a quickfix to automatically extract this hardcoded string into a resource lookup.\"\n" +
+                "In Android Studio and Eclipse there are quickfixes to automatically extract this hardcoded string into a resource lookup.\"\n" +
                 "        errorLine1=\"        android:text=&quot;Fooo&quot; />\"\n" +
                 "        errorLine2=\"        ~~~~~~~~~~~~~~~~~~~\">\n" +
                 "        <location\n" +
@@ -223,7 +223,7 @@ public class XmlReporterTest extends AbstractCheckTest {
                 "\n" +
                 "* The application cannot be translated to other languages by just adding new translations for existing string resources.\n" +
                 "\n" +
-                "In Eclipse there is a quickfix to automatically extract this hardcoded string into a resource lookup.\"\n" +
+                "In Android Studio and Eclipse there are quickfixes to automatically extract this hardcoded string into a resource lookup.\"\n" +
                 "        errorLine1=\"        android:text=&quot;Fooo&quot; />\"\n" +
                 "        errorLine2=\"        ~~~~~~~~~~~~~~~~~~~\">\n" +
                 "        <location\n" +
diff --git a/lint/libs/lint-checks/src/main/java/com/android/tools/lint/checks/HardcodedValuesDetector.java b/lint/libs/lint-checks/src/main/java/com/android/tools/lint/checks/HardcodedValuesDetector.java
index e16f4b8cae..4f91c38736 100644
--- a/lint/libs/lint-checks/src/main/java/com/android/tools/lint/checks/HardcodedValuesDetector.java
+++ b/lint/libs/lint-checks/src/main/java/com/android/tools/lint/checks/HardcodedValuesDetector.java
@@ -59,8 +59,8 @@ public class HardcodedValuesDetector extends LayoutDetector {
             "* The application cannot be translated to other languages by just adding new " +
             "translations for existing string resources.\n" +
             "\n" +
-            "In Eclipse there is a quickfix to automatically extract this hardcoded string into " +
-            "a resource lookup.",
+            "In Android Studio and Eclipse there are quickfixes to automatically extract this " +
+            "hardcoded string into a resource lookup.",
 
             Category.I18N,
             5,
diff --git a/lint/libs/lint-checks/src/main/java/com/android/tools/lint/checks/ViewConstructorDetector.java b/lint/libs/lint-checks/src/main/java/com/android/tools/lint/checks/ViewConstructorDetector.java
index b8ccec1350..075c17694c 100644
--- a/lint/libs/lint-checks/src/main/java/com/android/tools/lint/checks/ViewConstructorDetector.java
+++ b/lint/libs/lint-checks/src/main/java/com/android/tools/lint/checks/ViewConstructorDetector.java
@@ -53,7 +53,7 @@ public class ViewConstructorDetector extends Detector implements Detector.ClassS
             "Missing View constructors for XML inflation",
             "Checks that custom views define the expected constructors",
 
-            "Some layout tools (such as the Android layout editor for Eclipse) needs to " +
+            "Some layout tools (such as the Android layout editor for Studio & Eclipse) needs to " +
             "find a constructor with one of the following signatures:\n" +
             "* `View(Context context)`\n" +
             "* `View(Context context, AttributeSet attrs)`\n" +