From b886ed941b116aa072282e52d2b62a04a2a254b5 Mon Sep 17 00:00:00 2001 From: David Faure Date: Sun, 24 Mar 2019 00:38:18 +0100 Subject: [PATCH] Make attribute getters truly const (and non-const) Also cleans up the code a little. This change requires const fixes basically everywhere where the API is used, please do NOT merge this until everything else has been adjusted. Differential Revision: https://phabricator.kde.org/D19942 --- src/core/attributestorage.cpp | 8 ++++- src/core/attributestorage_p.h | 3 +- src/core/collection.cpp | 14 +++++--- src/core/collection.h | 50 +++++++++++++---------------- src/core/collectionsync.cpp | 2 +- src/core/item.cpp | 13 ++++---- src/core/item.h | 53 +++++++++++++++---------------- src/core/itemchangelog.cpp | 7 +++- src/core/itemchangelog_p.h | 3 +- src/core/models/entitytreemodel.cpp | 4 +-- src/core/models/statisticsproxymodel.cpp | 2 +- src/core/models/tagmodel.cpp | 3 +- src/core/tag.cpp | 10 ++++-- src/core/tag.h | 29 ++++++++--------- src/widgets/collectionmaintenancepage.cpp | 2 +- src/xml/autotests/collectiontest.cpp | 2 +- 16 files changed, 110 insertions(+), 95 deletions(-) diff --git a/src/core/attributestorage.cpp b/src/core/attributestorage.cpp index 0d6793f9f..f6af86edd 100644 --- a/src/core/attributestorage.cpp +++ b/src/core/attributestorage.cpp @@ -97,11 +97,17 @@ void AttributeStorage::clearAttributes() mModifiedAttributes.clear(); } -Attribute *AttributeStorage::attribute(const QByteArray &type) const +const Attribute *AttributeStorage::attribute(const QByteArray &type) const { return mAttributes.value(type); } +Attribute *AttributeStorage::attribute(const QByteArray &type) +{ + markAttributeModified(type); + return mAttributes.value(type); +} + void AttributeStorage::markAttributeModified(const QByteArray &type) { mDeletedAttributes.remove(type); diff --git a/src/core/attributestorage_p.h b/src/core/attributestorage_p.h index e38f255f7..47ac096cc 100644 --- a/src/core/attributestorage_p.h +++ b/src/core/attributestorage_p.h @@ -48,7 +48,8 @@ public: bool hasAttribute(const QByteArray &type) const; Attribute::List attributes() const; void clearAttributes(); - Attribute *attribute(const QByteArray &type) const; + const Attribute *attribute(const QByteArray &type) const; + Attribute *attribute(const QByteArray &type); void markAttributeModified(const QByteArray &type); void resetChangeLog(); diff --git a/src/core/collection.cpp b/src/core/collection.cpp index 9fe833913..c94ea291a 100644 --- a/src/core/collection.cpp +++ b/src/core/collection.cpp @@ -189,7 +189,13 @@ void Akonadi::Collection::clearAttributes() return d_ptr->mAttributeStorage.clearAttributes(); } -Attribute *Collection::attribute(const QByteArray &type) const +Attribute *Collection::attribute(const QByteArray &type) +{ + markAttributeModified(type); + return d_ptr->mAttributeStorage.attribute(type); +} + +const Attribute *Collection::attribute(const QByteArray &type) const { return d_ptr->mAttributeStorage.attribute(type); } @@ -236,8 +242,7 @@ void Collection::setName(const QString &name) Collection::Rights Collection::rights() const { - CollectionRightsAttribute *attr = attribute(); - if (attr) { + if (const auto attr = attribute()) { return attr->rights(); } else { return AllRights; @@ -246,8 +251,7 @@ Collection::Rights Collection::rights() const void Collection::setRights(Rights rights) { - CollectionRightsAttribute *attr = attribute(AddIfMissing); - attr->setRights(rights); + attribute(AddIfMissing)->setRights(rights); } QStringList Collection::contentMimeTypes() const diff --git a/src/core/collection.h b/src/core/collection.h index 8a51eb0eb..f886ed083 100644 --- a/src/core/collection.h +++ b/src/core/collection.h @@ -253,6 +253,10 @@ public: /** * Returns a list of all attributes of the collection. + * + * @warning Do not modify the attributes returned from this method, + * the change will not be reflected when updating the Collection + * through CollectionModifyJob. */ Q_REQUIRED_RESULT Attribute::List attributes() const; @@ -264,30 +268,32 @@ public: /** * Returns the attribute of the given type @p name if available, 0 otherwise. */ - Attribute *attribute(const QByteArray &name) const; + Attribute *attribute(const QByteArray &name); + const Attribute *attribute(const QByteArray &name) const; /** * Describes the options that can be passed to access attributes. */ enum CreateOption { - AddIfMissing ///< Creates the attribute if it is missing + AddIfMissing, ///< Creates the attribute if it is missing + DontCreate ///< Default value }; /** * Returns the attribute of the requested type. - * If the collection has no attribute of that type yet, a new one - * is created and added to the entity. + * If the collection has no attribute of that type yet, passing AddIfMissing + * as an argument will create and add it to the entity * * @param option The create options. */ template - inline T *attribute(CreateOption option); + inline T *attribute(CreateOption option = DontCreate); /** * Returns the attribute of the requested type or 0 if it is not available. */ template - inline T *attribute() const; + inline const T *attribute() const; /** * Removes and deletes the attribute of the requested type. @@ -562,40 +568,30 @@ AKONADICORE_EXPORT uint qHash(const Akonadi::Collection &collection); template inline T *Akonadi::Collection::attribute(Collection::CreateOption option) { - Q_UNUSED(option); - const QByteArray type = T().type(); - markAttributeModified(type); // do this first in case it detaches if (hasAttribute(type)) { - T *attr = dynamic_cast(attribute(type)); - if (attr) { + if (T *attr = dynamic_cast(attribute(type))) { return attr; } - //Reuse 5250 qWarning() << "Found attribute of unknown type" << type << ". Did you forget to call AttributeFactory::registerAttribute()?"; + } else if (option == AddIfMissing) { + T *attr = new T(); + addAttribute(attr); + return attr; } - T *attr = new T(); - addAttribute(attr); - return attr; + return nullptr; } template -inline T *Akonadi::Collection::attribute() const +inline const T *Akonadi::Collection::attribute() const { const QByteArray type = T().type(); if (hasAttribute(type)) { - T *attr = dynamic_cast(attribute(type)); - if (attr) { - // FIXME: This method returns a non-const pointer, so callers may still modify the - // attribute. Unfortunately, just making this function return a const pointer and - // creating a non-const overload does not work, as many users of this function abuse the - // non-const pointer and modify the attribute even on a const object. - const_cast(this)->markAttributeModified(type); + if (const T *attr = dynamic_cast(attribute(type))) { return attr; } - //reuse 5250 qWarning() << "Found attribute of unknown type" << type << ". Did you forget to call AttributeFactory::registerAttribute()?"; } @@ -606,15 +602,13 @@ inline T *Akonadi::Collection::attribute() const template inline void Akonadi::Collection::removeAttribute() { - const T dummy; - removeAttribute(dummy.type()); + removeAttribute(T().type()); } template inline bool Akonadi::Collection::hasAttribute() const { - const T dummy; - return hasAttribute(dummy.type()); + return hasAttribute(T().type()); } } // namespace Akonadi diff --git a/src/core/collectionsync.cpp b/src/core/collectionsync.cpp index ffdfbf706..055f7d03c 100644 --- a/src/core/collectionsync.cpp +++ b/src/core/collectionsync.cpp @@ -573,7 +573,7 @@ public: Q_FOREACH (Attribute *remoteAttr, upd.attributes()) { if (ignoreAttributeChanges(remote, remoteAttr->type()) && local.hasAttribute(remoteAttr->type())) { //We don't want to overwrite the attribute changes with the defaults provided by the resource. - Attribute *localAttr = local.attribute(remoteAttr->type()); + const Attribute *localAttr = local.attribute(remoteAttr->type()); upd.removeAttribute(localAttr->type()); upd.addAttribute(localAttr->clone()); } diff --git a/src/core/item.cpp b/src/core/item.cpp index be6a3c32c..7f676da6e 100644 --- a/src/core/item.cpp +++ b/src/core/item.cpp @@ -252,13 +252,14 @@ void Akonadi::Item::clearAttributes() ItemChangeLog::instance()->attributeStorage(d_ptr).clearAttributes(); } -Attribute *Item::attribute(const QByteArray &type) const +Attribute *Item::attribute(const QByteArray &type) { - // FIXME: Createa a truly const and non-const overloads of this method - // so only non-const access marks the attribute as modified - auto &storage = ItemChangeLog::instance()->attributeStorage(d_ptr); - storage.markAttributeModified(type); - return storage.attribute(type); + return ItemChangeLog::instance()->attributeStorage(d_ptr).attribute(type); +} + +const Attribute *Item::attribute(const QByteArray &type) const +{ + return ItemChangeLog::instance()->attributeStorage(d_ptr).attribute(type); } Collection &Item::parentCollection() diff --git a/src/core/item.h b/src/core/item.h index 7999af3fe..d347e3ad2 100644 --- a/src/core/item.h +++ b/src/core/item.h @@ -293,6 +293,10 @@ public: /** * Returns a list of all attributes of the item. + * + * @warning Do not modify the attributes returned from this method, + * the change will not be reflected when updating the Item through + * ItemModifyJob. */ Attribute::List attributes() const; @@ -304,13 +308,15 @@ public: /** * Returns the attribute of the given type @p name if available, 0 otherwise. */ - Attribute *attribute(const QByteArray &name) const; + Attribute *attribute(const QByteArray &name); + const Attribute *attribute(const QByteArray &name) const; /** * Describes the options that can be passed to access attributes. */ enum CreateOption { - AddIfMissing ///< Creates the attribute if it is missing + AddIfMissing, ///< Creates the attribute if it is missing + DontCreate ///< Do not create the attribute if it is missing (default) }; /** @@ -321,13 +327,13 @@ public: * @param option The create options. */ template - inline T *attribute(CreateOption option); + inline T *attribute(CreateOption option = DontCreate); /** * Returns the attribute of the requested type or 0 if it is not available. */ template - inline T *attribute() const; + inline const T *attribute() const; /** * Removes and deletes the attribute of the requested type. @@ -787,36 +793,31 @@ AKONADICORE_EXPORT uint qHash(const Akonadi::Item &item); template inline T *Item::attribute(Item::CreateOption option) { - Q_UNUSED(option); - - const T dummy; - if (hasAttribute(dummy.type())) { - T *attr = dynamic_cast(attribute(dummy.type())); - if (attr) { + const QByteArray type = T().type(); + if (hasAttribute(type)) { + if (T *attr = dynamic_cast(attribute(type))) { return attr; } - //Reuse 5250 - qWarning() << "Found attribute of unknown type" << dummy.type() + qWarning() << "Found attribute of unknown type" << type << ". Did you forget to call AttributeFactory::registerAttribute()?"; + } else if (option == AddIfMissing) { + T *attr = new T(); + addAttribute(attr); + return attr; } - T *attr = new T(); - addAttribute(attr); - return attr; + return nullptr; } template -inline T *Item::attribute() const +inline const T *Item::attribute() const { - const T dummy; - if (hasAttribute(dummy.type())) { - T *attr = dynamic_cast(attribute(dummy.type())); - if (attr) { - + const QByteArray type = T().type(); + if (hasAttribute(type)) { + if (const T *attr = dynamic_cast(attribute(type))) { return attr; } - //reuse 5250 - qWarning() << "Found attribute of unknown type" << dummy.type() + qWarning() << "Found attribute of unknown type" << type << ". Did you forget to call AttributeFactory::registerAttribute()?"; } @@ -826,15 +827,13 @@ inline T *Item::attribute() const template inline void Item::removeAttribute() { - const T dummy; - removeAttribute(dummy.type()); + removeAttribute(T().type()); } template inline bool Item::hasAttribute() const { - const T dummy; - return hasAttribute(dummy.type()); + return hasAttribute(T().type()); } template diff --git a/src/core/itemchangelog.cpp b/src/core/itemchangelog.cpp index a3e300287..5059b6aec 100644 --- a/src/core/itemchangelog.cpp +++ b/src/core/itemchangelog.cpp @@ -57,7 +57,12 @@ Tag::List &ItemChangeLog::deletedTags(const ItemPrivate *priv) return m_deletedTags[const_cast(priv)]; } -AttributeStorage &ItemChangeLog::attributeStorage(const ItemPrivate *priv) +AttributeStorage &ItemChangeLog::attributeStorage(ItemPrivate *priv) +{ + return m_attributeStorage[priv]; +} + +const AttributeStorage &ItemChangeLog::attributeStorage(const ItemPrivate *priv) { return m_attributeStorage[const_cast(priv)]; } diff --git a/src/core/itemchangelog_p.h b/src/core/itemchangelog_p.h index 78c6c2a3a..34f201c0c 100644 --- a/src/core/itemchangelog_p.h +++ b/src/core/itemchangelog_p.h @@ -41,7 +41,8 @@ public: Tag::List &addedTags(const ItemPrivate *priv); Tag::List &deletedTags(const ItemPrivate *priv); - AttributeStorage &attributeStorage(const ItemPrivate *priv); + const AttributeStorage &attributeStorage(const ItemPrivate *priv); + AttributeStorage &attributeStorage(ItemPrivate *priv); void removeItem(const ItemPrivate *priv); void clearItemChangelog(const ItemPrivate *priv); diff --git a/src/core/models/entitytreemodel.cpp b/src/core/models/entitytreemodel.cpp index 606e2d330..da7f459e5 100644 --- a/src/core/models/entitytreemodel.cpp +++ b/src/core/models/entitytreemodel.cpp @@ -317,7 +317,7 @@ QVariant EntityTreeModel::data(const QModelIndex &index, int role) const return d->m_pendingCutCollections.contains(node->id); case Qt::BackgroundRole: { if (collection.hasAttribute()) { - EntityDisplayAttribute *eda = collection.attribute(); + const EntityDisplayAttribute *eda = collection.attribute(); QColor color = eda->backgroundColor(); if (color.isValid()) { return color; @@ -368,7 +368,7 @@ QVariant EntityTreeModel::data(const QModelIndex &index, int role) const return d->m_pendingCutItems.contains(node->id); case Qt::BackgroundRole: { if (item.hasAttribute()) { - EntityDisplayAttribute *eda = item.attribute(); + const EntityDisplayAttribute *eda = item.attribute(); const QColor color = eda->backgroundColor(); if (color.isValid()) { return color; diff --git a/src/core/models/statisticsproxymodel.cpp b/src/core/models/statisticsproxymodel.cpp index d5ff92891..8feda6bbb 100644 --- a/src/core/models/statisticsproxymodel.cpp +++ b/src/core/models/statisticsproxymodel.cpp @@ -103,7 +103,7 @@ public: .arg(i18n("Unread Messages")).arg(collection.statistics().unreadCount()); if (collection.hasAttribute()) { - CollectionQuotaAttribute *quota = collection.attribute(); + const CollectionQuotaAttribute *quota = collection.attribute(); if (quota->currentValue() > -1 && quota->maximumValue() > 0) { qreal percentage = (100.0 * quota->currentValue()) / quota->maximumValue(); diff --git a/src/core/models/tagmodel.cpp b/src/core/models/tagmodel.cpp index 14028a1f6..96d529431 100644 --- a/src/core/models/tagmodel.cpp +++ b/src/core/models/tagmodel.cpp @@ -109,8 +109,7 @@ QVariant TagModel::data(const QModelIndex &index, int role) const case TagRole: return QVariant::fromValue(tag); case Qt::DecorationRole: { - TagAttribute *attr = tag.attribute(); - if (attr) { + if (const TagAttribute *attr = tag.attribute()) { return QIcon::fromTheme(attr->iconName()); } else { return QVariant(); diff --git a/src/core/tag.cpp b/src/core/tag.cpp index 131eadccf..563304c83 100644 --- a/src/core/tag.cpp +++ b/src/core/tag.cpp @@ -145,11 +145,17 @@ void Tag::clearAttributes() d_ptr->mAttributeStorage.clearAttributes(); } -Attribute *Tag::attribute(const QByteArray &type) const +const Attribute *Tag::attribute(const QByteArray &type) const { return d_ptr->mAttributeStorage.attribute(type); } +Attribute *Tag::attribute(const QByteArray &type) +{ + markAttributeModified(type); + return d_ptr->mAttributeStorage.attribute(type); +} + void Tag::setId(Tag::Id identifier) { d_ptr->id = identifier; @@ -244,7 +250,7 @@ Tag Tag::genericTag(const QString &name) return tag; } -bool Tag::checkAttribute(Attribute *attr, const QByteArray &type) const +bool Tag::checkAttribute(const Attribute *attr, const QByteArray &type) const { if (attr) { return true; diff --git a/src/core/tag.h b/src/core/tag.h index 20ca6c9ab..9f41c7301 100644 --- a/src/core/tag.h +++ b/src/core/tag.h @@ -116,13 +116,15 @@ public: /** * Returns the attribute of the given type @p name if available, 0 otherwise. */ - Attribute *attribute(const QByteArray &name) const; + const Attribute *attribute(const QByteArray &name) const; + Attribute *attribute(const QByteArray &name); /** * Describes the options that can be passed to access attributes. */ enum CreateOption { - AddIfMissing ///< Creates the attribute if it is missing + AddIfMissing, ///< Creates the attribute if it is missing + DontCreate ///< Does not create an attribute if it is missing (default) }; /** @@ -133,13 +135,13 @@ public: * @param option The create options. */ template - inline T *attribute(CreateOption option); + inline T *attribute(CreateOption option = DontCreate); /** * Returns the attribute of the requested type or 0 if it is not available. */ template - inline T *attribute() const; + inline const T *attribute() const; /** * Removes and deletes the attribute of the requested type. @@ -197,7 +199,7 @@ public: static Tag genericTag(const QString &name); private: - bool checkAttribute(Attribute *attr, const QByteArray &type) const; + bool checkAttribute(const Attribute *attr, const QByteArray &type) const; void markAttributeModified(const QByteArray &type); //@cond PRIVATE @@ -214,8 +216,6 @@ AKONADICORE_EXPORT uint qHash(const Akonadi::Tag &); template inline T *Tag::attribute(CreateOption option) { - Q_UNUSED(option); - const QByteArray type = T().type(); markAttributeModified(type); if (hasAttribute(type)) { @@ -223,23 +223,22 @@ inline T *Tag::attribute(CreateOption option) if (checkAttribute(attr, type)) { return attr; } + } else if (option == AddIfMissing) { + T *attr = new T(); + addAttribute(attr); + return attr; } - T *attr = new T(); - addAttribute(attr); - return attr; + return nullptr; } template -inline T *Tag::attribute() const +inline const T *Tag::attribute() const { const QByteArray type = T().type(); if (hasAttribute(type)) { - T *attr = dynamic_cast(attribute(type)); + const T *attr = dynamic_cast(attribute(type)); if (checkAttribute(attr, type)) { - // FIXME: Make this a truly const method so that callers may not modify - // the attribute returned from here. - const_cast(this)->markAttributeModified(type); return attr; } } diff --git a/src/widgets/collectionmaintenancepage.cpp b/src/widgets/collectionmaintenancepage.cpp index 47a0a22f9..ef96c77f8 100644 --- a/src/widgets/collectionmaintenancepage.cpp +++ b/src/widgets/collectionmaintenancepage.cpp @@ -129,7 +129,7 @@ void CollectionMaintenancePage::load(const Collection &col) init(col); if (col.isValid()) { d->updateLabel(col.statistics().count(), col.statistics().unreadCount(), col.statistics().size()); - Akonadi::IndexPolicyAttribute *attr = col.attribute(); + const Akonadi::IndexPolicyAttribute *attr = col.attribute(); const bool indexingWasEnabled(!attr || attr->indexingEnabled()); d->ui.enableIndexingChkBox->setChecked(indexingWasEnabled); if (indexingWasEnabled) { diff --git a/src/xml/autotests/collectiontest.cpp b/src/xml/autotests/collectiontest.cpp index 9f181710b..e7006c0da 100644 --- a/src/xml/autotests/collectiontest.cpp +++ b/src/xml/autotests/collectiontest.cpp @@ -76,7 +76,7 @@ void CollectionTest::testBuildCollection() verifyCollection(colist, 2, QStringLiteral("c112"), QStringLiteral("Akonadi"), mimeType); QVERIFY(colist.at(0).hasAttribute()); - EntityDisplayAttribute *attr = colist.at(0).attribute(); + const EntityDisplayAttribute *attr = colist.at(0).attribute(); QCOMPARE(attr->displayName(), QStringLiteral("Posteingang")); } -- 2.16.4