AAPT2: Clean up R JavaDoc generation

- Don't generate private attributes in public R.java
- Strip out @SystemApi from comment when generating @android.annotation.SystemApi
- Only emit a single line (up to the first period) of an attribute's comment within
  a styleable's attribute table.

Change-Id: Id6316a6861540325934133958939a12074ad4428

7 files changed, 85 insertions, 68 deletions

diff --git a/tools/aapt2/java/AnnotationProcessor.cpp b/tools/aapt2/java/AnnotationProcessor.cpp
index ba744395ece0..b7e7f903a2b1 100644
--- a/tools/aapt2/java/AnnotationProcessor.cpp
+++ b/tools/aapt2/java/AnnotationProcessor.cpp
@@ -21,7 +21,7 @@
 
 namespace aapt {
 
-void AnnotationProcessor::appendCommentLine(const std::string& comment) {
+void AnnotationProcessor::appendCommentLine(std::string& comment) {
     static const std::string sDeprecated = "@deprecated";
     static const std::string sSystemApi = "@SystemApi";
 
@@ -29,8 +29,14 @@ void AnnotationProcessor::appendCommentLine(const std::string& comment) {
         mAnnotationBitMask |= kDeprecated;
     }
 
-    if (comment.find(sSystemApi) != std::string::npos) {
+    std::string::size_type idx = comment.find(sSystemApi);
+    if (idx != std::string::npos) {
         mAnnotationBitMask |= kSystemApi;
+        comment.erase(comment.begin() + idx, comment.begin() + idx + sSystemApi.size());
+    }
+
+    if (util::trimWhitespace(comment).empty()) {
+        return;
     }
 
     if (!mHasComments) {
@@ -46,7 +52,8 @@ void AnnotationProcessor::appendComment(const StringPiece16& comment) {
     for (StringPiece16 line : util::tokenize(comment, u'\n')) {
         line = util::trimWhitespace(line);
         if (!line.empty()) {
-            appendCommentLine(util::utf16ToUtf8(line));
+            std::string utf8Line = util::utf16ToUtf8(line);
+            appendCommentLine(utf8Line);
         }
     }
 }
@@ -55,7 +62,8 @@ void AnnotationProcessor::appendComment(const StringPiece& comment) {
     for (StringPiece line : util::tokenize(comment, '\n')) {
         line = util::trimWhitespace(line);
         if (!line.empty()) {
-            appendCommentLine(line.toString());
+            std::string utf8Line = line.toString();
+            appendCommentLine(utf8Line);
         }
     }
 }
diff --git a/tools/aapt2/java/AnnotationProcessor.h b/tools/aapt2/java/AnnotationProcessor.h
index 0fc5b08e0bf3..8309dd978175 100644
--- a/tools/aapt2/java/AnnotationProcessor.h
+++ b/tools/aapt2/java/AnnotationProcessor.h
@@ -43,7 +43,6 @@ namespace aapt {
  *  /\*
  *   * This is meant to be hidden because
  *   * It is system api. Also it is @deprecated
- *   * @SystemApi
  *   *\/
  *
  * Output Annotations:
@@ -79,7 +78,7 @@ private:
     bool mHasComments = false;
     uint32_t mAnnotationBitMask = 0;
 
-    void appendCommentLine(const std::string& line);
+    void appendCommentLine(std::string& line);
 };
 
 } // namespace aapt
diff --git a/tools/aapt2/java/AnnotationProcessor_test.cpp b/tools/aapt2/java/AnnotationProcessor_test.cpp
index d3860a5825b2..5a39add48fbd 100644
--- a/tools/aapt2/java/AnnotationProcessor_test.cpp
+++ b/tools/aapt2/java/AnnotationProcessor_test.cpp
@@ -14,65 +14,37 @@
  * limitations under the License.
  */
 
-#include "ResourceParser.h"
-#include "ResourceTable.h"
-#include "ResourceValues.h"
 #include "java/AnnotationProcessor.h"
-#include "test/Builders.h"
-#include "test/Context.h"
-#include "xml/XmlPullParser.h"
-
-#include <gtest/gtest.h>
+#include "test/Test.h"
 
 namespace aapt {
 
-struct AnnotationProcessorTest : public ::testing::Test {
-    std::unique_ptr<IAaptContext> mContext;
-    ResourceTable mTable;
-
-    void SetUp() override {
-        mContext = test::ContextBuilder().build();
-    }
-
-    ::testing::AssertionResult parse(const StringPiece& str) {
-        ResourceParserOptions options;
-        ResourceParser parser(mContext->getDiagnostics(), &mTable, Source{}, ConfigDescription{},
-                              options);
-        std::stringstream in;
-        in << "<?xml version=\"1.0\" encoding=\"utf-8\"?>\n" << str;
-        xml::XmlPullParser xmlParser(in);
-        if (parser.parse(&xmlParser)) {
-            return ::testing::AssertionSuccess();
-        }
-        return ::testing::AssertionFailure();
-    }
-};
+TEST(AnnotationProcessorTest, EmitsDeprecated) {
+    const char* comment = "Some comment, and it should contain a marker word, "
+                          "something that marks this resource as nor needed. "
+                          "{@deprecated That's the marker! }";
 
-TEST_F(AnnotationProcessorTest, EmitsDeprecated) {
-    const char* xmlInput = R"EOF(
-    <resources>
-      <declare-styleable name="foo">
-        <!-- Some comment, and it should contain
-             a marker word, something that marks
-             this resource as nor needed.
-             {@deprecated That's the marker! } -->
-        <attr name="autoText" format="boolean" />
-      </declare-styleable>
-    </resources>)EOF";
+    AnnotationProcessor processor;
+    processor.appendComment(comment);
 
-    ASSERT_TRUE(parse(xmlInput));
+    std::stringstream result;
+    processor.writeToStream(&result, "");
+    std::string annotations = result.str();
 
-    Attribute* attr = test::getValue<Attribute>(&mTable, u"@attr/autoText");
-    ASSERT_NE(nullptr, attr);
+    EXPECT_NE(std::string::npos, annotations.find("@Deprecated"));
+}
 
+TEST(AnnotationProcessorTest, EmitsSystemApiAnnotationAndRemovesFromComment) {
     AnnotationProcessor processor;
-    processor.appendComment(attr->getComment());
+    processor.appendComment("@SystemApi This is a system API");
 
     std::stringstream result;
     processor.writeToStream(&result, "");
     std::string annotations = result.str();
 
-    EXPECT_NE(std::string::npos, annotations.find("@Deprecated"));
+    EXPECT_NE(std::string::npos, annotations.find("@android.annotation.SystemApi"));
+    EXPECT_EQ(std::string::npos, annotations.find("@SystemApi"));
+    EXPECT_NE(std::string::npos, annotations.find("This is a system API"));
 }
 
 } // namespace aapt
diff --git a/tools/aapt2/java/ClassDefinition.h b/tools/aapt2/java/ClassDefinition.h
index 53e0f6fe2f1e..d45328fedba2 100644
--- a/tools/aapt2/java/ClassDefinition.h
+++ b/tools/aapt2/java/ClassDefinition.h
@@ -125,7 +125,7 @@ public:
     void writeToStream(const StringPiece& prefix, bool final, std::ostream* out) const override {
         ClassMember::writeToStream(prefix, final, out);
 
-        *out << "public static final int[] " << mName << "={";
+        *out << prefix << "public static final int[] " << mName << "={";
 
         const auto begin = mElements.begin();
         const auto end = mElements.end();
diff --git a/tools/aapt2/java/JavaClassGenerator.cpp b/tools/aapt2/java/JavaClassGenerator.cpp
index 32b8600358b8..24347a1cdc1e 100644
--- a/tools/aapt2/java/JavaClassGenerator.cpp
+++ b/tools/aapt2/java/JavaClassGenerator.cpp
@@ -188,8 +188,8 @@ bool JavaClassGenerator::skipSymbol(SymbolState state) {
 
 struct StyleableAttr {
     const Reference* attrRef;
-    std::shared_ptr<Attribute> attribute;
     std::string fieldName;
+    std::unique_ptr<SymbolTable::Symbol> symbol;
 };
 
 static bool lessStyleableAttr(const StyleableAttr& lhs, const StyleableAttr& rhs) {
@@ -245,8 +245,9 @@ void JavaClassGenerator::addMembersToStyleableClass(const StringPiece16& package
         // legal values for this attribute.
         const SymbolTable::Symbol* symbol = mContext->getExternalSymbols()->findByReference(
                 mangledReference);
-        if (symbol) {
-            styleableAttr.attribute = symbol->attribute;
+        if (symbol && symbol->attribute) {
+            // Copy the symbol data structure because the returned instance can be destroyed.
+            styleableAttr.symbol = util::make_unique<SymbolTable::Symbol>(*symbol);
         }
         sortedAttributes.push_back(std::move(styleableAttr));
     }
@@ -273,6 +274,16 @@ void JavaClassGenerator::addMembersToStyleableClass(const StringPiece16& package
                 "<tr><th>Attribute</th><th>Description</th></tr>\n";
 
         for (const StyleableAttr& entry : sortedAttributes) {
+            if (!entry.symbol) {
+                continue;
+            }
+
+            if (mOptions.types == JavaClassGeneratorOptions::SymbolTypes::kPublic &&
+                    !entry.symbol->isPublic) {
+                // Don't write entries for non-public attributes.
+                continue;
+            }
+
             const ResourceName& attrName = entry.attrRef->name.value();
             styleableComment << "<tr><td>";
             styleableComment << "<code>{@link #"
@@ -284,14 +295,30 @@ void JavaClassGenerator::addMembersToStyleableClass(const StringPiece16& package
             styleableComment << "</td>";
 
             styleableComment << "<td>";
-            if (entry.attribute) {
-                styleableComment << entry.attribute->getComment();
+
+            // Only use the comment up until the first '.'. This is to stay compatible with
+            // the way old AAPT did it (presumably to keep it short and to avoid including
+            // annotations like @hide which would affect this Styleable).
+            StringPiece16 attrCommentLine = entry.symbol->attribute->getComment();
+            auto iter = std::find(attrCommentLine.begin(), attrCommentLine.end(), u'.');
+            if (iter != attrCommentLine.end()) {
+                attrCommentLine = attrCommentLine.substr(
+                        0, (iter - attrCommentLine.begin()) + 1);
             }
-            styleableComment << "</td></tr>\n";
+            styleableComment << attrCommentLine << "</td></tr>\n";
         }
         styleableComment << "</table>\n";
 
         for (const StyleableAttr& entry : sortedAttributes) {
+            if (!entry.symbol) {
+                continue;
+            }
+
+            if (mOptions.types == JavaClassGeneratorOptions::SymbolTypes::kPublic &&
+                    !entry.symbol->isPublic) {
+                // Don't write entries for non-public attributes.
+                continue;
+            }
             styleableComment << "@see #" << entry.fieldName << "\n";
         }
 
@@ -310,6 +337,17 @@ void JavaClassGenerator::addMembersToStyleableClass(const StringPiece16& package
     // Now we emit the indices into the array.
     for (size_t i = 0; i < attrCount; i++) {
         const StyleableAttr& styleableAttr = sortedAttributes[i];
+
+        if (!styleableAttr.symbol) {
+            continue;
+        }
+
+        if (mOptions.types == JavaClassGeneratorOptions::SymbolTypes::kPublic &&
+                !styleableAttr.symbol->isPublic) {
+            // Don't write entries for non-public attributes.
+            continue;
+        }
+
         const ResourceName& attrName = styleableAttr.attrRef->name.value();
 
         StringPiece16 packageName = attrName.package;
@@ -323,8 +361,8 @@ void JavaClassGenerator::addMembersToStyleableClass(const StringPiece16& package
         AnnotationProcessor* attrProcessor = indexMember->getCommentBuilder();
 
         StringPiece16 comment = styleableAttr.attrRef->getComment();
-        if (styleableAttr.attribute && comment.empty()) {
-            comment = styleableAttr.attribute->getComment();
+        if (styleableAttr.symbol->attribute && comment.empty()) {
+            comment = styleableAttr.symbol->attribute->getComment();
         }
 
         if (!comment.empty()) {
@@ -342,10 +380,8 @@ void JavaClassGenerator::addMembersToStyleableClass(const StringPiece16& package
 
         attrProcessor->appendNewLine();
 
-        if (styleableAttr.attribute) {
-            addAttributeFormatDoc(attrProcessor, styleableAttr.attribute.get());
-            attrProcessor->appendNewLine();
-        }
+        addAttributeFormatDoc(attrProcessor, styleableAttr.symbol->attribute.get());
+        attrProcessor->appendNewLine();
 
         std::stringstream doclavaName;
         doclavaName << "@attr name " << packageName << ":" << attrName.entry;;
diff --git a/tools/aapt2/java/JavaClassGenerator_test.cpp b/tools/aapt2/java/JavaClassGenerator_test.cpp
index 370e78a7b9d2..7d0aa83a0fb8 100644
--- a/tools/aapt2/java/JavaClassGenerator_test.cpp
+++ b/tools/aapt2/java/JavaClassGenerator_test.cpp
@@ -43,7 +43,8 @@ TEST(JavaClassGeneratorTest, TransformInvalidJavaIdentifierCharacter) {
     std::unique_ptr<ResourceTable> table = test::ResourceTableBuilder()
             .setPackageId(u"android", 0x01)
             .addSimple(u"@android:id/hey-man", ResourceId(0x01020000))
-            .addSimple(u"@android:attr/cool.attr", ResourceId(0x01010000))
+            .addValue(u"@android:attr/cool.attr", ResourceId(0x01010000),
+                      test::AttributeBuilder(false).build())
             .addValue(u"@android:styleable/hey.dude", ResourceId(0x01030000),
                       test::StyleableBuilder()
                               .addItem(u"@android:attr/cool.attr", ResourceId(0x01010000))
@@ -199,8 +200,10 @@ TEST(JavaClassGeneratorTest, EmitOtherPackagesAttributesInStyleable) {
     std::unique_ptr<ResourceTable> table = test::ResourceTableBuilder()
                 .setPackageId(u"android", 0x01)
                 .setPackageId(u"com.lib", 0x02)
-                .addSimple(u"@android:attr/bar", ResourceId(0x01010000))
-                .addSimple(u"@com.lib:attr/bar", ResourceId(0x02010000))
+                .addValue(u"@android:attr/bar", ResourceId(0x01010000),
+                          test::AttributeBuilder(false).build())
+                .addValue(u"@com.lib:attr/bar", ResourceId(0x02010000),
+                           test::AttributeBuilder(false).build())
                 .addValue(u"@android:styleable/foo", ResourceId(0x01030000),
                           test::StyleableBuilder()
                                   .addItem(u"@android:attr/bar", ResourceId(0x01010000))
diff --git a/tools/aapt2/java/ManifestClassGenerator_test.cpp b/tools/aapt2/java/ManifestClassGenerator_test.cpp
index e7210db8b743..d3bca7068cb2 100644
--- a/tools/aapt2/java/ManifestClassGenerator_test.cpp
+++ b/tools/aapt2/java/ManifestClassGenerator_test.cpp
@@ -126,7 +126,6 @@ R"EOF(    /**
 R"EOF(    /**
      * This is a private permission for system only!
      * @hide
-     * @SystemApi
      */
     @android.annotation.SystemApi
     public static final String SECRET="android.permission.SECRET";)EOF";