Bug 31900

(In reply to comment #0)
> 1. a ::finished signal for notifying when done. Or, if you want, a
> ::got-password or something, and an ::error. It doesn't really matter.
> 
> 2. tp_simple_password_manager_popup_channel to notify the manager to create a
> new auth channel and let the world know about it.

How about tp_simple_password_manager_prompt_async() which is GIO-ish and can either return a password (a byte-blob, i.e. GString or GByteArray) or an error?

> we
> could keep the simple auth channel out of the API and put the signal on the
> manager

I'd prefer that - a bit like TpBaseContactList.

(In reply to comment #1)
> (In reply to comment #0)
> How about tp_simple_password_manager_prompt_async() which is GIO-ish and can
> either return a password (a byte-blob, i.e. GString or GByteArray) or an error?

Sounds beautiful. I'll get implementing.

Here's a branch.

And here's a haze branch that uses it:
http://git.collabora.co.uk/?p=user/jonny/telepathy-haze.git;a=shortlog;h=refs/heads/simple-sasl

I left the TpSimplePasswordChannel::finished signal in place as that is what I was using before, and it felt a bit weird having a manager_prompt_async just making a channel_prompt_async call.

Thoughts please?

A question that popped out of the haze implementation review:

Do we also want to pass the SASLErrorDetails a{sv} to the prompt_async callback?

The channel's header should be -internal.h, assuming you don't want it to be API. You also need to edit docs/reference/Makefile.am to add it to the list of headers to not scan (I'm fairly sure wildcards don't work in that context, sadly).

The get_type function should be underscore-prefixed to not be ABI, which has the side-effect that the init and class_init get prefixed too.

+GType tp_simple_password_channel_get_type (void);

Instead of the current "finished" signal, you could emit a signal with no arguments, and have the recipient call a method to get the info out - that might be nicer? I'd use something like this:

    GString *_tp_simple_password_channel_get_password (GError **error);

Or, if you're keeping the signal arguments:

> +      _tp_marshal_VOID__POINTER_POINTER,
> +      G_TYPE_NONE, 2,
> +      G_TYPE_STRING, G_TYPE_POINTER);

That should be VOID__STRING_POINTER. Sadly, development (odd-numbered) versions of GLib really are that pedantic, and will warn.

What I think you *actually* want here is the BOXED marshaller and G_TYPE_GSTRING.

If you're going to throw a GError via a signal, I'd be inclined to use three arguments (guint, gint, string) like TpProxy::invalidated does.

> Do we also want to pass the SASLErrorDetails a{sv} to the prompt_async
> callback?

I don't think we do; what would go there, except the debug message? The Handler for SASL channels has no way to provide anything beyond a reason code and a debug message.

(Escape hatch: we can always add a get_more_info() method you can call from the callback, if needed.)

(In reply to comment #5)
> The channel's header should be -internal.h, assuming you don't want it to be
> API.

Done.

> You also need to edit docs/reference/Makefile.am to add it to the list of
> headers to not scan (I'm fairly sure wildcards don't work in that context,
> sadly).

Already done, as said on IRC.

> The get_type function should be underscore-prefixed to not be ABI, which has
> the side-effect that the init and class_init get prefixed too.
> 
> +GType tp_simple_password_channel_get_type (void);

Done.

> > +      _tp_marshal_VOID__POINTER_POINTER,
> > +      G_TYPE_NONE, 2,
> > +      G_TYPE_STRING, G_TYPE_POINTER);
> 
> That should be VOID__STRING_POINTER. Sadly, development (odd-numbered) versions
> of GLib really are that pedantic, and will warn.

Oh, whoops, I changed the marshaller but forgot to change the following argument!

> What I think you *actually* want here is the BOXED marshaller and
> G_TYPE_GSTRING.

Yes, you're right, done.

> If you're going to throw a GError via a signal, I'd be inclined to use three
> arguments (guint, gint, string) like TpProxy::invalidated does.

Done.

> > Do we also want to pass the SASLErrorDetails a{sv} to the prompt_async
> > callback?
> 
> I don't think we do; what would go there, except the debug message? The Handler
> for SASL channels has no way to provide anything beyond a reason code and a
> debug message.
> 
> (Escape hatch: we can always add a get_more_info() method you can call from the
> callback, if needed.)

Okay, cool, thanks.

Looks good, sorry for the delay.

Thanks.