Merge "AAPT2: Fix merging of styleables the right way"
diff --git a/tools/aapt2/ResourceTable.cpp b/tools/aapt2/ResourceTable.cpp
index ae5d299..21d2f64 100644
--- a/tools/aapt2/ResourceTable.cpp
+++ b/tools/aapt2/ResourceTable.cpp
@@ -201,8 +201,7 @@
}
/**
- * The default handler for collisions. A return value of -1 means keep the
- * existing value, 0 means fail, and +1 means take the incoming value.
+ * The default handler for collisions.
*
* Typically, a weak value will be overridden by a strong value. An existing weak
* value will not be overridden by an incoming weak value.
@@ -219,45 +218,34 @@
*
* A DECL will override a USE without error. Two DECLs must match in their format for there to be
* no error.
- *
- * Styleables: Styleables are not actual resources, but they are treated as such during the
- * compilation phase. Styleables are allowed to override each other, and their definitions merge
- * and accumulate. If both values are Styleables, we just merge them into the existing value.
*/
-int ResourceTable::resolveValueCollision(Value* existing, Value* incoming) {
- if (Styleable* existingStyleable = valueCast<Styleable>(existing)) {
- if (Styleable* incomingStyleable = valueCast<Styleable>(incoming)) {
- // Styleables get merged.
- existingStyleable->mergeWith(incomingStyleable);
- return -1;
- }
- }
-
+ResourceTable::CollisionResult ResourceTable::resolveValueCollision(
+ Value* existing, Value* incoming) {
Attribute* existingAttr = valueCast<Attribute>(existing);
Attribute* incomingAttr = valueCast<Attribute>(incoming);
if (!incomingAttr) {
if (incoming->isWeak()) {
// We're trying to add a weak resource but a resource
// already exists. Keep the existing.
- return -1;
+ return CollisionResult::kKeepOriginal;
} else if (existing->isWeak()) {
// Override the weak resource with the new strong resource.
- return 1;
+ return CollisionResult::kTakeNew;
}
// The existing and incoming values are strong, this is an error
// if the values are not both attributes.
- return 0;
+ return CollisionResult::kConflict;
}
if (!existingAttr) {
if (existing->isWeak()) {
// The existing value is not an attribute and it is weak,
// so take the incoming attribute value.
- return 1;
+ return CollisionResult::kTakeNew;
}
// The existing value is not an attribute and it is strong,
// so the incoming attribute value is an error.
- return 0;
+ return CollisionResult::kConflict;
}
assert(incomingAttr && existingAttr);
@@ -272,20 +260,20 @@
// The two attributes are both DECLs, but they are plain attributes
// with the same formats.
// Keep the strongest one.
- return existingAttr->isWeak() ? 1 : -1;
+ return existingAttr->isWeak() ? CollisionResult::kTakeNew : CollisionResult::kKeepOriginal;
}
if (existingAttr->isWeak() && existingAttr->typeMask == android::ResTable_map::TYPE_ANY) {
// Any incoming attribute is better than this.
- return 1;
+ return CollisionResult::kTakeNew;
}
if (incomingAttr->isWeak() && incomingAttr->typeMask == android::ResTable_map::TYPE_ANY) {
// The incoming attribute may be a USE instead of a DECL.
// Keep the existing attribute.
- return -1;
+ return CollisionResult::kKeepOriginal;
}
- return 0;
+ return CollisionResult::kConflict;
}
static constexpr const char* kValidNameChars = "._-";
@@ -367,7 +355,7 @@
const StringPiece& product,
std::unique_ptr<Value> value,
const char* validChars,
- const std::function<int(Value*,Value*)>& conflictResolver,
+ const CollisionResolverFunc& conflictResolver,
IDiagnostics* diag) {
assert(value && "value can't be nullptr");
assert(diag && "diagnostics can't be nullptr");
@@ -431,17 +419,22 @@
configValue->value = std::move(value);
} else {
- int collisionResult = conflictResolver(configValue->value.get(), value.get());
- if (collisionResult > 0) {
+ switch (conflictResolver(configValue->value.get(), value.get())) {
+ case CollisionResult::kTakeNew:
// Take the incoming value.
configValue->value = std::move(value);
- } else if (collisionResult == 0) {
+ break;
+
+ case CollisionResult::kConflict:
diag->error(DiagMessage(value->getSource())
- << "duplicate value for resource '" << name << "' "
- << "with config '" << config << "'");
+ << "duplicate value for resource '" << name << "' "
+ << "with config '" << config << "'");
diag->error(DiagMessage(configValue->value->getSource())
- << "resource previously defined here");
+ << "resource previously defined here");
return false;
+
+ case CollisionResult::kKeepOriginal:
+ break;
}
}
diff --git a/tools/aapt2/ResourceTable.h b/tools/aapt2/ResourceTable.h
index 6b52a43..df60814 100644
--- a/tools/aapt2/ResourceTable.h
+++ b/tools/aapt2/ResourceTable.h
@@ -38,8 +38,8 @@
enum class SymbolState {
kUndefined,
+ kPrivate,
kPublic,
- kPrivate
};
/**
@@ -185,13 +185,18 @@
public:
ResourceTable() = default;
+ enum class CollisionResult {
+ kKeepOriginal,
+ kConflict,
+ kTakeNew
+ };
+
+ using CollisionResolverFunc = std::function<CollisionResult(Value*, Value*)>;
+
/**
* When a collision of resources occurs, this method decides which value to keep.
- * Returns -1 if the existing value should be chosen.
- * Returns 0 if the collision can not be resolved (error).
- * Returns 1 if the incoming value should be chosen.
*/
- static int resolveValueCollision(Value* existing, Value* incoming);
+ static CollisionResult resolveValueCollision(Value* existing, Value* incoming);
bool addResource(const ResourceNameRef& name,
const ConfigDescription& config,
@@ -299,7 +304,7 @@
const StringPiece& product,
std::unique_ptr<Value> value,
const char* validChars,
- const std::function<int(Value*,Value*)>& conflictResolver,
+ const CollisionResolverFunc& conflictResolver,
IDiagnostics* diag);
bool setSymbolStateImpl(const ResourceNameRef& name,
diff --git a/tools/aapt2/ResourceTable_test.cpp b/tools/aapt2/ResourceTable_test.cpp
index feaca4e..4db40a6 100644
--- a/tools/aapt2/ResourceTable_test.cpp
+++ b/tools/aapt2/ResourceTable_test.cpp
@@ -118,39 +118,6 @@
EXPECT_FALSE(attr->isWeak());
}
-TEST(ResourceTable, MergeStyleables) {
- ResourceTable table;
-
- ASSERT_TRUE(table.addResource(
- test::parseNameOrDie("android:styleable/Foo"),
- ConfigDescription{}, "",
- test::StyleableBuilder()
- .addItem("android:attr/bar")
- .addItem("android:attr/foo")
- .build(),
- test::getDiagnostics()));
-
- ASSERT_TRUE(table.addResource(
- test::parseNameOrDie("android:styleable/Foo"),
- ConfigDescription{}, "",
- test::StyleableBuilder()
- .addItem("android:attr/bat")
- .addItem("android:attr/foo")
- .build(),
- test::getDiagnostics()));
-
- Styleable* styleable = test::getValue<Styleable>(&table, "android:styleable/Foo");
- ASSERT_NE(nullptr, styleable);
-
- std::vector<Reference> expectedRefs = {
- Reference(test::parseNameOrDie("android:attr/bar")),
- Reference(test::parseNameOrDie("android:attr/bat")),
- Reference(test::parseNameOrDie("android:attr/foo")),
- };
-
- EXPECT_EQ(expectedRefs, styleable->entries);
-}
-
TEST(ResourceTableTest, ProductVaryingValues) {
ResourceTable table;
diff --git a/tools/aapt2/ResourceValues.cpp b/tools/aapt2/ResourceValues.cpp
index 321f776..492155d 100644
--- a/tools/aapt2/ResourceValues.cpp
+++ b/tools/aapt2/ResourceValues.cpp
@@ -18,7 +18,6 @@
#include "ResourceUtils.h"
#include "ResourceValues.h"
#include "ValueVisitor.h"
-#include "io/File.h"
#include "util/Util.h"
#include <algorithm>
@@ -66,7 +65,7 @@
*out << "(raw string) " << *value;
}
-Reference::Reference() : referenceType(Reference::Type::kResource) {
+Reference::Reference() : referenceType(Type::kResource) {
}
Reference::Reference(const ResourceNameRef& n, Type t) :
@@ -76,6 +75,10 @@
Reference::Reference(const ResourceId& i, Type type) : id(i), referenceType(type) {
}
+Reference::Reference(const ResourceNameRef& n, const ResourceId& i) :
+ name(n.toResourceName()), id(i), referenceType(Type::kResource) {
+}
+
bool Reference::equals(const Value* value) const {
const Reference* other = valueCast<Reference>(value);
if (!other) {
@@ -747,8 +750,16 @@
return a.name != b.name || a.id != b.id;
}
+struct NameOnlyComparator {
+ bool operator()(const Reference& a, const Reference& b) const {
+ return a.name < b.name;
+ }
+};
+
void Styleable::mergeWith(Styleable* other) {
- std::set<Reference> references;
+ // Compare only names, because some References may already have their IDs assigned
+ // (framework IDs that don't change).
+ std::set<Reference, NameOnlyComparator> references;
references.insert(entries.begin(), entries.end());
references.insert(other->entries.begin(), other->entries.end());
entries.clear();
diff --git a/tools/aapt2/ResourceValues.h b/tools/aapt2/ResourceValues.h
index eb7559a..5e5d1f3 100644
--- a/tools/aapt2/ResourceValues.h
+++ b/tools/aapt2/ResourceValues.h
@@ -172,6 +172,7 @@
Reference();
explicit Reference(const ResourceNameRef& n, Type type = Type::kResource);
explicit Reference(const ResourceId& i, Type type = Type::kResource);
+ explicit Reference(const ResourceNameRef& n, const ResourceId& i);
bool equals(const Value* value) const override;
bool flatten(android::Res_value* outValue) const override;
diff --git a/tools/aapt2/flatten/TableFlattener.cpp b/tools/aapt2/flatten/TableFlattener.cpp
index 6a35e8c..d5067b1 100644
--- a/tools/aapt2/flatten/TableFlattener.cpp
+++ b/tools/aapt2/flatten/TableFlattener.cpp
@@ -83,19 +83,19 @@
void visit(Attribute* attr) override {
{
- Reference key = Reference(ResTable_map::ATTR_TYPE);
+ Reference key = Reference(ResourceId(ResTable_map::ATTR_TYPE));
BinaryPrimitive val(Res_value::TYPE_INT_DEC, attr->typeMask);
flattenEntry(&key, &val);
}
if (attr->minInt != std::numeric_limits<int32_t>::min()) {
- Reference key = Reference(ResTable_map::ATTR_MIN);
+ Reference key = Reference(ResourceId(ResTable_map::ATTR_MIN));
BinaryPrimitive val(Res_value::TYPE_INT_DEC, static_cast<uint32_t>(attr->minInt));
flattenEntry(&key, &val);
}
if (attr->maxInt != std::numeric_limits<int32_t>::max()) {
- Reference key = Reference(ResTable_map::ATTR_MAX);
+ Reference key = Reference(ResourceId(ResTable_map::ATTR_MAX));
BinaryPrimitive val(Res_value::TYPE_INT_DEC, static_cast<uint32_t>(attr->maxInt));
flattenEntry(&key, &val);
}
diff --git a/tools/aapt2/integration-tests/AppOne/res/values/test.xml b/tools/aapt2/integration-tests/AppOne/res/values/test.xml
index f4b7471..91f8bfd 100644
--- a/tools/aapt2/integration-tests/AppOne/res/values/test.xml
+++ b/tools/aapt2/integration-tests/AppOne/res/values/test.xml
@@ -29,4 +29,11 @@
<flag name="pub" value="2" />
<flag name="weak" value="4" />
</attr>
+
+ <!-- Override the Widget styleable declared in StaticLibOne.
+ This should merge the two when built in overlay mode. -->
+ <declare-styleable name="Widget">
+ <attr name="android:text" />
+ <attr name="layout_width" />
+ </declare-styleable>
</resources>
diff --git a/tools/aapt2/integration-tests/StaticLibOne/res/values/values.xml b/tools/aapt2/integration-tests/StaticLibOne/res/values/values.xml
index d09a485..b4dc90b 100644
--- a/tools/aapt2/integration-tests/StaticLibOne/res/values/values.xml
+++ b/tools/aapt2/integration-tests/StaticLibOne/res/values/values.xml
@@ -23,5 +23,6 @@
<declare-styleable name="Widget">
<attr name="StaticLibOne_attr" />
+ <attr name="android:text" />
</declare-styleable>
</resources>
diff --git a/tools/aapt2/link/TableMerger.cpp b/tools/aapt2/link/TableMerger.cpp
index 379c991..889ac70 100644
--- a/tools/aapt2/link/TableMerger.cpp
+++ b/tools/aapt2/link/TableMerger.cpp
@@ -45,49 +45,56 @@
}
/**
+ * Ignore packages with an ID that is not our desired package ID or 0x0, or if the name
+ * is not equal to the package we are compiling.
+ */
+static bool shouldIgnorePackage(IAaptContext* context, ResourceTablePackage* package) {
+ const Maybe<ResourceId>& id = package->id;
+ const std::string& packageName = package->name;
+ return (id && id.value() != 0x0 && id.value() != context->getPackageId())
+ || (!packageName.empty() && packageName != context->getCompilationPackage());
+}
+
+/**
* This will merge packages with the same package name (or no package name).
*/
bool TableMerger::mergeImpl(const Source& src, ResourceTable* table,
io::IFileCollection* collection,
bool overlay, bool allowNew) {
- const uint8_t desiredPackageId = mContext->getPackageId();
-
bool error = false;
for (auto& package : table->packages) {
- // Warn of packages with an unrelated ID.
- if (package->id && package->id.value() != 0x0 && package->id.value() != desiredPackageId) {
+ // Warn of packages with an unrelated ID or name.
+ if (shouldIgnorePackage(mContext, package.get())) {
mContext->getDiagnostics()->warn(DiagMessage(src)
<< "ignoring package " << package->name);
continue;
}
- if (package->name.empty() || mContext->getCompilationPackage() == package->name) {
- FileMergeCallback callback;
- if (collection) {
- callback = [&](const ResourceNameRef& name, const ConfigDescription& config,
- FileReference* newFile, FileReference* oldFile) -> bool {
- // The old file's path points inside the APK, so we can use it as is.
- io::IFile* f = collection->findFile(*oldFile->path);
- if (!f) {
- mContext->getDiagnostics()->error(DiagMessage(src) << "file '"
- << *oldFile->path
- << "' not found");
- return false;
- }
+ FileMergeCallback callback;
+ if (collection) {
+ callback = [&](const ResourceNameRef& name, const ConfigDescription& config,
+ FileReference* newFile, FileReference* oldFile) -> bool {
+ // The old file's path points inside the APK, so we can use it as is.
+ io::IFile* f = collection->findFile(*oldFile->path);
+ if (!f) {
+ mContext->getDiagnostics()->error(DiagMessage(src) << "file '"
+ << *oldFile->path
+ << "' not found");
+ return false;
+ }
- newFile->file = f;
- return true;
- };
- }
-
- // Merge here. Once the entries are merged and mangled, any references to
- // them are still valid. This is because un-mangled references are
- // mangled, then looked up at resolution time.
- // Also, when linking, we convert references with no package name to use
- // the compilation package name.
- error |= !doMerge(src, table, package.get(),
- false /* mangle */, overlay, allowNew, callback);
+ newFile->file = f;
+ return true;
+ };
}
+
+ // Merge here. Once the entries are merged and mangled, any references to
+ // them are still valid. This is because un-mangled references are
+ // mangled, then looked up at resolution time.
+ // Also, when linking, we convert references with no package name to use
+ // the compilation package name.
+ error |= !doMerge(src, table, package.get(), false /* mangle */, overlay, allowNew,
+ callback);
}
return !error;
}
@@ -129,6 +136,106 @@
return !error;
}
+static bool mergeType(IAaptContext* context, const Source& src, ResourceTableType* dstType,
+ ResourceTableType* srcType) {
+ if (dstType->symbolStatus.state < srcType->symbolStatus.state) {
+ // The incoming type's visibility is stronger, so we should override
+ // the visibility.
+ if (srcType->symbolStatus.state == SymbolState::kPublic) {
+ // Only copy the ID if the source is public, or else the ID is meaningless.
+ dstType->id = srcType->id;
+ }
+ dstType->symbolStatus = std::move(srcType->symbolStatus);
+ } else if (dstType->symbolStatus.state == SymbolState::kPublic
+ && srcType->symbolStatus.state == SymbolState::kPublic
+ && dstType->id && srcType->id
+ && dstType->id.value() != srcType->id.value()) {
+ // Both types are public and have different IDs.
+ context->getDiagnostics()->error(DiagMessage(src)
+ << "cannot merge type '" << srcType->type
+ << "': conflicting public IDs");
+ return false;
+ }
+ return true;
+}
+
+static bool mergeEntry(IAaptContext* context, const Source& src, ResourceEntry* dstEntry,
+ ResourceEntry* srcEntry) {
+ if (dstEntry->symbolStatus.state < srcEntry->symbolStatus.state) {
+ // The incoming type's visibility is stronger, so we should override
+ // the visibility.
+ if (srcEntry->symbolStatus.state == SymbolState::kPublic) {
+ // Only copy the ID if the source is public, or else the ID is meaningless.
+ dstEntry->id = srcEntry->id;
+ }
+ dstEntry->symbolStatus = std::move(srcEntry->symbolStatus);
+ } else if (srcEntry->symbolStatus.state == SymbolState::kPublic
+ && dstEntry->symbolStatus.state == SymbolState::kPublic
+ && dstEntry->id && srcEntry->id
+ && dstEntry->id.value() != srcEntry->id.value()) {
+ // Both entries are public and have different IDs.
+ context->getDiagnostics()->error(DiagMessage(src)
+ << "cannot merge entry '" << srcEntry->name
+ << "': conflicting public IDs");
+ return false;
+ }
+ return true;
+}
+
+/**
+ * Modified CollisionResolver which will merge Styleables. Used with overlays.
+ *
+ * Styleables are not actual resources, but they are treated as such during the
+ * compilation phase. Styleables don't simply overlay each other, their definitions merge
+ * and accumulate. If both values are Styleables, we just merge them into the existing value.
+ */
+static ResourceTable::CollisionResult resolveMergeCollision(Value* existing, Value* incoming) {
+ if (Styleable* existingStyleable = valueCast<Styleable>(existing)) {
+ if (Styleable* incomingStyleable = valueCast<Styleable>(incoming)) {
+ // Styleables get merged.
+ existingStyleable->mergeWith(incomingStyleable);
+ return ResourceTable::CollisionResult::kKeepOriginal;
+ }
+ }
+ // Delegate to the default handler.
+ return ResourceTable::resolveValueCollision(existing, incoming);
+}
+
+static ResourceTable::CollisionResult mergeConfigValue(IAaptContext* context,
+ const ResourceNameRef& resName,
+ const bool overlay,
+ ResourceConfigValue* dstConfigValue,
+ ResourceConfigValue* srcConfigValue) {
+ using CollisionResult = ResourceTable::CollisionResult;
+
+ Value* dstValue = dstConfigValue->value.get();
+ Value* srcValue = srcConfigValue->value.get();
+
+ CollisionResult collisionResult;
+ if (overlay) {
+ collisionResult = resolveMergeCollision(dstValue, srcValue);
+ } else {
+ collisionResult = ResourceTable::resolveValueCollision(dstValue, srcValue);
+ }
+
+ if (collisionResult == CollisionResult::kConflict) {
+ if (overlay) {
+ return CollisionResult::kTakeNew;
+ }
+
+ // Error!
+ context->getDiagnostics()->error(DiagMessage(srcValue->getSource())
+ << "resource '" << resName
+ << "' has a conflicting value for "
+ << "configuration ("
+ << srcConfigValue->config << ")");
+ context->getDiagnostics()->note(DiagMessage(dstValue->getSource())
+ << "originally defined here");
+ return CollisionResult::kConflict;
+ }
+ return collisionResult;
+}
+
bool TableMerger::doMerge(const Source& src,
ResourceTable* srcTable,
ResourceTablePackage* srcPackage,
@@ -140,115 +247,64 @@
for (auto& srcType : srcPackage->types) {
ResourceTableType* dstType = mMasterPackage->findOrCreateType(srcType->type);
- if (srcType->symbolStatus.state == SymbolState::kPublic) {
- if (dstType->symbolStatus.state == SymbolState::kPublic && dstType->id && srcType->id
- && dstType->id.value() != srcType->id.value()) {
- // Both types are public and have different IDs.
- mContext->getDiagnostics()->error(DiagMessage(src)
- << "can not merge type '"
- << srcType->type
- << "': conflicting public IDs");
- error = true;
- continue;
- }
-
- dstType->symbolStatus = std::move(srcType->symbolStatus);
- dstType->id = srcType->id;
+ if (!mergeType(mContext, src, dstType, srcType.get())) {
+ error = true;
+ continue;
}
for (auto& srcEntry : srcType->entries) {
- ResourceEntry* dstEntry;
+ std::string entryName = srcEntry->name;
if (manglePackage) {
- std::string mangledName = NameMangler::mangleEntry(srcPackage->name,
- srcEntry->name);
- if (allowNewResources) {
- dstEntry = dstType->findOrCreateEntry(mangledName);
- } else {
- dstEntry = dstType->findEntry(mangledName);
- }
- } else {
- if (allowNewResources) {
- dstEntry = dstType->findOrCreateEntry(srcEntry->name);
- } else {
- dstEntry = dstType->findEntry(srcEntry->name);
- }
+ entryName = NameMangler::mangleEntry(srcPackage->name, srcEntry->name);
}
+ ResourceEntry* dstEntry;
+ if (allowNewResources) {
+ dstEntry = dstType->findOrCreateEntry(entryName);
+ } else {
+ dstEntry = dstType->findEntry(entryName);
+ }
+
+ const ResourceNameRef resName(srcPackage->name, srcType->type, srcEntry->name);
+
if (!dstEntry) {
mContext->getDiagnostics()->error(DiagMessage(src)
- << "resource "
- << ResourceNameRef(srcPackage->name,
- srcType->type,
- srcEntry->name)
+ << "resource " << resName
<< " does not override an existing resource");
mContext->getDiagnostics()->note(DiagMessage(src)
<< "define an <add-resource> tag or use "
- "--auto-add-overlay");
+ << "--auto-add-overlay");
error = true;
continue;
}
- if (srcEntry->symbolStatus.state != SymbolState::kUndefined) {
- if (srcEntry->symbolStatus.state == SymbolState::kPublic) {
- if (dstEntry->symbolStatus.state == SymbolState::kPublic &&
- dstEntry->id && srcEntry->id &&
- dstEntry->id.value() != srcEntry->id.value()) {
- // Both entries are public and have different IDs.
- mContext->getDiagnostics()->error(DiagMessage(src)
- << "can not merge entry '"
- << srcEntry->name
- << "': conflicting public IDs");
- error = true;
- continue;
- }
-
- if (srcEntry->id) {
- dstEntry->id = srcEntry->id;
- }
- }
-
- if (dstEntry->symbolStatus.state != SymbolState::kPublic &&
- dstEntry->symbolStatus.state != srcEntry->symbolStatus.state) {
- dstEntry->symbolStatus = std::move(srcEntry->symbolStatus);
- }
+ if (!mergeEntry(mContext, src, dstEntry, srcEntry.get())) {
+ error = true;
+ continue;
}
- ResourceNameRef resName(mMasterPackage->name, dstType->type, dstEntry->name);
+ for (auto& srcConfigValue : srcEntry->values) {
+ using CollisionResult = ResourceTable::CollisionResult;
- for (auto& srcValue : srcEntry->values) {
- ResourceConfigValue* dstValue = dstEntry->findValue(srcValue->config,
- srcValue->product);
- if (dstValue) {
- const int collisionResult = ResourceTable::resolveValueCollision(
- dstValue->value.get(), srcValue->value.get());
- if (collisionResult == 0 && !overlay) {
- // Error!
- ResourceNameRef resourceName(srcPackage->name,
- srcType->type,
- srcEntry->name);
-
- mContext->getDiagnostics()->error(DiagMessage(srcValue->value->getSource())
- << "resource '" << resourceName
- << "' has a conflicting value for "
- << "configuration ("
- << srcValue->config << ")");
- mContext->getDiagnostics()->note(DiagMessage(dstValue->value->getSource())
- << "originally defined here");
+ ResourceConfigValue* dstConfigValue = dstEntry->findValue(srcConfigValue->config,
+ srcConfigValue->product);
+ if (dstConfigValue) {
+ CollisionResult collisionResult = mergeConfigValue(
+ mContext, resName, overlay, dstConfigValue, srcConfigValue.get());
+ if (collisionResult == CollisionResult::kConflict) {
error = true;
continue;
- } else if (collisionResult < 0) {
- // Keep our existing value.
+ } else if (collisionResult == CollisionResult::kKeepOriginal) {
continue;
}
-
+ } else {
+ dstConfigValue = dstEntry->findOrCreateValue(srcConfigValue->config,
+ srcConfigValue->product);
}
- if (!dstValue) {
- // Force create the entry if we didn't have it.
- dstValue = dstEntry->findOrCreateValue(srcValue->config, srcValue->product);
- }
+ // Continue if we're taking the new resource.
- if (FileReference* f = valueCast<FileReference>(srcValue->value.get())) {
+ if (FileReference* f = valueCast<FileReference>(srcConfigValue->value.get())) {
std::unique_ptr<FileReference> newFileRef;
if (manglePackage) {
newFileRef = cloneAndMangleFile(srcPackage->name, *f);
@@ -258,15 +314,15 @@
}
if (callback) {
- if (!callback(resName, srcValue->config, newFileRef.get(), f)) {
+ if (!callback(resName, srcConfigValue->config, newFileRef.get(), f)) {
error = true;
continue;
}
}
- dstValue->value = std::move(newFileRef);
+ dstConfigValue->value = std::move(newFileRef);
} else {
- dstValue->value = std::unique_ptr<Value>(srcValue->value->clone(
+ dstConfigValue->value = std::unique_ptr<Value>(srcConfigValue->value->clone(
&mMasterTable->stringPool));
}
}
@@ -277,7 +333,6 @@
std::unique_ptr<FileReference> TableMerger::cloneAndMangleFile(const std::string& package,
const FileReference& fileRef) {
-
StringPiece prefix, entry, suffix;
if (util::extractResFilePathParts(*fileRef.path, &prefix, &entry, &suffix)) {
std::string mangledEntry = NameMangler::mangleEntry(package, entry.toString());
diff --git a/tools/aapt2/link/TableMerger_test.cpp b/tools/aapt2/link/TableMerger_test.cpp
index ff3e21f..fb1cb21 100644
--- a/tools/aapt2/link/TableMerger_test.cpp
+++ b/tools/aapt2/link/TableMerger_test.cpp
@@ -274,4 +274,43 @@
ASSERT_FALSE(merger.mergeOverlay({}, tableB.get()));
}
+TEST_F(TableMergerTest, OverlaidStyleablesShouldBeMerged) {
+ std::unique_ptr<ResourceTable> tableA = test::ResourceTableBuilder()
+ .setPackageId("com.app.a", 0x7f)
+ .addValue("com.app.a:styleable/Foo", test::StyleableBuilder()
+ .addItem("com.app.a:attr/bar")
+ .addItem("com.app.a:attr/foo", ResourceId(0x01010000))
+ .build())
+ .build();
+
+ std::unique_ptr<ResourceTable> tableB = test::ResourceTableBuilder()
+ .setPackageId("com.app.a", 0x7f)
+ .addValue("com.app.a:styleable/Foo", test::StyleableBuilder()
+ .addItem("com.app.a:attr/bat")
+ .addItem("com.app.a:attr/foo")
+ .build())
+ .build();
+
+ ResourceTable finalTable;
+ TableMergerOptions options;
+ options.autoAddOverlay = true;
+ TableMerger merger(mContext.get(), &finalTable, options);
+
+ ASSERT_TRUE(merger.merge({}, tableA.get()));
+ ASSERT_TRUE(merger.mergeOverlay({}, tableB.get()));
+
+ Debug::printTable(&finalTable, {});
+
+ Styleable* styleable = test::getValue<Styleable>(&finalTable, "com.app.a:styleable/Foo");
+ ASSERT_NE(nullptr, styleable);
+
+ std::vector<Reference> expectedRefs = {
+ Reference(test::parseNameOrDie("com.app.a:attr/bar")),
+ Reference(test::parseNameOrDie("com.app.a:attr/bat")),
+ Reference(test::parseNameOrDie("com.app.a:attr/foo"), ResourceId(0x01010000)),
+ };
+
+ EXPECT_EQ(expectedRefs, styleable->entries);
+}
+
} // namespace aapt