Pidgin

Hi,

I just tried to write a mostly empty prpl, and discovered that when you
set list_icon to NULL in the prpl_info structure, gaim will crash when
you try to add an account (of any type) from the account window.

The following patch fixes the issue in the following way: if list_icon
isn't available, just take the plugin's name as protocol name.

Hope this helps,

Snark on #gaim

[Gaim-devel] Patches to the sourceforge tracker From: Mark Doliner <mark@ki...> - 2004-11-01 22:55 Can I make a request? That all patches intended for inclusion in Gaim be filed in Gaim's sourceforge patch tracker. Benefits of using the tracker instead of the mailing list: -Reduce the chance that a patch will be overlooked -Patches can be assigned to people -Discussion about a patch can be done in the tracker instead of the mailing list. This keeps the discussion between people interested in the patch, and reduces the number of emails on gaim-devel -There is a single place to view the history of any given patch (comments, updated patches, reasons for rejection, etc.) -Patches can be big, and emailing them to tons of people that might not be interested in testing/ reviewing them wastes a bit o' bandwidth Anyone have any good reasons against this? I'd like to know so I CAN KILL YOU. I mean, um, in case I overlooked something. If people still want to annouce patches here that's cool with me. It's probably a good idea, actually. Soooo... Ima request that, if you've sent a patch to gaim-devel and it hasn't been accepted, please add a tracker item for it. The tracker URL is http://sourceforge.net/tracker/?group_id=235&atid=300235 Thanks! -Mark -- O O Mark Doliner \ | mark@... \ | http://www.kingant.net "There needs to be a better word for weird."

Re: [Gaim-devel] Patches to the sourceforge tracker From: Don Seiler <don@se...> - 2004-11-02 02:46 Attachments: Message as HTML      On 17:34 Mon 01 Nov , Mark Doliner wrote: > Can I make a request? That all patches intended for inclusion in Gaim be= filed in Gaim's sourceforge=20 > patch tracker. +1 Another benefit is then distro maintainers have a central location to pick and choose their patches. --=20 Don Seiler don@... Public Key: http://pgp.mit.edu:11371/pks/lookup?op=3Dget&search=3D0xFC87F041 Fingerprint: 0B56 50D5 E91E 4D4C 83B7 207C 76AC 5DA2 FC87 F041

Re: [Gaim-devel] [PATCH] little check to avoid a crash From: Mark Doliner <mark@ki...> - 2004-11-04 04:50 On Mon, 01 Nov 2004 20:20:39 +0100, PUYDT Julien wrote > Hi, > > I just tried to write a mostly empty prpl, and discovered that when you > set list_icon to NULL in the prpl_info structure, gaim will crash > when you try to add an account (of any type) from the account window. > > The following patch fixes the issue in the following way: if > list_icon isn't available, just take the plugin's name as protocol name. > > Hope this helps, > > Snark on #gaim I've seen this problem before and agree that it's NotSoGreat. I didn't like this patch because it's not really the correct way to fix it. The list_icon function is pretty important... it basically tells the core what the short-name of the PRPL is, and it's used in more places than just the one you patch. Instead, I modified the plugin loading code so that it will not load plugins if they don't implement the list_icon function, login, or close. So you'll want to make sure your empty prpl implements those three functions (even if they don't do much). Perhaps "list_icon" should be renamed to "short_name" or "nickname"? -Mark -- O O Mark Doliner \ | mark@... \ | http://www.kingant.net "There needs to be a better word for weird."

Re: [Gaim-devel] [PATCH] little check to avoid a crash From: PUYDT Julien <jpuydt@fr...> - 2004-11-04 06:41 Le mercredi 03 novembre 2004 =E0 23:46 -0500, Mark Doliner a =E9crit : > I've seen this problem before and agree that it's NotSoGreat. I didn't= like > this patch because it's not really the correct way to fix it.=20 > The list_icon function is pretty important... it basically tells the c= ore what the > short-name of the PRPL is, and it's used in more places than just the o= ne you > patch. No problem. I only patched the hole I knew ; I'm not familiar enough with gaim's code (yet) to propose better. > Instead, I modified the plugin loading code so that it will not load pl= ugins > if they don't implement the list_icon function, login, or close. So yo= u'll > want to make sure your empty prpl implements those three functions (eve= n if > they don't do much). Indeed nicer. > Perhaps "list_icon" should be renamed to "short_name" or "nickname"? It definitely has a very misleading name. Perhaps those have-to functions should be documented in prpl.h, in struct _GaimPluginProtocolInfo? Snark

