Re: [evolution-patches] 63881 (dragging messages from vfolders)

[Not Zed <notzed ximian com>]

> ly prefer the way it was before.  Sure the inner loop might be
> > more complex, but this almost doubles the lines of code required so is
> > worse to maintain.
>
> OK, sure. I guess a good compiler will optimise it out anyway.
Its not a speed-critical section anyway :)

> > > @@ -226,24 +223,16 @@ vee_folder_construct (CamelVeeFolder *vf
> > >  	/* should CamelVeeMessageInfo be subclassable ..? */
> > >  	folder->summary = camel_folder_summary_new();
> > >  	folder->summary->message_info_size = sizeof(CamelVeeMessageInfo);
> > > +
> > > +	if (CAMEL_IS_VEE_STORE (parent_store)) {
> > > +		vf->folder_unmatched = ((CamelVeeStore *) parent_store)->folder_unmatched;
> > > +		vf->unmatched_uids = ((CamelVeeStore *) parent_store)->unmatched_uids;
> > > +	}
> > 
> > No don't copy these values onto the vee folder, it should only
> > reference the ones on its parent store, directly.  It could probably
> > have a bit though to say whether the parent is a veestore/and if it
> > should update it.  There's a comment near the end on how to get around
> > the pain of a double-dereference everywhere.
>
> OK. What I've done in this patch is to put a struct _CamelVeeStore
> *parent_vee_store on CamelVeeFolder, which gets set if the ((CamelFolder
> *)vf)->parent_store is a CamelVeeStore. 
>
> Mainly I just wanted to avoid calling camel_object_is() more than
> necessary, which means adding /something/ to CamelVeeFolder to store
> that information.

Camel object_is is very fast, much faster than gobject class checking (usually 3-4 single-line function calls).  But i guess this will do.  I suggest you add a comment saying why we have a duplicate of the parent store on the folder.

> I've also done what you said and made folder_unmatched and
> unmatched_uids local variables. It does cut down on the patch size, more
> so if you use diff -bB (--ignore-blank-lines --ignore-space-change).
Nod.

> > >  void
> > >  camel_vee_folder_construct(CamelVeeFolder *vf, CamelStore *parent_store, const char *name, guint32 flags)
> > >  {
> > > -	UNMATCHED_LOCK();
> > > -	
> > > -	/* setup unmatched folder if we haven't yet */
> > > -	if (folder_unmatched == NULL) {
> > > -		unmatched_uids = g_hash_table_new (g_str_hash, g_str_equal);
> > > -		folder_unmatched = (CamelVeeFolder *)camel_object_new (camel_vee_folder_get_type ());
> > > -		d(printf("created foldeer unmatched %p\n", folder_unmatched));
> > > -		
> > > -		vee_folder_construct (folder_unmatched, parent_store, CAMEL_UNMATCHED_NAME, CAMEL_STORE_FOLDER_PRIVATE);
> > > -	}
> > > -	
> > > -	UNMATCHED_UNLOCK();
> > > -	
> > >  	vee_folder_construct (vf, parent_store, name, flags);
> > 
> > vee_folder_construct should probably be renamed
> > camel_vee_folder_construct and this one above deleted.
>
> Done.
>
> Also: getting and releasing locks in the right order, added
> folder_unmatched and unmatched_uids to struct _update_data, removed some
> redundant checks, used forward references (struct _CamelVeeFolder *).
Almost there, I noticed in vee_folder_remove_folder it doesn't lock the vf.summary_lock before folder_unmatched.summary_lock anymore, this should still be in the same order as it was before.

You also have to free the uid's in the unmatched_uid's table when you finalise it.

Thats about all I can see right now, and it certainly appears to work as expected.  Can you include a changelog with the next patch please?

I have a little concern with such a big patch going into 2.8, but its the right fix, and its fairly easy to see if it changes the logic.

--

Michael Zucchi <notzed ximian com> "born to die, live to work, it's all downhill from here" Novell's Evolution and Free Software Developer