[Adium-devl] Severe Structural AIChat-Issue in Adium

[Andreas Monitzer]

On Jul 26, 2007, at 22:59, Evan Schoenberg wrote:

> On Jul 19, 2007, at 12:12 PM, Andreas Monitzer wrote:
>
>> convLookupFromChat() sets up the necessary information for
>> lilbpurple, and calls serv_join_chat (this one doesn't return
>> anything). This is all that has to be done from Adium.
>
> It seems like here we have the first problem.  serv_join_chat()  
> appears not to be able to return the PurpleConversation, because it  
> may not exist for some time to come - at least some of the prpls do  
> asynchronous joins and create the conversation on success.  I think  
> that, for starters:
>  * serv_join_chat() needs some way of indicating an error.  This  
> could be a return value or an error-returned-by-reference.

The problem is, that it doesn't know yet if there's an error. Only  
the server knows, and it takes at least one read() call to get this  
reply.

>  * I say the latter because I think the return value should be a  
> handle which can be used to identify the PurpleConversation once it  
> is created.

That'd be another solution.

> There is already an integer id used when passing to  
> serv_got_joined_chat - it appears this is unique per prpl.  Perhaps  
> returning this and making use of this could be a good direction?

Perhaps.

>> Now, libpurple joins the chat, and calls adiumPurpleConvCreate() on
>> success. Note that libpurple doesn't call anything in Adium when it
>> doesn't succeed. Since Adium already created the AIChat object, this
>> is the cause for bug #1874 (Adium thinks that there's already a
>> groupchat with that MUC, so it disallows the join).
>
> I think having already created the AIChat object is fine - the  
> problem is that it's not destroyed on error.  As mentioned above,  
> serv_join_chat() should return an error if it fails immediately...  
> but if there's a failure some other way, we still need to find  
> out.  What do you think about a serv_failed_joined_chat() or  
> something like that which the prpl would call and which would, in  
> turn, let us know that the join attempt failed?

The problem is that the libpurple API is reasonable here, Adium is  
just utilizing it incorrectly. You will have a hard time explaining  
to them why changing the way it works is needed.
One argument would be that libpurple shouldn't assume a certain code  
design in the application, since the current design in Adium simply  
doesn't support this way of handling things.

> I'm a bit confused at this point - are you saying you've found that  
> 2 AIChat objects are created for every joined group chat?

Only on success. On failure, only the leaked one stays, the real  
instance doesn't get created.

> I think, if so, that's a 1.1/trunk regression, though I'm not  
> positive - and in that case I may have caused that while working on  
> fixing MSN group chats.

The corresponding ticket was created two years ago, so I don't think  
this is a new one.

> Does this case only occur if there has been a previous failure, or  
> does it happen every time?

It happens every time. Usually it's not a problem (except for the  
memory leak of course), but there are certain cases where the user  
notices it, as described in those tickets.

andy