Re: [Gaim-devel] [PATCH] little check to avoid a crash From: Tim Ringenbach <omarvo@ho...> - 2004-11-04 15:13 PUYDT Julien wrote: >Le mercredi 03 novembre 2004 =E0 23:46 -0500, Mark Doliner a =E9crit : > =20 > >>Perhaps "list_icon" should be renamed to "short_name" or "nickname"? >> =20 >> >It definitely has a very misleading name. > I thought it was only used to decide which icon to use too. >Perhaps those have-to functions should be documented in prpl.h, in >struct _GaimPluginProtocolInfo? > I got the impression Chip was going to redo the core/prpl interface=20 sometime, and no one felt inclined to do much in the way of cleaing it=20 up or documenting it in the mean while. One of my own suggestions was to make the giant struct of function=20 pointers hidden and accessed through fuctions, at least by the prpls.=20 That way changes to the prpl struct wouldn't break binary or source=20 compatibility. Of course there would still be lots of other places to=20 break compatibility, so it might not help much, but at least reording=20 things wouldn't get function pointers set to the wrong members. --Tim

Re: [Gaim-devel] [PATCH] little check to avoid a crash From: Mark Doliner <mark@ki...> - 2004-11-04 23:07 On Thu, 04 Nov 2004 07:41:14 +0100, PUYDT Julien wrote > Perhaps those have-to functions should be documented in prpl.h, in > struct _GaimPluginProtocolInfo? Already done. I would point you to http://cvs.sourceforge.net/viewcvs.py/gaim/gaim/src/prpl.h?r1=1.189&r2=1.190 But sourceforge anonymous CVS is currently in a state of suck (surprise, surprise), and hasn't been updated yet. I should thank you for pointing out this problem and providing a patch. Thanks! -Mark -- O O Mark Doliner \ | mark@... \ | http://www.kingant.net "There needs to be a better word for weird."

