Change the way accounts connect while keeping backwards compatibility

Review Request #1967 — Created Oct. 26, 2022 and submitted

Information

pidgin/pidgin
default

Reviewers

This adds a new method to PurpleProtocol named create_connection which allows
a protocol to return a subclassed PurpleConnection so it can avoid using
purple_connection_[gs]et_protocol_data. The default implementation for this
method, just creates a PurpleConnection with the given protocol, account,
and password.

This also adds two new methods to PurpleConnection; connect and
disconnect. The default implementation for these two just call
purple_protocol_login and purple_protocol_close respectively.

The long term intent of this, is to have all protocol plugins subclass
PurpleConnection and then remove the login and close methods from
PurpleProtocol and also remove purple_connection_[gs]et_protocol_data.

Connected the demo account and used the account actions to test reconnecting.
Connected an XMPP account after running and at startup.
Verified that the infinite loop bug on error due to connection failure is now fixed as well.

Summary ID
Change the way accounts connect while keeping backwards compability
This adds a new method to PurpleProtocol named create_connection which allows a protocol to return a subclassed PurpleConnection so it can avoid using purple_connection_set_protocol_[gs]et_protocol_data. The default implementation for this method, just create a PurpleConnection with the given protocol, account, and password. This also adds two new methods to PurpleConnection; connect and disconnect. The default implementation for these two just call purple_protocol_login and purple_protocol_close respectively. The long term intent of this, is to have all protocol plugins subclass PurpleConnection and then remove the login and close methods from PurpleProtocol as well as remove purple_connection_[gs]et_protocol_data.
9c59801d841a6dd7ef90fd1c51978ea7beb0b181
Description From Last Updated

Shouldn't you check error here? It'll leak if this returns early and is set.

QuLogicQuLogic

%TRUE if

QuLogicQuLogic

Since _purple_connection_new is gone, I guess this comment can go too?

QuLogicQuLogic

Not sure, should we set an error here?

QuLogicQuLogic

Maybe here also?

QuLogicQuLogic

Should this not be cleared in dispose now? I suppose it might happen to work because disconnect will set it …

QuLogicQuLogic

Do you want to document the default disconnect here, as done for connect?

QuLogicQuLogic

Extra space.

QuLogicQuLogic

This condition is always true, because you're checking it after purple_connnection_set_state(connection, PURPLE_CONNECTION_STATE_DISCONNECTING).

QuLogicQuLogic

Isn't the protocol's close method called in klass->disconnect? But that method call's now after the buddy iteration, when it was …

QuLogicQuLogic
grim
grim
grim
QuLogic
QuLogic
  1. 
      
  2. libpurple/account.c (Diff revision 3)
     
     

    Shouldn't you check error here? It'll leak if this returns early and is set.

  3. libpurple/connection.h (Diff revision 3)
     
     

    %TRUE if

  4. libpurple/connection.c (Diff revision 3)
     
     

    Since _purple_connection_new is gone, I guess this comment can go too?

    1. Did I miss something here?

  5. libpurple/connection.c (Diff revision 3)
     
     

    Not sure, should we set an error here?

  6. libpurple/purpleprotocol.c (Diff revision 3)
     
     

    Maybe here also?

  7. 
      
grim
QuLogic
  1. 
      
  2. libpurple/account.c (Diff revision 4)
     
     

    Should this not be cleared in dispose now?

    I suppose it might happen to work because disconnect will set it to NULL.

    1. Accounts out live connections so we need to be able to set it to null outside of dispose.

    2. Yes, but we should still clear it in dispose just in case something really weird happens, or if we happen to change the lifetimes later (e.g., maybe with PurpleConnectionManager?).

  3. libpurple/connection.h (Diff revision 4)
     
     

    Do you want to document the default disconnect here, as done for connect?

  4. libpurple/connection.c (Diff revision 4)
     
     
     

    Extra space.

  5. libpurple/connection.c (Diff revision 4)
     
     

    This condition is always true, because you're checking it after purple_connnection_set_state(connection, PURPLE_CONNECTION_STATE_DISCONNECTING).

  6. libpurple/connection.c (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    Isn't the protocol's close method called in klass->disconnect? But that method call's now after the buddy iteration, when it was previously before it in the old code that called purple_protocol_close directly.

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

Status: Closed (submitted)

Loading...