Bug 48210

(In reply to comment #0)
> So when the MUC channel is closed, but should remain open behind the scenes:
> 
> > tp_svc_channel_emit_closed (chan);
> > tp_base_channel_unregister ((TpBaseChannel *) chan);

API users shouldn't have to deal with the Closed signal directly. See also the way the TpBaseChannel API tries to make dealing with Destroyable (and respawning channels) easier.

What you basically want, I think, is a way to "come back" from tp_base_channel_destroyed(), plus some way to have the channel start off in a "disappeared" state? (Perhaps just not calling register, or setting channel-destroyed to TRUE initially?)

Perhaps tp_base_channel_disappear() would be a better name than tp_base_channel_destroyed() if you plan to come back, even if the effect is pretty similar.

TpExportableChannel::closed will also need augmenting with some sort of "pretend to close" and "come back" functionality, because it currently assumes that the two happen simultaneously.

> and when we want the channel to reappear:
> 
> > tp_base_channel_reset ((TpBaseChannel *) chan, initiator_handle, requested);
> > tp_base_channel_register ((TpBaseChannel *) chan);
> > /* notify the channel manager so it can _emit_new_channel */

tp_base_channel_appear (channel, initiator, requested) which does all three?

Then tp_base_channel_reopened() would become equivalent to an atomic pair of disappear() and appear(initiator, FALSE). (We assume that the initiator is someone other than us, because reopened() is designed for the "respawned by a message" case.)

> However, this makes TpBaseChannel pretty easy to abuse and get wrong

Indeed, but perhaps the API can help users out enough that they'll get it right.

(In reply to comment #1)
> API users shouldn't have to deal with the Closed signal directly. See also the
> way the TpBaseChannel API tries to make dealing with Destroyable (and
> respawning channels) easier.
> 
> What you basically want, I think, is a way to "come back" from
> tp_base_channel_destroyed(), plus some way to have the channel start off in a
> "disappeared" state? (Perhaps just not calling register, or setting
> channel-destroyed to TRUE initially?)
> 
> Perhaps tp_base_channel_disappear() would be a better name than
> tp_base_channel_destroyed() if you plan to come back, even if the effect is
> pretty similar.
> 
> TpExportableChannel::closed will also need augmenting with some sort of
> "pretend to close" and "come back" functionality, because it currently assumes
> that the two happen simultaneously.

Right, but we want to make sure there's a distinction between the channel having disappeared temporarily or disappeared for good.

We do want a channel, when unregistered, to emit Closed, but we also want a way for channel managers to ask "is this actually closed, should I unref it, or not?"

If we used _destroyed() then where would the distinction be? I kind of like what I've done in that destroyed ~= disposed, in that you can never come back from destroyed (hence the name).

So I'm a little confused as to what exactly you're counter-proposing here?

Use cases:

1. Text channel appears, closes+respawns, is really destroyed.

Already works:

* register() + emit_new_channel()
* (time passes)
* reopened() (emits ::closed with :channel-destroyed = FALSE;
  channel manager emits ChannelClosed and NewChannel in response)
* (time passes)
* destroyed() (emits ::closed with :channel-destroyed = TRUE;
  channel manager emits ChannelClosed in response)

2. MUC text channel is present within CM but not "officially" there yet, then appears, then closes+respawns, then is really destroyed

One possibility is:

* don't register(), don't emit_new_channel()
* (time passes)
* channel manager decides the channel should appear: now it
  calls register() and emit_new_channel(), and connects to ::closed
* (time passes)
* reopened() (emits ::closed with :channel-destroyed = FALSE;
  channel manager emits ChannelClosed and NewChannel in response)
* (time passes)
* destroyed() (emits ::closed with :channel-destroyed = TRUE;
  channel manager emits ChannelClosed in response)

Another possibility:

* don't register(), don't emit_new_channel()
* (time passes)
* channel manager decides the channel should appear: now it
  calls reopened() and emit_new_channel(), and connects to ::closed.
  reopened() emits ::closed (which the CM isn't connected to yet, and
  is not copied onto D-Bus because the object was never registered), then
  does an equivalent of register() internally.
* (time passes)
* reopened() (emits ::closed with :channel-destroyed = FALSE;
  channel manager emits ChannelClosed and NewChannel in response)
* (time passes)
* destroyed() (emits ::closed with :channel-destroyed = TRUE;
  channel manager emits ChannelClosed in response)

3. MUC text channel is present within CM but not "officially" there yet, then is closed when it has never actually appeared on D-Bus

* don't register(), don't emit_new_channel()
* (time passes)
* destroyed() (sets :channel-destroyed = TRUE, but either does not
  emit ::closed, or does emit it but it never reaches D-Bus
  because register() was never called; if it does emit ::closed,
  the channel manager refrains from emitting ChannelClosed because
  tp_base_channel_is_registered() = FALSE)

I think this needs new behaviour, but perhaps no new API? It requires participation from the TpChannelManager, but so does the current interface, so, whatever.

(In reply to comment #0)
> GabbleMucChannel is a TpBaseChannel subclass. To support this "disappearing and
> reappearing" feature, some more functions will need to be added to
> TpBaseChannel:
> 
> > void tp_base_channel_unregister (TpBaseChannel *chan);
> 
> This removes the channel from the bus. It does not emit Close and does not set
> TpExportableChannel:channel-destroyed.

Is there actually a use-case for this, bearing in mind that a disappearing channel that had previously appeared should always emit Closed?

I think if anything, what you want is a way to emulate closing, not immediately respawn, unregister from D-Bus, but not be destroyed by the channel manager. So the two-way decision the channel manager currently makes when a channel closes:

* channel-destroyed?
  - if yes, free it
  - if no, respawn it

becomes a tristate:

* channel-destroyed?
  - if yes, free it
  - if no, channel-respawning? (new property, reopened() would set this TRUE)
    - if yes, emit NewChannel
    - if no, do nothing special

disappear() would set channel-destroyed = channel-respawning = FALSE, then emit ::closed (to get Closed and ChannelClosed), and only then unregister.

I'm not really sure that I see the use-case for disappear(), though?

The big use case I want this for is when a MUC text channel appears but only for a "sub-channel" (such as a Tube). This is what I want:

the setup:

   1. a DBusTube MUC channel is requested.
   2. gabble has to connect to the XMPP MUC and it does so.
   3. a GabbleMucChannel is created internally, but is not exposed or
      announced on the bus at all.
   4. a GabbleTubeDBus is created and the DBusTube channel is exposed
      and announced.

use case #1:

   a. a Text MUC channel (to the same MUC) is requested
   b. the GabbleMucChannel already created is exposed and announced
   (time passes)
   c. the Text channel has Close() called on it; it is Closed()
   d. the DBus tube channel is still open and in use
   e. hence the GabbleMucChannel hides again

use case #2:

   a. a message appears in the MUC sitting around hidden inside gabble
   b. the GabbleMucChannel is exposed and announced
   (time passes)
   c. the Text channel has Close() called on it; thus it is Closed() and
      disappears from the bus
   d. the DBus tube channel is still open and in use
   e. hence the GabbleMucChannel hides again

Then this can go around in a circle. Use cases #1 and #2 are actually the same path, but they are the cases in mind. So if I tried to implement this now with what's available in TpBaseChannel, this is what I would do:

the setup implementation:

    * create a GabbleMucChannel, don't register(), emit_new_channel()
    * create a GabbleTubeDBus in said muc channel,
      *do* register() and *do* emit_new_channel()

use case #1 and #2 implementation:

    * register() and emit_new_channel() on the existing GabbleMucChannel
    (time passes)
    * the text MUC has been closed; if we call destroyed() here, the
      channel manager will dispose of the MUC and its tubes. if we call
      reopened it'll "close" but then reappear... we don't want that
      either. we want to emit_closed(), emit_channel_closed() and
      unregister()


Does that make more sense as to why we need the text MUC to be able to disappear?

(In reply to comment #5)
>     * the text MUC has been closed; if we call destroyed() here, the
>       channel manager will dispose of the MUC and its tubes. if we call
>       reopened it'll "close" but then reappear... we don't want that
>       either. we want to emit_closed(), emit_channel_closed() and
>       unregister()

OK, I see why the existing API isn't enough for you. 

I'd prefer you to add a new disappear() with the semantics from Comment #4. Pseudocode:

void disappear (self)
{
  channel-destroyed = FALSE;
  channel-respawning = FALSE;
  emit closed;
  unregister ();
}

This means that any channel manager whose channels will call disappear() will have to expand from 2- to 3-case processing of the closed signal, as in Comment #4:

void on_closed (self, channel)
{
  emit ChannelsClosed([channel]);

  if (channel.is_destroyed())
      /* destroyed() must have been called */
      priv.channels.remove(channel);
  else if (channel.is_respawning())
      /* reopened() must have been called */
      emit NewChannels([channel]);
  else
      /* disappear() must have been called */
      /* do nothing special */
} 

void foreach_channel (self, callback)
{
  for channel in priv.channels
      if channel.is_registered()
          callback (channel);
}

Oh the other requirement is that when the channel RE-appears, it can have different Requested and Initiator{Handle,ID} values.

(In reply to comment #7)
> Oh the other requirement is that when the channel RE-appears, it can have
> different Requested and Initiator{Handle,ID} values.

If Requested is becoming FALSE, that's fine: tp_base_channel_reopened() takes a new initiator handle.

If Requested is becoming TRUE (which I suppose it could, in principle), a new method, void reopened_by_request(self), would probably be the easiest solution? (It would set the initiator to the global self-handle and Requested to TRUE.)

Or you could add a reopened_with_requested(self, requested, initiator) and deprecate reopened() if you'd prefer; or I suppose you could even make reopened() guess the new value of Requested from whether the initiator is the (global) self-handle.

For channels with a channel-specific self-handle (XMPP MUC) I think it's probably less horrible for UIs if the initiator is always the *global* self-handle in channels initiated by us. I will admit that I haven't checked whether Gabble currently makes that true, though.

Okay here's a patch. It seems to work with my WIP gabble branch.

I propose in next we replace the old reopened() with reopened_with_requested().

> + * // destoyed() must have been called; forget this channel

"destroyed"

reopened[_with_requested] needs to re-register the object on D-Bus immediately after emitting closed, I think:

  if (!self->priv->registered)
    tp_base_channel_register (self);

Otherwise, it can't implement any methods during its second life, and any signals that it emits during its second life will not reach D-Bus.

This is sufficiently subtle that I would like a regression test in telepathy-glib.

+ * channel_closed_cb (TpBaseChannel *chan,
+ * TpChannelManager *manager)
+ * {
+ * MyChannelManager *self = MY_CHANNEL_MANAGER (manager);
+ * TpHandle handle = tp_base_channel_get_target_handle (chan);
+ *
+ * tp_channel_manager_emit_channel_closed (manager,
+ * TP_EXPORTABLE_CHANNEL (chan));

I think this also emits ChannelClosed twice in this situation, which isn't hugely harmful but seems non-ideal:

disappear()
    -> emits closed
        -> dbus-glib handler emits Closed on D-Bus
        -> channel manager handler emits ChannelClosed on D-Bus
    -> unregisters from bus

reopened()
    -> emits closed
        -> no dbus-glib handler because it was unregistered and has
           not yet re-registered
        -> channel manager handler emits ChannelClosed on D-Bus again (!)
        -> channel manager emits NewChannel, NewChannels
    -> synchronously (so you can't miss any messages)
       re-registers on bus (when you implement that)

Two possible solutions, depending how much subtlety you're OK with piling onto the channel manager:

* require the channel manager to do:

    if (tp_base_channel_is_registered)
      tp_channel_manager_emit_channel_closed (...)

* make tp_channel_manager_emit_channel_closed check for
  TP_IS_BASE_CHANNEL and, if it is, suppress the signal

The former is more subtlety in CMs, the latter is arguably a layering violation.

For next, I'd welcome patches to move some of the sublety into the TpExportableChannel GInterface: perhaps it could grow channel-destroyed, channel-disappeared and channel-reopened signals, and TpChannelManager could become responsible for emitting the channel's D-Bus closed signal at the same time it emits ChannelClosed?

(In reply to comment #10)
> * make tp_channel_manager_emit_channel_closed check for
>   TP_IS_BASE_CHANNEL and, if it is, suppress the signal

... should be "check for TP_IS_BASE_CHANNEL (x) && !tp_base_channel_is_registered (x)" obviously.

(In reply to comment #10)
> > + * // destoyed() must have been called; forget this channel
> 
> "destroyed"

Fixed.

> reopened[_with_requested] needs to re-register the object on D-Bus immediately
> after emitting closed, I think:
> 
>   if (!self->priv->registered)
>     tp_base_channel_register (self);
> 
> Otherwise, it can't implement any methods during its second life, and any
> signals that it emits during its second life will not reach D-Bus.

Yes, I was previously relying on the CM calling register, but this is better.

> This is sufficiently subtle that I would like a regression test in
> telepathy-glib.

Okay I added a simple test. Is this what you were thinking?

> I think this also emits ChannelClosed twice in this situation, which isn't
> hugely harmful but seems non-ideal:
[...]
> Two possible solutions, depending how much subtlety you're OK with piling onto
> the channel manager:
> 
> * require the channel manager to do:
> 
>     if (tp_base_channel_is_registered)
>       tp_channel_manager_emit_channel_closed (...)
> 
> * make tp_channel_manager_emit_channel_closed check for
>   TP_IS_BASE_CHANNEL and, if it is, suppress the signal
> 
> The former is more subtlety in CMs, the latter is arguably a layering
> violation.

I recommended the CM do the former.

The implementation looks fine now, thanks.

(In reply to comment #12)
> > This is sufficiently subtle that I would like a regression test in
> > telepathy-glib.
> 
> Okay I added a simple test. Is this what you were thinking?

It's progress. I was really hoping for a channel manager that does exactly what you recommend in the docs, and a test verifying that we see the right NewChannels, Closed and ChannelClosed signals (and no more) - but I'd settle for a similar test in a Gabble branch, if that's easier? (I assume you have a Gabble branch somewhere that needs this for MUCs anyway?)

Perhaps tests/lib/simple-channel-manager would be a good place, although I notice it doesn't currently deal with channels closing at all. :-(

(In reply to comment #13)
> The implementation looks fine now, thanks.
> 
> (In reply to comment #12)
> > > This is sufficiently subtle that I would like a regression test in
> > > telepathy-glib.
> > 
> > Okay I added a simple test. Is this what you were thinking?
> 
> It's progress. I was really hoping for a channel manager that does exactly what
> you recommend in the docs, and a test verifying that we see the right
> NewChannels, Closed and ChannelClosed signals (and no more) - but I'd settle
> for a similar test in a Gabble branch, if that's easier? (I assume you have a
> Gabble branch somewhere that needs this for MUCs anyway?)

Yes, see bug #32612 and bug #50828.

There are tests in the top commits.

Looks good; required for Bug #36212.

le merged to master. merci pour le review.