Create a manager for conversations.

Review Request #677 — Created May 28, 2021 and submitted

Information

pidgin/pidgin
default
391d7cd30c38

Reviewers

I Skipped unit tests because need a connection and full protocol implementation to do it properly.

Compiled and ran with an IRC account.

Description From Last Updated

Do you really need two vars? Seems like g_list_delete_link would work.

QuLogicQuLogic

Should be l->data, I believe.

QuLogicQuLogic

Actually, now it seems like purple_conversations_get_all is called multiple times; can you not free here and start from list again …

QuLogicQuLogic

Extra blank line.

QuLogicQuLogic

Same comments as the previous function.

QuLogicQuLogic

gint?

QuLogicQuLogic

#PurpleConversation\s or something like that?

QuLogicQuLogic

an

QuLogicQuLogic

@name

QuLogicQuLogic

@name

QuLogicQuLogic

All callers have already checked these things, so they're redundant, except for purple_conversation_manager_find_chat_by_id, which passes name=NULL and is broken by …

QuLogicQuLogic

!func(conversation, userdata)

QuLogicQuLogic

The hash table should be freed here?

QuLogicQuLogic

Don't need the if.

QuLogicQuLogic

Funny wrap indent.

QuLogicQuLogic

Funny wrap indent.

QuLogicQuLogic

Why is this guint instead of gint?

QuLogicQuLogic

Is gtkblist->menutrayicon not just created in the below deleted code, and thus unused now?

QuLogicQuLogic

Same questions as in finch.

QuLogicQuLogic
grim
  1. 
      
  2. Decided to do some more work here, so don't check this out yet...

  3. 
      
grim
QuLogic
  1. 
      
  2. finch/gntconv.c (Diff revision 2)
     
     

    Do you really need two vars? Seems like g_list_delete_link would work.

  3. finch/gntconv.c (Diff revision 2)
     
     

    Should be l->data, I believe.

  4. finch/gntconv.c (Diff revision 2)
     
     

    Actually, now it seems like purple_conversations_get_all is called multiple times; can you not free here and start from list again later? I assume the conversation count doesn't change.

  5. finch/gntconv.c (Diff revision 2)
     
     

    Extra blank line.

  6. finch/gntconv.c (Diff revision 2)
     
     

    Same comments as the previous function.

  7. libpurple/purpleconversationmanager.h (Diff revision 2)
     
     

    #PurpleConversation\s or something like that?

  8. libpurple/purpleconversationmanager.c (Diff revision 2)
     
     
     
     

    All callers have already checked these things, so they're redundant, except for purple_conversation_manager_find_chat_by_id, which passes name=NULL and is broken by this name != NULL check.

    1. You can drop the others too? It's not public.

    2. I always get hesitent with this as it could eventually get made public, but then again since it is named _internal that's probably fine.

  9. libpurple/purpleconversationmanager.c (Diff revision 2)
     
     

    !func(conversation, userdata)

  10. libpurple/purpleconversationmanager.c (Diff revision 2)
     
     

    The hash table should be freed here?

  11. libpurple/purpleconversationmanager.c (Diff revision 2)
     
     

    Don't need the if.

  12. libpurple/purpleconversationmanager.c (Diff revision 2)
     
     

    Funny wrap indent.

  13. libpurple/purpleconversationmanager.c (Diff revision 2)
     
     

    Funny wrap indent.

  14. libpurple/purpleconversationmanager.c (Diff revision 2)
     
     

    Why is this guint instead of gint?

    1. i was going with guint because id's aren't typically negative, but I'll change it back.

  15. pidgin/gtkblist.c (Diff revision 2)
     
     

    Is gtkblist->menutrayicon not just created in the below deleted code, and thus unused now?

  16. pidgin/gtkconv.c (Diff revision 2)
     
     

    Same questions as in finch.

  17. 
      
grim
grim
QuLogic
  1. Ship It!
  2. 
      
grim
Review request changed

Status: Closed (submitted)

Loading...