Port the KWallet Keyring to the new CredentialProvider API.

Review Request #575 — Created March 19, 2021 and submitted

Information

pidgin/pidgin
default
48fd16c109c4

Reviewers

Port the KWallet Keyring to the new CredentialProvider API.
  • connected account with wallet locked, verified that we weren't prompted for a password until the wallet was unlocked.
  • connected account, didn't save password, made sure it connected and wasn't stored in kwalletmanager5
  • connected account, saved password, made sure it connected and verified the password was stored in kwalletmanager5
  • reconnected account, made sure the account connected without prompting
  • reopened pidgin, made sure the account connected without prompting.
  • disconnected pidgin from kwalletmanager5, re-connected account, verified it reconnected via debug and kwalletmanager5
  • force closed the wallet in kwalletmanager5, re-connected account, unlocked wallet, verified no password prompts and that the account connected.
  • removed the account and verified the password was removed from kwalletmanager5
Description From Last Updated

g_task_return_new_error

QuLogicQuLogic

failed to read

QuLogicQuLogic

The only place error would be set would be in the above if; why split this in two and not …

QuLogicQuLogic

g_task_return_new_error

QuLogicQuLogic

Again, why split this from the if(result != 0)? And then can use g_task_return_new_error.

QuLogicQuLogic

g_task_return_new_error

QuLogicQuLogic

Combine with if(result != 0) and use g_task_return_new_error.

QuLogicQuLogic

g_task_return_new_error

QuLogicQuLogic

You've been using C++ false/true; is true equal to false, or can you not use if(this->connected) {?

QuLogicQuLogic

Should the queue be cleared when closing?

QuLogicQuLogic

Should you only open if there's something in the queue?

QuLogicQuLogic

Since Request takes a ref, should you unref task at the end of this function?

QuLogicQuLogic

Since Request takes a ref, should you unref task at the end of this function?

QuLogicQuLogic

Since Request takes a ref, should you unref task at the end of this function?

QuLogicQuLogic

Do you need a default if the wrapper checks klass->activate before calling it?

QuLogicQuLogic

Not changed here, but this should be startup.

QuLogicQuLogic

Shuts down

QuLogicQuLogic
grim
grim
grim
QuLogic
  1. 
      
  2. Show all issues

    g_task_return_new_error

    1. bleh I knew something like this existed but I forgot to check.. sorry about that.

  3. Show all issues

    failed to read

  4. Show all issues

    The only place error would be set would be in the above if; why split this in two and not return the error directly in the if(result != 0)?

    And then this should be g_task_return_new_error.

  5. Show all issues

    g_task_return_new_error

  6. Show all issues

    Again, why split this from the if(result != 0)?

    And then can use g_task_return_new_error.

  7. Show all issues

    g_task_return_new_error

  8. Show all issues

    Combine with if(result != 0) and use g_task_return_new_error.

  9. Show all issues

    g_task_return_new_error

  10. Show all issues

    You've been using C++ false/true; is true equal to false, or can you not use if(this->connected) {?

    1. I think there was more to this before and I didn't clean it up well..

  11. Show all issues

    Should the queue be cleared when closing?

    1. we should probably cancel everything yeah..

    2. Sorry, I might walk this back a bit; it looks like close is called when externally closed too. Do we want to cancel everything in that case? Or should we wait for a re-open attempt?

    3. If we don't accounts will be in a limbo state until another task causes us to process the queue. If we wanted to leave those open then we'd need to add a timeout or something to attempt to process the queue again.

    4. Right, makes sense.

  12. Show all issues

    Should you only open if there's something in the queue?

    1. that's a good call.

  13. Show all issues

    Since Request takes a ref, should you unref task at the end of this function?

  14. Show all issues

    Since Request takes a ref, should you unref task at the end of this function?

  15. Show all issues

    Since Request takes a ref, should you unref task at the end of this function?

  16. libpurple/purplecredentialprovider.c (Diff revision 4)
     
     
    Show all issues

    Do you need a default if the wrapper checks klass->activate before calling it?

    1. This is to deal with g_warnings for subclasses not implementing it.

    2. What's going to trigger the warning? GObject?

    3. So I guess I'm confusing the warning from unimplemented functions for GInterface with this. I threw together the following proof of concept and there were no warnings...

      #include <glib.h>
      #include <glib-object.h>
      
      G_DECLARE_DERIVABLE_TYPE(TestAbstract, test_abstract, TEST, ABSTRACT, GObject)
      
      struct _TestAbstractClass {
          GObjectClass parent;
      
          void (*somefunc)(TestAbstract *abstract);
      };
      
      G_DEFINE_ABSTRACT_TYPE(TestAbstract, test_abstract, G_TYPE_OBJECT)
      
      static void
      test_abstract_somefunc(TestAbstract *abstract) {
          TestAbstractClass *klass = TEST_ABSTRACT_GET_CLASS(abstract);
      
          if(klass != NULL && klass->somefunc) {
              klass->somefunc(abstract);
          }
      }
      
      static void
      test_abstract_init(TestAbstract *t) {
      }
      
      static void
      test_abstract_class_init(TestAbstractClass *klass) {
      }
      
      G_DECLARE_FINAL_TYPE(TestConcrete, test_concrete, TEST, CONCRETE, TestAbstract);
      
      struct _TestConcrete {
          TestAbstract parent;
      };
      
      G_DEFINE_TYPE(TestConcrete, test_concrete, test_abstract_get_type())
      
      static void
      test_concrete_init(TestConcrete *t) {
      }
      
      static void
      test_concrete_class_init(TestConcreteClass *klass) {
      }
      
      gint
      main(gint argc, gchar *argv[]) {
          GObject *obj = g_object_new(test_concrete_get_type(), NULL);
      
          test_abstract_somefunc(TEST_ABSTRACT(obj));
      
          return 0;
      }
      
  17. libpurple/purpleprivate.h (Diff revision 4)
     
     
    Show all issues

    Not changed here, but this should be startup.

    1. fixed in another pr.

  18. libpurple/purpleprivate.h (Diff revision 4)
     
     
    Show all issues

    Shuts down

    1. fixed in another pr.

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