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
Branch
Applications/18.12
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 7571
Build 7589: arc lint + arc unit
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.