From 626b3dbf74f02ae630ae0089632f5962340694dc Mon Sep 17 00:00:00 2001 From: Adam Lesinski Date: Thu, 7 Apr 2016 13:24:59 -0700 Subject: [PATCH] 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 --- tools/aapt2/java/AnnotationProcessor.cpp | 16 +++-- tools/aapt2/java/AnnotationProcessor.h | 3 +- tools/aapt2/java/AnnotationProcessor_test.cpp | 66 ++++++------------- tools/aapt2/java/ClassDefinition.h | 2 +- tools/aapt2/java/JavaClassGenerator.cpp | 60 +++++++++++++---- tools/aapt2/java/JavaClassGenerator_test.cpp | 9 ++- .../java/ManifestClassGenerator_test.cpp | 1 - tools/aapt2/process/SymbolTable.h | 21 +++++- 8 files changed, 107 insertions(+), 71 deletions(-) diff --git a/tools/aapt2/java/AnnotationProcessor.cpp b/tools/aapt2/java/AnnotationProcessor.cpp index ba744395ece0b..b7e7f903a2b17 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 0fc5b08e0bf36..8309dd978175c 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 d3860a5825b2f..5a39add48fbd7 100644 --- a/tools/aapt2/java/AnnotationProcessor_test.cpp +++ b/tools/aapt2/java/AnnotationProcessor_test.cpp @@ -14,59 +14,18 @@ * 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 +#include "test/Test.h" namespace aapt { -struct AnnotationProcessorTest : public ::testing::Test { - std::unique_ptr 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 << "\n" << str; - xml::XmlPullParser xmlParser(in); - if (parser.parse(&xmlParser)) { - return ::testing::AssertionSuccess(); - } - return ::testing::AssertionFailure(); - } -}; - -TEST_F(AnnotationProcessorTest, EmitsDeprecated) { - const char* xmlInput = R"EOF( - - - - - - )EOF"; - - ASSERT_TRUE(parse(xmlInput)); - - Attribute* attr = test::getValue(&mTable, u"@attr/autoText"); - ASSERT_NE(nullptr, attr); +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! }"; AnnotationProcessor processor; - processor.appendComment(attr->getComment()); + processor.appendComment(comment); std::stringstream result; processor.writeToStream(&result, ""); @@ -75,6 +34,19 @@ TEST_F(AnnotationProcessorTest, EmitsDeprecated) { EXPECT_NE(std::string::npos, annotations.find("@Deprecated")); } +TEST(AnnotationProcessorTest, EmitsSystemApiAnnotationAndRemovesFromComment) { + AnnotationProcessor processor; + 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("@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 53e0f6fe2f1e7..d45328fedba29 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 32b8600358b83..24347a1cdc1ef 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; std::string fieldName; + std::unique_ptr 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(*symbol); } sortedAttributes.push_back(std::move(styleableAttr)); } @@ -273,6 +274,16 @@ void JavaClassGenerator::addMembersToStyleableClass(const StringPiece16& package "AttributeDescription\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 << ""; styleableComment << "{@link #" @@ -284,14 +295,30 @@ void JavaClassGenerator::addMembersToStyleableClass(const StringPiece16& package styleableComment << ""; styleableComment << ""; - 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 << "\n"; + styleableComment << attrCommentLine << "\n"; } styleableComment << "\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 370e78a7b9d2e..7d0aa83a0fb84 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 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 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 e7210db8b7439..d3bca7068cb24 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"; diff --git a/tools/aapt2/process/SymbolTable.h b/tools/aapt2/process/SymbolTable.h index 0a6a4a541a8d1..e684bb06f1f5b 100644 --- a/tools/aapt2/process/SymbolTable.h +++ b/tools/aapt2/process/SymbolTable.h @@ -51,9 +51,28 @@ class ISymbolSource; class SymbolTable { public: struct Symbol { + Symbol() : Symbol(Maybe{}) { + } + + Symbol(const Maybe& i) : Symbol(i, nullptr) { + } + + Symbol(const Maybe& i, const std::shared_ptr& attr) : + Symbol(i, attr, false) { + } + + Symbol(const Maybe& i, const std::shared_ptr& attr, bool pub) : + id(i), attribute(attr), isPublic(pub) { + } + + Symbol(const Symbol&) = default; + Symbol(Symbol&&) = default; + Symbol& operator=(const Symbol&) = default; + Symbol& operator=(Symbol&&) = default; + Maybe id; std::shared_ptr attribute; - bool isPublic; + bool isPublic = false; }; SymbolTable() : mCache(200), mIdCache(200) {