Thoughts on this branch:

 - Why does passing channel = NULL in tp_simple_password_manager_prompt_for_channel_async() create a channel for you? Why not require channel != NULL and then create the channel in tp_simple_password_manager_prompt_async()?

 - Assumedly you will want to do something with the channel once the client is finished with it. There should be a tp_simple_password_manager_prompt_for_channel_finish() which returns the channel you used as one of its arguments.

(In reply to comment #1)
> Thoughts on this branch:
> 
>  - Why does passing channel = NULL in
> tp_simple_password_manager_prompt_for_channel_async() create a channel for you?
> Why not require channel != NULL and then create the channel in
> tp_simple_password_manager_prompt_async()?
> 
>  - Assumedly you will want to do something with the channel once the client is
> finished with it. There should be a
> tp_simple_password_manager_prompt_for_channel_finish() which returns the
> channel you used as one of its arguments.

Both of these points addressed in a new commit added to the branch mentioned above

Tp*Channel is currently used for client-side TpChannel sub-classes. We already
have TpTextChannel and TpStreamTubeChannel and will add TpFileTransferChannel
at some point.

On the other hand, server-side classes are generally named TpBase*:
TpBaseChannel, TpBaseClient, TpBaseConnection, etc.
So I think this class should be named TpBaseSimplePasswordChannel


+ * @channel: an output location to retrieve the custom password channel that
+ * was passed to tp_simple_password_manager_prompt_for_channel_async()
Should be annotate with (transfer none)
Or maybe it should ref it and be (transfer full), I *think* we usually return
a ref in such case.

 * Returns: a #GString with the password (or byte-blob) retrieved
+ *  by @manager

+ * Returns: a #GString with the password (or byte-blob) retrieved
+ *  by @manager
Shouldn't we return a copy of this string? We should have a transfer
annotation anyway.

Also, the lifetime of the GString stored in the GSimpleAsyncResult isn't very
clear to me. We get it from a signal, so it would be cleaner/safer to copy the
GString stored in the result and pass a GDestroyNotify to
g_simple_async_result_set_op_res_gpointer().
I'm all new to this code so please let me know if I missed something.

2 new commits pushed to my branch.  See explanations below

(In reply to comment #3)
> Tp*Channel is currently used for client-side TpChannel sub-classes. We already
> have TpTextChannel and TpStreamTubeChannel and will add TpFileTransferChannel
> at some point.
> 
> On the other hand, server-side classes are generally named TpBase*:
> TpBaseChannel, TpBaseClient, TpBaseConnection, etc.
> So I think this class should be named TpBaseSimplePasswordChannel

I felt that TpBaseSimplePasswordChannel was a bit long, and in some ways 'Base' implies that it's simple, so I decided to rename this class to TpBasePasswordChannel.  If you disagree with that, I can still rename it to TpBaseSimple...

> + * @channel: an output location to retrieve the custom password channel that
> + * was passed to tp_simple_password_manager_prompt_for_channel_async()
> Should be annotate with (transfer none)
> Or maybe it should ref it and be (transfer full), I *think* we usually return
> a ref in such case.

I added an annotation for the current behavior (transfer none).  I think it's analogous to e.g.  g_socket_listener_accept_finish() where the 'source_object' that the user set earlier is passed back out but without a ref.  I think that's kind of nice since it won't accidentally leak, but the user can still easily take a ref if necessary.  But I'm willing to be convinced otherwise.

> 
>  * Returns: a #GString with the password (or byte-blob) retrieved
> + *  by @manager
> 
> + * Returns: a #GString with the password (or byte-blob) retrieved
> + *  by @manager
> Shouldn't we return a copy of this string? We should have a transfer
> annotation anyway.

In this case, the API is already in released tp-glib, so changing it to return a copy of the string would cause leaks in existing code.  So I don't think we can change that.  And I made my prompt_for_channel_finish() function behave the same as prompt_finish().  I've added the (transfer none) annotations though.
 
> Also, the lifetime of the GString stored in the GSimpleAsyncResult isn't very
> clear to me. We get it from a signal, so it would be cleaner/safer to copy the
> GString stored in the result and pass a GDestroyNotify to
> g_simple_async_result_set_op_res_gpointer().
> I'm all new to this code so please let me know if I missed something.

Yes, it would probably be a little bit more future-proof to copy the GString as you suggest.  I guess that the string is only guaranteed to live until the end of the signal.  This is not a problem in the current implementation since we call g_simple_async_result_complete() from within the signal handler, so by the time it returns, the string will still be alive.  By contrast, if we copied the string and used a GDestroyNotify, the string would actually be destroyed slightly earlier (e.g. when the GSimpleAsyncResult object is unreffed at the end of this function).  So it didn't seem all that important to me to copy the string.  The only situation where I can see this becoming important is if we change to using complete_in_idle() instead of complete(), and that seems unlikely to me.

(In reply to comment #4)
> (In reply to comment #3)
> I felt that TpBaseSimplePasswordChannel was a bit long, and in some ways 'Base'
> implies that it's simple, so I decided to rename this class to
> TpBasePasswordChannel.  If you disagree with that, I can still rename it to
> TpBaseSimple...

Yeah that's fine.

> > + * @channel: an output location to retrieve the custom password channel that
> > + * was passed to tp_simple_password_manager_prompt_for_channel_async()
> > Should be annotate with (transfer none)
> > Or maybe it should ref it and be (transfer full), I *think* we usually return
> > a ref in such case.
> 
> I added an annotation for the current behavior (transfer none).  I think it's
> analogous to e.g.  g_socket_listener_accept_finish() where the 'source_object'
> that the user set earlier is passed back out but without a ref.  I think that's
> kind of nice since it won't accidentally leak, but the user can still easily
> take a ref if necessary.  But I'm willing to be convinced otherwise.

Yeah I think that's ok, the user should already have a ref on the channel
anyway.

> >  * Returns: a #GString with the password (or byte-blob) retrieved
> > + *  by @manager
> > 
> > + * Returns: a #GString with the password (or byte-blob) retrieved
> > + *  by @manager
> > Shouldn't we return a copy of this string? We should have a transfer
> > annotation anyway.
> 
> In this case, the API is already in released tp-glib, so changing it to return
> a copy of the string would cause leaks in existing code.  So I don't think we
> can change that.  And I made my prompt_for_channel_finish() function behave the
> same as prompt_finish().  I've added the (transfer none) annotations though.

You're right, I didn't notice that was already public API (as said, I'm new to
this code :).

> > Also, the lifetime of the GString stored in the GSimpleAsyncResult isn't very
> > clear to me. We get it from a signal, so it would be cleaner/safer to copy the
> > GString stored in the result and pass a GDestroyNotify to
> > g_simple_async_result_set_op_res_gpointer().
> > I'm all new to this code so please let me know if I missed something.
> 
> Yes, it would probably be a little bit more future-proof to copy the GString as
> you suggest.  I guess that the string is only guaranteed to live until the end
> of the signal.  This is not a problem in the current implementation since we
> call g_simple_async_result_complete() from within the signal handler, so by the
> time it returns, the string will still be alive.  By contrast, if we copied the
> string and used a GDestroyNotify, the string would actually be destroyed
> slightly earlier (e.g. when the GSimpleAsyncResult object is unreffed at the
> end of this function).  So it didn't seem all that important to me to copy the
> string.  The only situation where I can see this becoming important is if we
> change to using complete_in_idle() instead of complete(), and that seems
> unlikely to me.

Ok, as long as you know what you're doing that's fine with me.

I reviewed the credentials-storage branch yesterday, but as you pushed your
new commits on top of sasl-may-save-response I guess I should review the extra
2 commits as well.

e3c4afbd6535e458a217afb7921d83ad9052a7b7
Is that in a spec release? We usually update the whole spec to a new version
rather than just one file; so we can say in tp-glib NEW file that the spec has
been update to this specific version.

0e97beb778367111acafe68974ca1b88ec7e1050
looks good

(In reply to comment #5)
> e3c4afbd6535e458a217afb7921d83ad9052a7b7
> Is that in a spec release? We usually update the whole spec to a new version
> rather than just one file; so we can say in tp-glib NEW file that the spec has
> been update to this specific version.

So I made a spec release and update tp-glib to it. This patch was not needed any more so I dropped it while rebasing on top of master. I added 2 commits fixing doc and merged the branch; will be in 0.13.15.

Thanks for your work on this.