Commit d5ccee37 authored by David Faure's avatar David Faure
Browse files

Fix tag name/type/gid missing in notifications about tags.

Summary:
The logic in AggregatedFetchScope was wrong, it led to fetchIdOnly()
being always true.

The mechanism in Aggregated* is all based on default_value || A || B || C
where A, B, C are the boolean request from every subscriber.
This works well for fetchRemoteId(), where the default_value is false.

For fetchIdOnly(), what we really want is "(A && B && C)" ("everyone
wants fetchIdOnly) which was implemented as "!(A || B || C)"
(the '!' in front is the == 0 instead of > 0 in Aggregated*FetchScope::fetchIdOnly()).

The flaw in this logic is the default value. With no subscribers,
we want fetchIdOnly() == false, while it currently returns true.

A default_value of true in the default_value || A || B || C logic,
can't work: the result was always true.

The solution I implemented is to compare the number of subscribers that
called setFetchIdOnly(true) with the total number of subscribers.
This directly implements A && B && C, with a default value of false.

Test Plan: 1) extended tagstest. 2) this fixes the remaining failures in zanshin's akonadi tests.

Reviewers: ervin, dvratil

Reviewed By: dvratil

Subscribers: kde-pim

Tags: #kde_pim

Differential Revision: https://phabricator.kde.org/D18556

(cherry picked from commit 25087bb6,
sorry about that, arc land messed up once more)
parent 4c1b1269
......@@ -756,10 +756,17 @@ void TagTest::testMonitor()
TagCreateJob *createjob = new TagCreateJob(tag, this);
AKVERIFYEXEC(createjob);
createdTag = createjob->tag();
QCOMPARE(createdTag.type(), tag.type());
QCOMPARE(createdTag.name(), tag.name());
QCOMPARE(createdTag.gid(), tag.gid());
//We usually pick up signals from the previous tests as well (due to server-side notification caching)
QTRY_VERIFY(addedSpy.count() >= 1);
QTRY_COMPARE(addedSpy.last().first().value<Akonadi::Tag>().id(), createdTag.id());
QVERIFY(addedSpy.last().first().value<Akonadi::Tag>().hasAttribute<Akonadi::TagAttribute>());
const Akonadi::Tag notifiedTag = addedSpy.last().first().value<Akonadi::Tag>();
QCOMPARE(notifiedTag.type(), createdTag.type());
QCOMPARE(notifiedTag.gid(), createdTag.gid());
QVERIFY(notifiedTag.hasAttribute<Akonadi::TagAttribute>());
QCOMPARE(notifiedTag.name(), createdTag.name()); // requires the TagAttribute
}
{
......
......@@ -89,6 +89,7 @@ class AggregatedCollectionFetchScopePrivate : public AggregatedFetchScopePrivate
public:
QSet<QByteArray> attrs;
QHash<QByteArray, int> attrsCount;
int subscribers = 0;
int fetchIdOnly = 0;
int fetchStats = 0;
};
......@@ -99,6 +100,7 @@ class AggregatedTagFetchScopePrivate : public AggregatedFetchScopePrivate
public:
QSet<QByteArray> attrs;
QHash<QByteArray, int> attrsCount;
int subscribers = 0;
int fetchIdOnly = 0;
int fetchRemoteId = 0;
int fetchAllAttributes = 0;
......@@ -186,7 +188,7 @@ bool AggregatedCollectionFetchScope::fetchIdOnly() const
LOCKED_D(const AggregatedCollectionFetchScope)
// Aggregation: we can return true only if everyone wants fetchIdOnly,
// otherwise there's at least one subscriber who wants everything
return d->fetchIdOnly == 0;
return d->fetchIdOnly == d->subscribers;
}
void AggregatedCollectionFetchScope::setFetchIdOnly(bool fetchIdOnly)
......@@ -208,6 +210,17 @@ void AggregatedCollectionFetchScope::setFetchStatistics(bool fetchStats)
d->updateBool(fetchStats, d->fetchStats);
}
void AggregatedCollectionFetchScope::addSubscriber()
{
LOCKED_D(AggregatedCollectionFetchScope)
++d->subscribers;
}
void AggregatedCollectionFetchScope::removeSubscriber()
{
LOCKED_D(AggregatedCollectionFetchScope)
--d->subscribers;
}
AggregatedItemFetchScope::AggregatedItemFetchScope()
......@@ -567,7 +580,7 @@ bool AggregatedTagFetchScope::fetchIdOnly() const
LOCKED_D(const AggregatedTagFetchScope)
// Aggregation: we can return true only if everyone wants fetchIdOnly,
// otherwise there's at least one subscriber who wants everything
return d->fetchIdOnly == 0;
return d->fetchIdOnly == d->subscribers;
}
void AggregatedTagFetchScope::setFetchIdOnly(bool fetchIdOnly)
......@@ -618,4 +631,16 @@ void AggregatedTagFetchScope::removeAttribute(const QByteArray &attribute)
d->removeFromSet(attribute, d->attrs, d->attrsCount);
}
void AggregatedTagFetchScope::addSubscriber()
{
LOCKED_D(AggregatedTagFetchScope)
++d->subscribers;
}
void AggregatedTagFetchScope::removeSubscriber()
{
LOCKED_D(AggregatedTagFetchScope)
--d->subscribers;
}
#undef LOCKED_D
......@@ -49,6 +49,9 @@ public:
bool fetchStatistics() const;
void setFetchStatistics(bool fetchStats);
void addSubscriber();
void removeSubscriber();
private:
AggregatedCollectionFetchScopePrivate * const d_ptr;
Q_DECLARE_PRIVATE(AggregatedCollectionFetchScope)
......@@ -121,6 +124,9 @@ public:
void addAttribute(const QByteArray &attribute);
void removeAttribute(const QByteArray &attribute);
void addSubscriber();
void removeSubscriber();
bool fetchIdOnly() const;
void setFetchIdOnly(bool fetchIdOnly);
......
......@@ -48,6 +48,10 @@ NotificationSubscriber::NotificationSubscriber(NotificationManager *manager)
, mExclusive(false)
, mNotificationDebugging(false)
{
if (mManager) {
mManager->collectionFetchScope()->addSubscriber();
mManager->tagFetchScope()->addSubscriber();
}
}
NotificationSubscriber::NotificationSubscriber(NotificationManager *manager, quintptr socketDescriptor)
......@@ -163,6 +167,10 @@ void NotificationSubscriber::disconnectSubscriber()
for (const auto &attr : attrs) {
cfs->removeAttribute(attr);
}
cfs->removeSubscriber();
auto tfs = mManager->tagFetchScope();
tfs->removeSubscriber();
mManager->forgetSubscriber(this);
deleteLater();
......@@ -268,6 +276,8 @@ void NotificationSubscriber::modifySubscription(const Protocol::ModifySubscripti
const auto newScope = command.tagFetchScope();
mManager->tagFetchScope()->apply(mTagFetchScope, newScope);
mTagFetchScope = newScope;
if (!newScope.fetchIdOnly())
Q_ASSERT(!mManager->tagFetchScope()->fetchIdOnly());
}
if (mManager) {
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment