[Adium-devl] More design issues, and what I'm working on

[David Smith]

	I'm sure most of those following commits have now seen my per-group  
visibility branch. It basically works, but has exposed two (or three,  
depending on how you count) difficult issues. Colin and I were  
discussing this yesterday but didn't come to all that many  
conclusions. The following is something of a braindump + request for  
ideas. On the upside though, Zac and I did figure out a pretty nice  
way of dealing with the previous issue I emailed the list about, which  
is now implemented in my branch.

	The first issue is propagating changes from the model (AIList* in  
this case) to the rest of Adium. We use ESObjectWithProperties +  
AIContactObserverManager for this currently. Prior to my recent  
changes, contained contacts would also notify their parent when they  
became visible or invisible. The logging enabled by http://trac.adiumx.com/changeset/25568 
  has turned up the expected result, which is that in certain  
circumstances (particularly when accounts sign off) we aren't re- 
sorting groups in response. This leads to visible and invisible  
contacts being mixed, which breaks an AIListGroup invariant (all  
visible contacts must be sorted first). I suspect the 'stale contacts'  
bugs that have been reported are also symptoms of this.
	We could attack this problem piecemeal and simply identify all the  
places we should be notifying and aren't, and fix them, but the whole  
system feels rather fragile. The bugs caused by not notifying properly  
are subtle and hard to diagnose, and can have very bad consequences.  
ESObjectWithProperties itself makes debugging harder (can't easily  
inspect object state in the debugger), is a performance issue with  
frequent property access (~10% of visibility calculation is spent  
accessing properties), and doesn't support keypaths; however, its  
delayed notification system is pretty essential.
	So, what to do? Bringing back direct containing object notification  
of visibility is difficult or impossible in a per-group visibility  
world. We could have groups watch all the properties that can change  
visibility of their contained objects, I suppose, and invalidate based  
on that. I've made a list of what those properties are, as far as I  
can tell:

online
idle
mobile
blocked
displayName
UID
containedObjects.visibility //when the number of visible objects in a  
container changes from 0 to 1 or vice versa, the container's  
visibility changes.

	The second issue is performance. By eliminating a lot of the caching  
we were doing, I've hurt contact list performance rather a lot. The  
new offline group design in my branch is even worse about this. My  
bad. My initial take on this is that it's actually a symptom of the  
first issue: the reason I eliminated caching in the first place was in  
an attempt to keep our state consistent and avoid breaking  
AIListGroup's invariants. This didn't work. A secondary benefit of it  
was that it allowed moving to per-group visibility, which is a  
requirement for contacts in multiple groups and making the offline  
group less complicated. I suspect that if we reintroduced visibility  
caching at the container level (an array or set of visible contacts)  
rather than the listobject level (a flag in AIListObject), and fixed  
the notification issues, this issue would go away.

	The third issue is a good deal more specific and maybe not a problem:  
we don't have a good way of getting a list of "top level" contacts.  
AIContactController has accessors for -allContacts (iterates the  
contacts dictionary and puts it in an array), -allMetaContacts (ditto  
for metas), and -allGroups. Unfortunately -allContacts includes ones  
that are contained in metas, so to get what I'm looking for I  
basically need to get allContacts, then subtract the contents of  
allMetaContacts from it. This means we can't effectively KVO "all  
contacts", and attempting to make the offline group into a filter on  
the contact list involves a bunch of iteration and adding and removing  
things from arrays, which hurts performance a lot.This sounds like  
maybe a good place to use the 'all contacts should be in metas' idea  
that's come up several times over the last year? Maybe it would be  
less of an issue with caching reinstated though. I'm not sure.