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

[Edward Catmur <ed catmur co uk>]

Sorry, got eaten by my MX. Sending again.

On Tue, 2004-09-14 at 23:43 +0100, Edward Catmur wrote:
> Thanks for reviewing the patch. I've attached the latest version.
> 
> > > +	if (vf == vf->folder_unmatched)
> > > +		while (node) {
> > > +			CamelFolder *f = node->data;
> > > +	
> > > +			camel_object_unref((CamelObject *)f);
> > >  
> > > -		if (vf != folder_unmatched) {
> > > +			node = g_list_next(node);
> > > +		}
> > > +	else
> > > +		while (node) {
> > > +			CamelFolder *f = node->data;
> > > +	
> > >  			camel_object_unhook_event((CamelObject *)f, "folder_changed", (CamelObjectEventHookFunc) folder_changed, vf);
> > >  			camel_object_unhook_event((CamelObject *)f, "deleted", (CamelObjectEventHookFunc) subfolder_deleted, vf);
> > >  			camel_object_unhook_event((CamelObject *)f, "renamed", (CamelObjectEventHookFunc) subfolder_renamed, vf);
> > >  			/* this updates the vfolder */
> > >  			if ((vf->flags & CAMEL_STORE_FOLDER_PRIVATE) == 0)
> > >  				vee_folder_remove_folder(vf, f, FALSE);
> > > -		}
> > > -		camel_object_unref((CamelObject *)f);
> > > +			camel_object_unref((CamelObject *)f);
> > >  
> > > -		node = g_list_next(node);
> > > -	}
> > > +			node = g_list_next(node);
> > > +		}
> > 
> > I actually 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.
> 
> > > @@ -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.
> 
> 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).
> 
> > >  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 *).
> 
> Thanks again.
> 
> Ed