Proposal for an extended callbacks field

[Etan Reisner]

On Wed, Jul 25, 2007 at 05:57:57PM +0200, Andreas Monitzer wrote:
> On Jul 25, 2007, at 16:46, Etan Reisner wrote:
>
> >As I recall, and I don't recally particularly clearly, you weren't
> >allowed
> >to add what we considered a frivolous prpl_info callback, that is
> >one that
> >could be solved in other ways we considered more useful. There is a
> >huge
> >distinction between not being allowed to do something at one point
> >and not
> >being allowed to it at all.
>
> The first occurrence was the user tune information, which is now
> solved using the user status, requiring a bad hack in the XMPP code
> (since not the whole status information is sent via <presence/>
> stanzas any more, and thus some status changes don't require pushing
> the <presence/> stanza to the server).
> OT: This could be solved by not having a callback for setting the
> whole status, but for changing specific fields of the status. So you
> could have one callback for the field x, y and z, and another one for
> the status fields a, b and c.

Putting the user tune information into the status is the *right thing*,
the fact that it makes some code in the XMPP prpl more complicated is
either a side-effect of the current status api itself (which could be
changed if a change could be decided on) or a side-effect of the way XMPP
handles status stuff. Neither of which make having a prpl_info callback a
good idea, especially since the prpl_info stuff is as was pointed out
*static* while the status stuff is not (by design).

> The second occurrence was the registration callback. This wasn't
> solved, but a workaround was introduced by having an out-of-struct
> function for it.

That is a solution, the fact that it wasn't your initial proposal isn't
the point. The point is that having looked over the requirements it was
decided that a callback method was not the right one, especially since
your proposal included passing ui callbacks directly into the prpl.

> >I don't know why it is that you seem to keep over-reacting to the
> >comments
> >that come from the pidgin side of things but you really need to
> >stop it.
>
> I don't feel like I'm over-reacting (I'm not saying that I didn't do
> that in the past). Sean told me to avoid changes to the prpl struct
> at all costs (even sacrificing having a clean API or readability), so
> I proposed a solution to this issue. This is not a rant, but a
> rational way of proceeding when there are problems that hinder
> development. There were multiple cases where I answered to feature
> requests "I can't do that, because it'd require another function in
> the prpl struct". The most recent one that triggered this proposal
> was http://trac.adiumx.com/ticket/7326

In this case I felt like you had over-reacted in that the presentation of
this issue seemed to be that you had been explicitely barred from ever
adding anything to the prpl_info struct even when that was obviously the
right answer. And that is not something I have ever seen you be told, nor
do I really believe anyone would make a blanket statement like that.

Adding a prpl_info callback to unregister an account is *exactly* the
right thing to do. I support doing that whole-heartedly. I didn't see you
bring that up anywhere that I look, I don't routinely watch adium bugs had
you brought it up here I would have immediately supported it.

The point here is that I don't think you are in a position to really
assume what we will or will not allow in the prpl_info struct, so whenever
you run into situations where you think you need to mess with it *talk* to
us.

I also have a doubt that Sean actually said that as such, or if he did I
don't agree with it at all. Because when adding prpl_info members is the
right thing to do it should be done. So far your proposed changes that
'required' prpl_info struct members were discussed and decided to not need
such.

> >So again I ask, what exactly is the problem that you are running
> >into now
> >that requires a prpl_info callback?
>
> The ones I can I think of right now are:
>
> * setting a register callback

Discussed above.

> * unregistration

Discussed above.

> * vv (that one probably needs more than one)

Not brought up anywhere so far, at least not really, unless this is the
current problem you are running into. In which case solving it requires
the much awaited vv framework to be discussed and implemented, and nothing
as simple as adding a couple prpl_info structs will really suffice.

> The point is though, that this list can be extended indefinitely. I'm
> not the only one who wants to add features to some prpl I guess.
> Of course, in regular libraries, big features like vv would only be
> introduced in major version jumps, but since libpurple versioning is
> limited by the pidgin release cycle, the option of having major new
> features in minor versions would help tremendously. This would allow
> IM clients' devs other than pidgin's to implement those features into
> their libpurple-using clients without having to wait for the next
> pidgin release.

No, the list can't really be extended indefinitely, because at any given
time only so many changes will be needed before the next major release.
And we can increase the padding at each major release.

Splitting libpurple and pidgin versioning is something that has been
discussed on a number of occasions but there hasn't been a real push for
it as of yet. If there needs to be such a split it can almost certainly be
done.

And no, adding this hashtable wouldn't really allow for libpurple UIs to
add arbitrary features without needing a pidgin release unless they intend
to be tracking libpurple from MTN, because libpurple itself will still
need the code added to actually do things with the added callbacks, and
those additions will all block on a release, they may only block on a
minor/micro release as opposed to possibly/occasionally blocking on a
major release (but that should basically never happen, because that is the
point of the struct padding). And if the UI is going to be tracking MTN
then they can maintain any arbitrary changes they want in their libpurple
build because they won't be able to use the installed version of libpurple
anyway.

> andy

One last thing, unless I missed it you have *still* not told us what
problem you are running into at this point. Unless you aren't running into
a problem right now and are just trying to 'future-proof' yourself against
cases in the future where you come up with a design that you think
requires a prpl_info struct change and don't want to discuss with us if
that is in fact needed or if a better design exists.

And that is really the point here, the issues so far have never been 'do
not ever touch the prpl_info struct, if is forbidden and you are not
allowed to touch it' but rather that your original design ideas were
decided not to be the optimal ones for any of a number of reasons and
designs that didn't require prpl_info struct changes were suggested
instead.

Long story short: TALK to us when you are trying to figure out how to implement something, if it really does need a prpl_info struct change that is fine, but when it doesn't accept that and move on (this is where you tend to over-react by the waym

Long story short: TALK to us when you are trying to figure out how to
implement something, if it really does need a prpl_info struct change that
is fine, but when it doesn't accept that and move on (this is where you
tend to over-react by the way, because you assume that your original
design must be the best and get offended/angry when told otherwise and you
assume that we are being unreasonable ogres when suggesting you use a
different method despite our giving reasons for the suggestion).

	-Etan