SetBanner (misleading)

[hakaku]

Here's the description given in banner.h:

{{{ #!c

void (SetBanner)(Player p, Banner banner, int frombiller); / sets banner. NULL to remove. everyone except the biller modules * should have frombiller false. */ }}}

According to the given description, I should be setting int frombiller to //FALSE//. I can only assume that this means that the banner is not suppose to get sent to the biller, since modules shouldn't really be touching players' banners across a network. It would really be more intuitive to name it int tobiller, since you're sending it to the biller rather than receiving it from the biller. Even the billing modules respect this.

The problem is, the value is getting inverted in the code, so that FALSE is changed to TRUE and vice versa. This means that any module that sets 'frombiller' to FALSE will be overwriting players' custom banners. From banners.c:

{{{ #!c

check_and_send_banner(p, !frombiller); }}}

I would like to see the //frombiller// changed to //tobiller//, remove the exclamation mark in banners.c, update the description in banners.h, and subsequently update the two billing modules to change 'TRUE' to 'FALSE'.

[Former user]

The problem runs a bit deeper than your bug report might suggest.

The problem is that the Billers use the CB_SET_BANNER to watch for an updated banner. All the flag does is tell the banner module whether or not the callback should be run during SetBanner. The flag is TRUE from the biller since you don't want to set the banner every time one comes in. This means that when setting a temporary banner, NO CALLBACK WILL BE EXECUTED. This is a problem for user modules that want to be notified of a banner change.

So, I think a better change is to alter the callback to include a parameter 'from_player', which is TRUE if the banner request was initiated directly by the player. The billing module will only relay the banner if this is TRUE.

The banner interface should be changed to drop the frombiller flag entirely, and a new function can be added for those (rare) circumstances where you want the callback to have from_player TRUE without having received a banner packet.

This reply is a bit rambling, but I hope it gets the rationale across. Typing it up at least let me know what changes are needed in the code.

Future work should involve introducing a banner adviser to allow the prevention of a player uploading their own banner. This should be a separate bug report.

[Former user]

This is addressed in 2183b7421d9aWiki macro error: Changeset 2183b7421d9a not found. and Wiki macro error: Changeset a5af8c4e7560 not found.