[Pidgin] #1187: Send Custom Emoticons in MSN

[Pidgin]

#1187: Send Custom Emoticons in MSN
---------------------------+------------------------------------------------
  Reporter:  salinasv      |       Owner:  rlaager            
      Type:  patch         |      Status:  assigned           
  Priority:  minor         |   Milestone:  Merge MSNP14 Branch
 Component:  pidgin (gtk)  |     Version:  2.0                
Resolution:                |    Keywords:                     
   Pending:  0             |  
---------------------------+------------------------------------------------
Comment (by rlaager):

 I'm reading the diff right now. I'm not going to test anything today. I'm
 just going to comment as I go. Some of these things may be incredibly
 trivial, knowing me. ;)

 get_per_account.patch:
  * I would prefer you use "mtn diff" to generate your patches. Even if you
 do it file-by-file to split functional changes, that'd be better for me.
  * I don't like that pidgin_themes_get_proto_smileys() is doing custom
 smileys. That seems inconsistent with the name. Additionally, you've
 changed that function to being something that returns a GSList that the
 caller MUST NOT free with one that the caller MUST free. I haven't looked
 for callers of it to see if you've updated them, but I'd rather you did
 something else. Plus, this change (and adding the username parameter)
 would push us to 3.0.0, so that's another reason to change it.
  * Adding a member (username) to GtkIMHtmlToolbar breaks ABI, so this
 would force us to 3.0.0. Can this change be avoided without hackery? If
 so, please do that. If it needs hackery, then I'd rather accept the patch
 as-is for 3.0.0 and backport it to 2.x with whatever hackery separately.

 prpl.patch:
  * needs a space after the comma in the prototype for del_smiley.

 gtktoolbar.patch:
  * Why are you doing this? Call g_slist_free(smileys) instead.
 {{{
 +       while(smileys)
 +               smileys = g_slist_delete_link(smileys, smileys);
 }}}
  * The change to gtk_imthtmltoolbar_associate_smileys() breaks API/ABI
 compatibility as well. The same comments as above apply here. (Can you
 avoid it without hackery?)
  * The changes at the end here belong in another file, if you're going for
 splitting. ;)

 custom_send.patch:
  * You have a few things like this: "gtk_imhtml_get_account_name(GtkIMHtml
 *imhtml){". Put the opening brace at the beginning of the next line.
  * This is not necessary, as g_free() will check for NULL and return if
 it's NULL:
 {{{
 +       if(imhtml->account_name)
 +               g_free(imhtml->account_name);
 }}}
  * Removing the sml argument from gtk_imhtml_associate_smiley(), et al.,
 breaks API/ABI compatibility. You should leave the argument there (and
 ignore it), if that makes sense here. Then, the changes to remove it
 should be separated into another patch that we can apply for 3.0.0.
  * +void pidgin_themes_smiley_themeize(!GtkWidget *, gboolean custom); is
 another ABI breaker. Perhaps these changes are all unavailable, and we
 should just do this for 3.0.0?

 gtk_account_ui_ops.patch:
  * I would prefer that pidgin_account_custom_smiley_del() be named
 pidgin_account_delete_custom_smiley().
  * Likewise for pidgin_account_custom_smiley_add().
  * Do we do this (see below) everywhere? I think that you can assume
 purple_find_prpl() will always return something when you use the
 protocol_id member of an account.
 {{{
 +       proto = purple_find_prpl(account->protocol_id);
 +       if(!proto)
 +               return;
 }}}
  * "for( iter..." doesn't need that space
  * This leaks and the first line is unnecessary:
 {{{
 +       directory = g_malloc(sizeof("custom_emoticons"));
 +       directory = g_strdup("custom_emoticons");
 }}}
  * You're missing spaces after commas other places, as well. Likewise for
 "){" at the end of a line.
  * I'm not a big fan of "function (foo);" or "keyword(blah)". You use "if
 (" in a number of places.
  * Your "Add custom smiley fail at Write %s\n" line... Why is Write caps?
  * That entire section is indented too much.

 account.patch:
  * The comment about "the first element" doesn't make sense to me.
  * I'm also confused about why that'd be a char **.
  * Is it best to call the UI op for every single custom smiley there?
 Would it make more sense to pass it the entire list in one call?
  * You call del_custom_smiley without checking if it's NULL. This will
 crash in other UIs, including those that worked fine before this patch.

 man.patch:
  * g_malloc0(sizeof(struct _smiley)); can be replaced with g_new0(struct
 _smiley);
  * There's a TODO in here.
  * You have #ifndef _GAIM_GTKSMILEY_H in gtksmiley.h.  Obviously, that
 should be changed to not reference GAIM.

 After you fix the above issues (many of which I realize are trivial and
 formatting related, I realize), I think you should do a read through of
 this patch and make sure every line, block, and function is polished. It's
 very important to read the diffs and make sure cruft hasn't slipped
 through from development.

 I think it would also be a good idea (especially in terms of showing
 yourself what you've changed) if you were to do the entries in
 !ChangeLog.API. You may find a command like this helpful (from the top of
 your pidgin checkout): mtn diff `find -name "*.h"`

 Finally, is it really best to be passing a filename down to the prpl?
 (Note: It might be, I really don't know.) Did you consider using the
 !PurpleStoredImage stuff at all?

-- 
Ticket URL: <https://developer.pidgin.im/ticket/1187#comment:22>
Pidgin <http://pidgin.im>
Pidgin