[Kde-pim] Review Request 109208: Work harder to ensure we have a trash folder

[Kevin Krammer]

> On Feb. 28, 2013, 3:54 p.m., Kevin Krammer wrote:
> > I am not sure this is fixing the problem at the right location. The question is why does SpecialCollections::hasDefaultCollection()return true if there is no valid collection for the queried type
> 
> Wolfgang Rohdewald wrote:
>     it has an invalid default collection (negative id, no remoteId), and the function is not "hasValidDefaultCollection". But I looked closer and the patch is now simpler. SpecialCollections::defaultCollection() always returns a collection - if there is none, it returns an invalid collection (with id -1). And all collections with negative id return false for isValid().
> 
> Kevin Krammer wrote:
>     Well, an invalid collection is not something one would consider as a collection if you explicitly call a check function.
>     Or at least I don't see any use case of first checking if there is a collection and then not getting one.
> 
> Wolfgang Rohdewald wrote:
>     invalid collection != no collection
>     
>     I do not know enough about akonadi internals but I believe those invalid collections are sort of placeholders until the database actually gives out a unique id. Have a look at the Collection constructor - passing out negative ids (meaning invalid collection) seems to be a normal use case. Changing hasDefaultCollection() (and hasCollection() to be consistent) to return only true for a valid collection is something I would not want to do - there may be places depending on current behaviour. And the API doc does not say that only valid collections should be found.
>     
>     Collection::Collection() :
>         Entity( new CollectionPrivate )
>     {
>       Q_D( Collection );
>       static int lastId = -1;
>       d->mId = lastId--;
>     }
>     
>     grepping for 'Collection()' finds a lot of places where such invalid collections are generated.
> 
> Kevin Krammer wrote:
>     A collection instance without valid id is just a locally created object of class Akonadi::Collection, it is not a handle to an actual collection yet.
>     
>     Getting such a default constructed object after checking for availability is like getting a default constructed value from a hash after checking with contains.
>     
>     In the same sense calling "hasDefaultCollection" and the not getting any is definitely not a good behavior. A hasDefaultCollection() that does not actually check anything is useless
> 
> Wolfgang Rohdewald wrote:
>     If you want to, I can change hasCollection() (which is called by hasDefaultCollection) to only return true for a valid collection.
>     
>     Looking at the source, I a very unsure if such a change of behaviour might not cause surprises.
>     
>     So IMHO this would not be a bug fix anymore, and I would not dare to apply it to KDE/4.10, only to master.
>     
>     So maybe my proposal for 4.10 and your proposal for master?

I looked into this a bit more. hasCollection() just checks if there is a collection for the given resource/type combination. So if it current returns true, it means that the underlying data structure, mFoldersForResource, got corrupted somehow, i.e. it should never contain an invalid collection.

My guess is that this happens in SpecialCollectionsPrivate::collectionFetchJobFinished(), where the code uses the collection name instead of the special collection type (which is used everywhere else) to identify the entry in the resource's sub hash.
But that would need to be verified.

Assuming you have some way of testing this problem, can you check where the invalid entry comes from?


- Kevin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/109208/#review28269
-----------------------------------------------------------


On Feb. 28, 2013, 4:45 p.m., Wolfgang Rohdewald wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109208/
> -----------------------------------------------------------
> 
> (Updated Feb. 28, 2013, 4:45 p.m.)
> 
> 
> Review request for KDEPIM.
> 
> 
> Description
> -------
> 
> collection.isValid() is not enough, collection might have a negative id and no remoteId
> 
> 
> This addresses bugs 285532, 303117 and 307016.
>     http://bugs.kde.org/show_bug.cgi?id=285532
>     http://bugs.kde.org/show_bug.cgi?id=303117
>     http://bugs.kde.org/show_bug.cgi?id=307016
> 
> 
> Diffs
> -----
> 
>   akonadi/specialcollectionsrequestjob.cpp 29db72f0e3bc60b0f8b9c9434151176bea44b14f 
> 
> Diff: http://git.reviewboard.kde.org/r/109208/diff/
> 
> 
> Testing
> -------
> 
> removed all .local/share/akonadi* and restarted kmail. Without patch, no trash. With patch, trash is generated.
> 
> 
> Thanks,
> 
> Wolfgang Rohdewald
> 
>

_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/