[Bug 34091] Allow TpSimplePasswordManager to be used with custom channels

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=34091

--- Comment #4 from jonner <jonathon at quotidian.org> 2011-02-23 20:36:07 PST ---
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.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.