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

Authored by dfaure on Jan 26 2019, 6:42 PM.

Details

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.

Diff Detail

Repository
R165 Akonadi
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
dfaure created this revision.Jan 26 2019, 6:42 PM
Restricted Application added a project: KDE PIM. · View Herald TranscriptJan 26 2019, 6:42 PM
dfaure requested review of this revision.Jan 26 2019, 6:42 PM
dfaure updated this revision to Diff 50350.Jan 26 2019, 8:57 PM

Fix crash when manager is null

dfaure retitled this revision from Minor: improve QDebug output for Akonadi::Tag to Fix tag name/type/gid missing in notifications about tags..Jan 26 2019, 8:58 PM
dfaure edited the summary of this revision. (Show Details)
dfaure updated this revision to Diff 50351.Jan 26 2019, 8:59 PM

this line is for the next commit, oops

dvratil accepted this revision.Jan 27 2019, 9:26 PM

Thanks for the patch, looks good.

This revision is now accepted and ready to land.Jan 27 2019, 9:26 PM
This revision was automatically updated to reflect the committed changes.

Thanks for the quick review. I found more instances of "counter ==0" : AggregatedItemFetchScope::cacheOnly and AggregatedItemFetchScope::ignoreErrors. It seems clear to me that this needs the same treatment (patch ready), but I'm not sure how to unittest that. Maybe I'll just write a unittest for AggregatedItemFetchScope itself, unless you have a better idea of what might currently be broken by these bools being always true.

Dedicated test for the AggregatedFetchScope is the easiest approach I think - it's a fairly isolated class, so it should be easy to write a test for it.