Revision f6a67901e79d8d35e6bf30f0766b2417740090b7

[Nathan Walp]

Andreas Monitzer wrote:
> First off, thank you for reviewing the code.
> 
> On Aug 28, 2007, at 03:28, Nathan Walp wrote:
> 
>> - I dislike implementing experimental JEPs in libpurple
> 
> The problem is that the XSF is very slow in moving XEPs to more stable 
> states, it usually takes a few years. I asked people in the know about 
> all experimental XEPs I considered implementing before doing so. All of 
> them are not expected to change much or getting thrown out again. That's 
> the reason I didn't implement User Profile.
> XEP-0115: Entity Capabilities has changed since I implemented it, the 
> implementation will need some modifications.

This is *exactly* why I don't like implementing them.  They do change 
out from under you.  I would suggest we either put the experimental ones 
behind a #ifdef, or keep them in a branch until they mature a bit more. 
  Most of the ones that have been sitting for eternity have been doing 
so because they depend on PEP, which took its dear sweet time.  Now that 
PEP is wrapping up, I think we should start to see these trickle to 
Draft.  When they're Draft, and "stable" I'm usually more comfortable 
unleashing them upon the masses.

> 
>> - forces buddy icons to PNGs, which will kill animated buddy icons
> 
> XEP-0084 says that PNG is required. It's possible to specify multiple 
> alternatives (at least one of them has to be PNG), but libpurple's API 
> doesn't allow for that, so I had to force it to PNG only.
> 
>> - the prpl now knows way too much about PNGs.  That functionality should
>> be abstracted out to somewhere else, core or otherwise.
> 
> Yes, knowing the image size would be great. I hope you understand that I 
> hesitated rewriting the core API for my project.

This is another one of those things that should have been discussed on 
this list, so a better solution could have come about.

Unfortunately, people get really cranky when their buddy icons don't 
animate.  If we have the proper abilities in the core, then we should be 
able to take whatever we get as a buddy icon, get both its mime type and 
size, and if the mime type isn't image/png, convert it to one so we have 
both, and post both in the PEP portion, and the original (with correct 
mime type) in the vCard (for the next 6 months to a year, then I'd like 
to see that code go the way of the do do, if the PEP implementations 
have "caught on" in the rest of the jabber world).

By the way, I haven't looked at your 0084 receiving code, but if it 
doesn't already do so, it should probably not prefer PNG, so we make 
sure we read the (potentially animated) GIF if it is there.

> 
>> - turns minor get-info code duplication in buddy.c into massive code
>> duplication.  Abstract that out to a function or something.
> 
> Should be easy to fix.
> 
>> - for the gateway interaction stuff, we assume that anything in our
>> roster w/o an '@' is a gateway.  Why don't we simply send a disco
>> request to all such entries in the roster, so we can know for sure?
> 
> You wouldn't be any wiser for nodes that are offline (even gateways can 
> be offline). I talked to stpeter about that at last year's SoC, there 
> simply isn't a proper solution for it.
> Besides that, doing a disco#info storm is one of the main things 
> XEP-0115 is trying to avoid. That XEP only works when you have received 
> a presence packet from the peer, which is not the case when the gateway 
> is offline, but it is needed to recognize it as a gateway, so you can 
> tell it that you want to go online.

Granted I haven't talked to stpeter about this, but I guess I don't 
understand why we wouldn't get a disco#info result from an "offline" 
gateway.  And we should be able to avoid the storm where possible by 
waiting a few seconds to receive presence from the gateways that we'll 
get it from, and then disco the rest.

It seems to me (and by all means, correct me if I'm mistaken), that the 
only way we're going to have an "offline" gateway is if one of the 
following has happened:

- We've purposefully logged out of it this session, and therefore 
already know it is a gateway.
- We've just registered with it, so we already know it is a gateway.
- The server is busted
- It isn't a gateway.

I'm not sure what the expected behavior is when a busted server comes 
back online, but otherwise, I don't see a problem with not assuming it 
is a gateway until we actually know.

I don't use gateways, haven't used them in probably 7 or 8 years now 
(hence the lack of implementation up to this point ;-)), so if I've 
missed something, it won't surprise me terribly.


> 
>> - reverts a fix in jabber_disco_finish_server_info_result_cb to not send
>> presence until we receive the roster.  If we don't wait for the roster,
>> we get presence we're not expecting, and no one shows up the first time
>> you log in.
> 
> This looks like a simple mtn merge error to me.
> 
>> - ping.[ch] seems like overkill for such a small feature (I know that's
>> not his code, but I made the same comment about the original patch when
>> it was submitted in the tracker)
> 
> I don't quite see the cost in having those two files there.
> 
>> - inconsistent indenting throughout
> 
> Could you give an example?

Use an editor that shows you the difference between tabs and spaces, or 
set your tab spacing to something other than 8, and it should become 
readily apparent.

-Nathan