Create a manager for conversations.

Review Request #677 — Created May 27, 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)
     
     
    Show all issues

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

  3. finch/gntconv.c (Diff revision 2)
     
     
    Show all issues

    Should be l->data, I believe.

  4. finch/gntconv.c (Diff revision 2)
     
     
    Show all issues

    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)
     
     
    Show all issues

    Extra blank line.

  6. finch/gntconv.c (Diff revision 2)
     
     
    Show all issues

    Same comments as the previous function.

  7. libpurple/protocols/null/nullprpl.c (Diff revision 2)
     
     
    Show all issues

    gint?

  8. libpurple/purpleconversationmanager.h (Diff revision 2)
     
     
    Show all issues

    #PurpleConversation\s or something like that?

  9. libpurple/purpleconversationmanager.h (Diff revision 2)
     
     
    Show all issues

    an

  10. libpurple/purpleconversationmanager.h (Diff revision 2)
     
     
    Show all issues

    @name

  11. libpurple/purpleconversationmanager.h (Diff revision 2)
     
     
    Show all issues

    @name

  12. libpurple/purpleconversationmanager.c (Diff revision 2)
     
     
     
     
    Show all issues

    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.

  13. libpurple/purpleconversationmanager.c (Diff revision 2)
     
     
    Show all issues

    !func(conversation, userdata)

  14. libpurple/purpleconversationmanager.c (Diff revision 2)
     
     
    Show all issues

    The hash table should be freed here?

  15. libpurple/purpleconversationmanager.c (Diff revision 2)
     
     
    Show all issues

    Don't need the if.

  16. libpurple/purpleconversationmanager.c (Diff revision 2)
     
     
    Show all issues

    Funny wrap indent.

  17. libpurple/purpleconversationmanager.c (Diff revision 2)
     
     
    Show all issues

    Funny wrap indent.

  18. libpurple/purpleconversationmanager.c (Diff revision 2)
     
     
    Show all issues

    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.

  19. pidgin/gtkblist.c (Diff revision 2)
     
     
    Show all issues

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

  20. pidgin/gtkconv.c (Diff revision 2)
     
     
    Show all issues

    Same questions as in finch.

  21. 
      
grim
grim
QuLogic
  1. Ship It!
  2. 
      
grim
Review request changed
Status:
Completed