Hi,

Is this the kind of API are in need of? Am I missing anything?

/**
 * TpMessageMixinAcknowledgeHook:
 * @object: An instance of the implementation that uses this mixin
 * @message: The acknowledged message.
 *
 * Signature of a virtual method which may be implemented to notify a CM
 * of a message that was acknowledged.
 */

/**
 * tp_message_mixin_implement_acknowledge_hook:
 * @object: An instance of the implementation that uses this mixin
 * @acknowledge_hook: A hook function with the signature of
 *  #TpMessageMixinAcknowledgeHook
 *
 * Set a hook function for each message that is acknowledged, just before it is
 * removed from the pending queue.
 *
 * @since 0.13.2
 */

Besides review, we need to hear from Mikhail that this is the feature he needs. Also, I didn't add a regression test for tp_message_mixin_implement_acknowledge_hook, only for tp_text_mixin_implement_acknowledge_hook as the TpMessageMixin instance is harder to access in the current tests.

It looks good to me, at least now it's possible to do something (e.g. mark as read) in a CM on message acknowledge. I'm not sure how the second part is important to have, a CM could easily have mapping between internal objects and TpMessages or pending message IDs.

(Please set the patch keyword if you want things reviewed.)

I don't think it's worth adding this to TpTextMixin: the first step for anyone wanting to do new things with TpTextMixin should be "port to TpMessageMixin".

tp_message_mixin_take_received returns the pending message ID: the acknowledgement hook ought to have that pending message ID as a parameter.

In the Messages world, we emit PendingMessagesRemoved(au: Pending_Message_IDs), which in the dbus-glib world is actually a pending-messages-removed GObject signal on the Channel object, whose parameter is a GArray of guint.

I wonder whether it would be sufficient to have CM implementors just connect to that signal, and maintain a hash table of { pending message ID => internal object representing the message }?

If we keep the acknowledgement-hook support in its current form, the TpMessageMixin version needs tests, since it's the version that will be important in future. Two easy ways to do this spring to mind:

- have the Echo2 example emit a signal from its acknowledgement hook, and catch that signal in the test

- subclass the Echo2 example's channel, and set an acknowledgement hook in the subclass's class_init

One way to have "user data" like Mikhail suggested would be to add guint tp_message_mixin_take_received_full (GObject *object, TpMessage *message, gpointer user_data, GDestroyNotify destroy), where @destroy is called after the message has been acknowledged. In simple cases you wouldn't necessarily even need the acknowledgement hook.

(In reply to comment #4)
> (Please set the patch keyword if you want things reviewed.)
> 

This isn't ready for review yet, I will add the keyword when it is :)

> I don't think it's worth adding this to TpTextMixin: the first step for anyone
> wanting to do new things with TpTextMixin should be "port to TpMessageMixin".
> 
> tp_message_mixin_take_received returns the pending message ID: the
> acknowledgement hook ought to have that pending message ID as a parameter.
> 

It is in the TpMessage struct, but I just realized it is opaque.

> In the Messages world, we emit PendingMessagesRemoved(au: Pending_Message_IDs),
> which in the dbus-glib world is actually a pending-messages-removed GObject
> signal on the Channel object, whose parameter is a GArray of guint.
> 
> I wonder whether it would be sufficient to have CM implementors just connect to
> that signal, and maintain a hash table of { pending message ID => internal
> object representing the message }?

That makes a lot of sense! Artem, what do you think?

> 
> If we keep the acknowledgement-hook support in its current form, the
> TpMessageMixin version needs tests, since it's the version that will be
> important in future. Two easy ways to do this spring to mind:
> 
> - have the Echo2 example emit a signal from its acknowledgement hook, and catch
> that signal in the test
> 
> - subclass the Echo2 example's channel, and set an acknowledgement hook in the
> subclass's class_init

Good ideas, I'll use those if we have something to test (if the signal is not enough).

> 
> One way to have "user data" like Mikhail suggested would be to add guint
> tp_message_mixin_take_received_full (GObject *object, TpMessage *message,
> gpointer user_data, GDestroyNotify destroy), where @destroy is called after the
> message has been acknowledged. In simple cases you wouldn't necessarily even
> need the acknowledgement hook.

Since we have unique IDs, I don't think user data is essential. Keeping a mapping is easy enough, probably not worth complicating our API for that. But I could be convinced otherwise.

(In reply to comment #3)
> It looks good to me, at least now it's possible to do something (e.g. mark as
> read) in a CM on message acknowledge. I'm not sure how the second part is
> important to have, a CM could easily have mapping between internal objects and
> TpMessages or pending message IDs.

Artem, what are your thoughts on this. Is using the pending-messages-removed signal good enough here?

> Artem, what are your thoughts on this. Is using the pending-messages-removed
> signal good enough here?

Sure, should be enough, we did not know about the signal.

(In reply to comment #7)
> > Artem, what are your thoughts on this. Is using the pending-messages-removed
> > signal good enough here?
> 
> Sure, should be enough, we did not know about the signal.

Shall we WONTFIX this, then?

(In reply to comment #8)
> > Sure, should be enough, we did not know about the signal.
> 
> Shall we WONTFIX this, then?

This is one of the acceptable resolutions :)

WONTFIX, not needed. Please reopen (or open another bug) if you discover you need this for some reason.