Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 5c3464c7 authored by Adam Lesinski's avatar Adam Lesinski
Browse files

AAPT2: Fix merging of styleables the right way 

Styleables should only be merged when processing overlays.
This moves the styleable merging code out of ResourceTable
and into TableMerger.

Change-Id: I3aae05cf4dd875cd25ac2ac744b61194409b2fee
parent e637f315

tools/aapt2/ResourceTable.cpp

+24 −31
Original line number Diff line number Diff line
@@ -201,8 +201,7 @@ std::vector<ResourceConfigValue*> ResourceEntry::findValuesIf( 
}



/**

 * 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 @@ std::vector<ResourceConfigValue*> ResourceEntry::findValuesIf( 
 *

 * 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 @@ int ResourceTable::resolveValueCollision(Value* existing, Value* incoming) { 
        // 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 @@ bool ResourceTable::addResourceImpl(const ResourceNameRef& name, 
                                    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 @@ bool ResourceTable::addResourceImpl(const ResourceNameRef& name, 
        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 << "'");

            diag->error(DiagMessage(configValue->value->getSource())

                                    << "resource previously defined here");

            return false;



        case CollisionResult::kKeepOriginal:

            break;

        }

    }

tools/aapt2/ResourceTable.h

+11 −6
Original line number Diff line number Diff line
@@ -38,8 +38,8 @@ namespace aapt { 


enum class SymbolState {

    kUndefined,

    kPrivate,

    kPublic,

    kPrivate

};



/**
@@ -185,13 +185,18 @@ class ResourceTable { 
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 @@ private: 
                         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,

tools/aapt2/ResourceTable_test.cpp

+0 −33
Original line number Diff line number Diff line
@@ -118,39 +118,6 @@ TEST(ResourceTableTest, OverrideWeakResourceValue) { 
    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;

tools/aapt2/ResourceValues.cpp

+14 −3
Original line number Diff line number Diff line
@@ -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 @@ void RawString::print(std::ostream* out) const { 
    *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 ResourceNameRef& n, Type t) : 
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 @@ bool operator!=(const Reference& a, const Reference& b) { 
    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();

tools/aapt2/ResourceValues.h

+1 −0
Original line number Diff line number Diff line
@@ -172,6 +172,7 @@ struct Reference : public BaseItem<Reference> { 
    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;