Re: [Evolution-hackers] EMsgComposer API support for "Send and Archive"

[Ben Liblit <liblit acm org>]

Milan Crha wrote:
> you are right, there is no good (and direct) API to achieve what you
> want right now. There is an indirect API.

Thanks for your ideas, Milan.  Using "notify::activity" to retrieve the
EActivity representing the send attempt is clever, but I agree that it
feels somewhat brittle.  Perhaps we can do this more directly:

    e_msg_composer_send(composer);
    EActivity *activity =
        e_activity_bar_get_activity(
            e_html_editor_get_activity_bar(
                e_msg_composer_get_editor(composer)));
    if (activity != NULL)
        g_signal_connect(activity, "notify::state", ...);

That ought to give me the activity that e_msg_composer_send() just
created (or NULL if it decided not to).  Is there any possible race
condition under which the activity might complete/cancel and go away
before I have an opportunity to grab it like this?  Or is there a race
condition under which some *other* activity might be set as the
activity bar's current activity before I can grab this one?

If the above works, then the only question is whether I can reliably
use "notify::state" on this EActivity to distinguish successful/failed
completion of the send attempt.  I think that I can.  Successsful
completion transitions the activity into the E_ACTIVITY_COMPLETED state
in composer_send_completed(), defined in "em-composer-utils.c". 
 Failure does not change the activity's state;
composer_send_completed() simply reduces its reference count.  But that
should be fine for me: I only need to act upon successful completion,
and that is easy to detect using "notify::state".

I like this!  Unless you think there's a race lurking in my code to
fetch the current EActivity, this seems to give me everything I need
with *no* API changes at all.

> The only current feasible way seems to me the code duplication, copy
> the implementation of the e_msg_composer_send() into your plugin
> (with other necessary parts) and call that, instead of calling
> e_msg_composer_send(). You cannot invoke EMsgComposer::presend signal
> before calling e_msg_composer_send() to verify whether the message
> will be send or not, because, if any signal listener asks users for
> anything, that could be double asked.

I don't quite understand your "double asked" concern.  If I were to
make my own private copy of e_msg_composer_send(), my private copy
would emit the "presend" signal just as the original does, and would
eventually call "e_msg_composer_get_message(..., msg_composer_send_cb,
...)", also just like the original.  My copy of e_msg_composer_send()
would not be calling the original e_msg_composer_send(), so where does
this "double ask" happen?

That being said, maintaining my own copy of e_msg_composer_send() is
not ideal.  I'd also need private copies of msg_composer_send_cb(),
async_context_free(), and the AsyncContext struct declaration. 
 e_msg_composer_send() directly or indirectly uses all of these but
they are not visible outside the original "e-msg-composer.c" source.

> I do not think returning an EActivity from e_msg_composer_send() is a
> good idea. The reason is that it's an API change. The thing is that
> the returned pointer should be referenced, thus each call would need
> to unref it. That means adding more responsibilities to the caller
> and other overheads.

I agree that we should not ask existing callers to g_object_unref() a
return value that they previously did not expect to see at all.  If we
believe that ignoring this return value will be the common case, is it
reasonable to return the EActivity *without* increasing its reference
count accordingly?  That would mean that a caller could ignore this
returned value without creating a leak.  Conversely, a caller that does
want to keep this value would need to know that it should call
g_object_ref() it to keep it alive.  Even in my case, I think I would
not need to call g_object_ref(), since I would not be keeping my own
reference to this EActivity.  Rather, I'd just be registering a signal
callback on it and after that I no longer need to track it myself.

Thinking about documentation and also language bindings, it should be
enough to annotate e_msg_composer_send()'s return value as "(transfer
none)".  Per <
https://wiki.gnome.org/Projects/GObjectIntrospection/Annotations>, that
marks the returned value as not owned by the recipient.  My
understanding is that GObject introspection bindings, then, should know
that the *caller* needs to increment the reference count rather than
assuming that e_msg_composer_send() has already done so.

> What about adding a "postsend" signal? The round trip of signals
> would be:
>     presend
>     send
>     postsend
> where the postsend will be called always after the send is complete,
> successfully or failed. That will also mean an API (ABI) change (the
> EMsgComposerClass structure will be larger), but it'll not add any
> additional requirements for the callers.

That would work for me provided we have some strong guarantee that no
other send can appear in the middle, as in:

        presend
        send
                presend
                send
                postsend
        postsend

Or even:

        presend
        send
                presend
                send
        postsend
                postsend

Notice that I'd have no way of distinguishing the preceding two
sequences, meaning I'd have no way of knowing whether or not a given
"postsend" corresponded to the "send" operation that I initiated.  Can
we guarantee that this would never happen?

-- Ben