Re: [Gaim-devel] [PATCH] little check to avoid a crash From: Mark Doliner <mark@ki...> - 2004-11-04 23:22 On Thu, 04 Nov 2004 09:09:59 -0600, Tim Ringenbach wrote > >>Perhaps "list_icon" should be renamed to "short_name" or "nickname"? > I thought it was only used to decide which icon to use too. You can basically grep list_icon src/*.c To see all the uses of it. It's used in logging to determine ~/.gaim/logs/THIS_DIRECTORY_NAME/bleh/bleh/bleh and it looks like it's used for some of the drag-n-drop stuff. But really, there's nothing that says that the value returned by "list_icon" must be used only to figure out which image to use. > I got the impression Chip was going to redo the core/prpl interface > sometime, and no one felt inclined to do much in the way of cleaing > it up or documenting it in the mean while. Two points (directed more at gaim-devel than you personally): 1. Rewriting things left and right isn't always good 2. Not fixing something that already exists because at some point in a year or two someone else may or may not change it is a bad way to think > One of my own suggestions was to make the giant struct of function > pointers hidden and accessed through fuctions, at least by the > prpls. That way changes to the prpl struct wouldn't break binary or > source compatibility. Of course there would still be lots of other > places to break compatibility, so it might not help much, but at > least reording things wouldn't get function pointers set to the > wrong members. The rendezvous PRPL does this in a way that avoids the reordering problem. -Mark -- O O Mark Doliner \ | mark@... \ | http://www.kingant.net "There needs to be a better word for weird."

Re: [Gaim-devel] [PATCH] little check to avoid a crash From: Ethan Blanton <eblanton@cs...> - 2004-11-04 23:50 Attachments: Message as HTML      Mark Doliner spake unto us the following wisdom: > Two points (directed more at gaim-devel than you personally): > 1. Rewriting things left and right isn't always good Definitely. > 2. Not fixing something that already exists because at some point in a ye= ar or > two someone else may or may not change it is a bad way to think Definitely. > > One of my own suggestions was to make the giant struct of function=20 > > pointers hidden and accessed through fuctions, at least by the=20 > > prpls. That way changes to the prpl struct wouldn't break binary or=20 > > source compatibility. Of course there would still be lots of other=20 > > places to break compatibility, so it might not help much, but at=20 > > least reording things wouldn't get function pointers set to the=20 > > wrong members. >=20 > The rendezvous PRPL does this in a way that avoids the reordering problem. I don't think that's true... Ethan --=20 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

Re: [Gaim-devel] [PATCH] little check to avoid a crash From: Tim Ringenbach <omarvo@ho...> - 2004-11-05 03:16 Mark Doliner wrote: >On Thu, 04 Nov 2004 09:09:59 -0600, Tim Ringenbach wrote > > >>One of my own suggestions was to make the giant struct of function >>pointers hidden and accessed through fuctions, at least by the >>prpls. That way changes to the prpl struct wouldn't break binary or >>source compatibility. Of course there would still be lots of other >>places to break compatibility, so it might not help much, but at >>least reording things wouldn't get function pointers set to the >>wrong members. >> >> > >The rendezvous PRPL does this in a way that avoids the reordering problem. > > > I haven't looked at what rendezvous does. Ethan pointed out to me what I said above was bogus, and would cause the creation of lots of single use functions. He suggested instead passing a name, value list to a single function, e.g. { ... "add_buddy", oscar_add_buddy, .... }, or a hash table of string keys, function pointer value pairs. It would then by converted internally to a table, so it didn't need to be looked up each time. I probably should have just pasted the irc log, but I didn't log it. --Tim

Re: [Gaim-devel] [PATCH] little check to avoid a crash From: Mark Doliner <mark@ki...> - 2004-11-05 04:24 On Thu, 04 Nov 2004 21:13:33 -0600, Tim Ringenbach wrote > >>One of my own suggestions was to make the giant struct of function > >>pointers hidden and accessed through fuctions, at least by the > >>prpls. That way changes to the prpl struct wouldn't break binary or > >>source compatibility. Of course there would still be lots of other > >>places to break compatibility, so it might not help much, but at > >>least reording things wouldn't get function pointers set to the > >>wrong members. > > > >The rendezvous PRPL does this in a way that avoids the reordering problem. > > I haven't looked at what rendezvous does. > > Ethan pointed out to me what I said above was bogus, and would cause > the creation of lots of single use functions. He suggested instead > passing a name, value list to a single function, e.g. { ... > "add_buddy", oscar_add_buddy, .... }, or a hash table of string keys, > function pointer value pairs. It would then by converted internally > to a table, so it didn't need to be looked up each time. > > I probably should have just pasted the irc log, but I didn't log it. > > --Tim No really, what's wrong with doing prpl_info.list_icon = rendezvous_prpl_list_icon; In the init_plugin() function of the PRPL? -Mark -- O O Mark Doliner \ | mark@... \ | http://www.kingant.net "There needs to be a better word for weird."

Re: [Gaim-devel] [PATCH] little check to avoid a crash From: Nathan Walp <faceprint@fa...> - 2004-11-05 08:41 Attachments: signature.asc      Mark Doliner wrote: <snip> > No really, what's wrong with doing > prpl_info.list_icon = rendezvous_prpl_list_icon; > In the init_plugin() function of the PRPL? > -Mark It's source-compatable, but not binary-compatable if we change the order. prpl_info.list_icon is calculated into an actual address at compile-time, not at run-time.

Re: [Gaim-devel] [PATCH] little check to avoid a crash From: Ethan Blanton <eblanton@cs...> - 2004-11-05 13:35 Attachments: Message as HTML      Mark Doliner spake unto us the following wisdom: > No really, what's wrong with doing > prpl_info.list_icon =3D rendezvous_prpl_list_icon; > In the init_plugin() function of the PRPL? > -Mark That isn't robust to changes in the prpl_info structure any more than a giant static initialization of the structure is. The offset of prpl_info.list_icon is still computed at compile time. Ethan --=20 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

6 messages have been excluded from this view by a project administrator.