Proposal for an extended callbacks field

[Ethan Blanton]

Andreas Monitzer spake unto us the following wisdom:
> On Jul 26, 2007, at 00:01, Ethan Blanton wrote:
> >There is definitely some tension here; Ad-hoc commands, for example,
> >are certainly interesting, but are really XMPP-specific.  This means
> >that they probably don't belong in a global prpl information
> >structure, as nothing else is likely to be able to use them.
> 
> However, that wouldn't matter in the hashtable. If the key exists,  
> the application can use it. If it doesn't exist, there's no mention  
> of them in any header, so there's no pollution with XMPP-specific  
> stuff happening.

Right, obviously one can solve this by going dynamic.

> Of course, the type safety is a valid concern. This would have to be  
> solved by some typedefs and casts.

You can't solve type safety in this fashion with typedefs and casts.
The best you can hope to do is GObject-style "at least if it's a
GObject we can type it" type-safety.  Do not be misled by the behavior
of Objective C with respect to typing, remember that it has just
enough compiler support to make the runtime support safe.

> >Learning about good design is certainly not a waste of your time, and
> >nor is solving things in a sane and supportable fashion.
> 
> I admit that I'm only used to object-oriented design, which includes  
> encapsulation and separation of concerns. Having a large procedural  
> codebase is pretty new to me, so my solutions might not always be
> similar to the libpurple's dev's solutions.

Fortunately, the libpurple codebase is very much object-oriented, so
you shouldn't have any metaphor shear here.

> In Objective C (the main programming language used by the Adium  
> project), the issue at hand is actually solved using linked lists  
> with the method name as the search key right in the runtime. I didn't  
> invent this idea out of thin air.

By this sentence, I assume that you are referring to the dynamic
runtime, rather than any particular object-oriented principles.  A
linked list is obviously not a good data structure for what you're
wanting, but sure, a hash table mapping strings (or brokered symbols
of some type) to functions is a reasonable way to achieve dynamic
dispatch in many respects; note that several prpls use this method in
their parsing (IRC, for example), and that we use an essentially
similar method for signal dispatch.

In Objective C, the big difference is that, as long as your arguments
are of *any* object type, you are essentially type-safe, or at least
guaranteed to catch the error in a coherent fashion, regardless of the
fact that your functions are unknown a priori.  If you are speaking of
the ObjC method dispatch itself, the story is even different yet, as
there are various other mechanisms around the method invocation which
give you yet more safety (counting arguments, etc.).

> >I'm sorry if you are being pressured to perform over learning and 
> >doing things right; I would be surprised if this is truly the case,
> >and perhaps you should have a discussion with your mentor and the
> >other Adium developers about this.
> 
> During the summer, I'm programming. Learning is what I'm doing the  
> rest of the year. FYI, I do have a Bachelor's degree in Software  
> Engineering and already wrote some successful commercial  
> applications, so I'm not a complete noob in that area.
> However, I did learn the libpurple design to a certain degree, so I  
> don't think I'm stepping on your toes every time I'm adding something.

SoC should be about learning, as well as programming.  If it's not,
there is a priority mixup somewhere.

I don't mean to imply that you're stepping on any toes; I think the
only frustration on the Pidgin side in any of this is that we see the
same issues coming up multiple times, and there doesn't appear to have
been any internalization of the answers which have come before.

> >I get the impression that you believe that the responses to your
> >suggestions and implementations coming from the Pidgin/libpurple team
> >have been unreasonable.  I'm sorry that you feel that way, but it
> >really has not been the case.  Perhaps more buy-in from your mentor,
> >the Adium team, or interaction with the Pidgin developers would help
> >ease this apparent friction.
> 
> In case you didn't know, Augie Fackler, who started this thread, is  
> my SoC mentor, so he does help me in this area. I don't have anybody  
> at the pidgin/libpurple team to talk to, which might be a problem. I  
> only talk to Sean sometimes on IRC when I can catch him online.

You have the entire team, either in #pidgin or devel at c.p.i.  Please,
join us and discuss whatever you like.

> >>Maybe my underlying problem is that I'm supposed to implement major
> >>features into the XMPP plugin during a 3 months period without
> >>resulting in a major version bump.
> >
> >So far, I haven't seen any features that would *require* a major
> >version bump, other than voice and video.  As Etan has requested
> >numerous times, if you could tell us what you're *trying* to do, we
> >can help you get it done.
> 
> It's not a single showstopper I could point my finger at, but the 
> issue crept up every now and then, and it really gets annoying over 
> time. The functions that are already implemented and have to be used
> from the application but are not in the prpl struct right now are:
> 
> purple_account_set_register_callback

To my understanding, this was simply an unfortunate API limitation,
right?  If register_user took a callback argument, this would just Go
Away?  I understand that you needed a fix Now, and this is a 3.0
change, so another function was needed.  This should have taken a
reserved field, if it's using independent bookkeeping now.

> jabber_register_gateway

This should probably also take a reserved field.

> jabber_adhoc_execute
> jabber_ping_jid
> jabber_user_search

I don't understand why any of these would need a reserved field at
all.  User search I could see an argument for, as one could envision
other protocols having such a feature, as well.


> Note that the first one should probably be replaced by a registration  
> callback in the _PurpleAccountUiOps struct (so you don't have to  
> define a callback in addition to defining this struct). Sean  
> suggested the current solution, since he didn't want to use up one of  
> those four reserved spots on the struct.

I think a callback to the registration function itself would be
cleaner; in either case, none of these callbacks are holy relics to be
left alone at all costs, and a callback can be co-opted for such use,
if there is agreement that the functionality being implemented is
worthwhile.

> One additional function to be added to the prpl struct would be  
> unregister (as already mentioned).
> 
> >If something *is* a status, it should be handled as a status.  It's
> >that simple.
> 
> I agree on that one, back then I didn't really know how the status  
> system works. However, the system is still not suited as the ultimate  
> solution to all of the status problems. It's not the concept, but the  
> particular implementation, that needs a fix.

Understood.

> >"Easier to implement" isn't the only metric, and "bugs in the initial
> >implementation" is a nearly worthless metric.  Things like "logical",
> >"consistent", "maintainable", and "correct" are very important.
> 
> I agree on that in theory. However, the current workaround I had to  
> implement is _not_ easily maintainable, but still required.
> In case you don't know, that's how it's implemented right now:
> On every status change function call, the function compares the new  
> status values to the old ones stored in the JabberStream struct. If  
> those are different, it sends the appropriate stanza (either  
> <presence/>, the user tune, the user nick, the user avatar or the  
> user mood right now). Afterwards, it copies the new values to the  
> JabberStream-struct.
> This means that every time a new status key is added, the  
> JabberStream struct and the status update function have to be  
> adapted. I hope that nobody forgets about this, otherwise things can  
> get really messy.

This sounds like something which is easy to fix in a clean and robust
fashion with a key-value mapping just like you're wanting to add to
handle the callbacks themselves.  It might take more than a minute or
two to implement that, of course.  You would be safer doing so in such
a situation, because the data you're storing is used in a tightly
scoped fashion (only within the xmpp prpl, and preferably largely by a
couple of simple accessor functions), and you can control your type
domain (for example, XMLNode subtrees, or what-have-you).

Ethan

-- 
The laws that forbid the carrying of arms are laws [that have no remedy
for evils].  They disarm only those who are neither inclined nor
determined to commit crimes.
		-- Cesare Beccaria, "On Crimes and Punishments", 1764
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://pidgin.im/pipermail/devel/attachments/20070725/2c3d3951/attachment.sig>