Bug 24768

Had a thought today. This could simply be a signal on the Observer interface that can Observer can emit, that way the CD can know the emitters path and have a look at the appropriate filter before calling ObserveChannels.

(In reply to comment #1)
> Had a thought today. This could simply be a signal on the Observer interface
> that can Observer can emit, that way the CD can know the emitters path and have
> a look at the appropriate filter before calling ObserveChannels.

I don't think it can be: how can the observer know that MissionControl is ready to receive its signals?

12:55 < smcv> I think this is actually a special case of the "please restart 
              me" flag
12:56 < danni> smcv: you may not have been restarted of course, you may have 
               been started for the first time
12:56 < smcv> PleaseRestartMe && activatable -> (re)start the observer whenever 
              a channel it's interested in appears, or whenever it exits while 
              such a channel exists
12:56 < danni> and want to know what's happening
12:56 < smcv> PleaseRestartMe && !activatable -> whenever the observer pops up, 
              call ObserveChannels for each channel it would have been 
              interested in
12:57 < smcv> in any case, someone (ptlo?) is working on the former case, which 
              makes the latter case considerably simpler to implement even if 
              it ends up not being the same API

PleaseRestartMe is Bug #24376.

If we implement this by calling ObserveChannels() again, there ought to be something in Observer_Info ({'catch-up': True} perhaps?) to indicate that they're not really new channels.

Branch for implementing this:

http://git.collabora.co.uk/?p=user/ptlo/tp-spec-senko/.git;a=shortlog;h=refs/heads/respawnable-observers

+        <p>If true, upon the startup of this observer, ObserveChannels will be
+        called for every already existing channel that this observer wants to
+        observe (ie. every channel that would cause ObserveChannels to be called
+        on this observer upon creation).</p>

Insert a blank line between this paragraph and the next, please.

+        <p>If the observer is an activatable client, ObserveChannels won't be
+        emitted after the client is ready. Instead, it will be omitted immediately
+        when the observer's well-known name disappears from the bus, so the client
+        will be restarted immediately via DBus service activation.</p>

Methods are "called", not "emitted". They're certainly not "omitted", which is something else entirely :-)

Also, I think this wording is a bit confusing. How about this?

         <p>If the Observer is service-activatable, instead of watching
           passively for a new instance of it to start, the channel
           dispatcher will call its ObserveChannels method as soon as
           its well-known name disappears, resulting in it being restarted
           immediately via D-Bus service activation.</p>

... and I'd like some rationale, like this:

         <tp:rationale>
           <p>This means that if an activatable Observer crashes, it will
             be restarted as soon as possible; while there is an unavoidable
             possibility that it will miss some events during this process
             (particularly <tp:dbus-ref 
               namespace="org.freedesktop.Channel.Type">Text</tp:dbus-ref>
             messages), this window of event loss is kept to a minimum.</p>

           <p>Non-activatable observers can't take advantage of this
             mechanism, but setting this property on a non-activatable
             observer does allow it to "catch up" on channels that are
             currently active at the time that it starts up.</p>
         </tp:rationale>

>        <p>When the ObserveChannels method is called due to observer recovery,
>        the "Observer_Info" dictionary will contain one extra item with key
>        "ExistingChannel" and boolean value of True.</p>

Our case convention for "extra misc info" dicts is usually like GObject properties (so, "existing-channel" in this case).

I think this key should be called "recovering", actually.

The Observer_Info parameter to ObserveChannels should document it, too. Pseudocode, in which /foo/ indicates a hyperlink:

    recovering : b
        If present and TRUE, this channel already existed before the observer
        started, and is being supplied to it due to the /Recover/ property
        (see that property for details).

>        <tp:rationale>
>          <p>The observers might hold state or do channel-specific initialisation
>          for the channels they observe. This is a way of signalling to them
>          that they were already observing this channel, so they don't have to
>          do extra round-trips to query for this information.</p>

I think this rationale is wrong. If you receive a channel with 'recovering', then by definition you've just started up, so you have no state!

Instead, the rationale for the "recovering" key should be something like:

    This allows observers to distinguish between new channels (the normal
    case), and existing channels that were given to the observer in order
    to catch up on previous events (perhaps after a previous instance of
    the same observer crashed).

Do you have an implementation of this in MC? I'll clone this bug for the implementation; please attach any implementation for review to the clone, with the patch keyword.

This spec change can potentially be merged without an implementation in MC, but can't be undrafted until we have a suitable implementation. To be honest, since this branch is so small, I'd be inclined to implement it in MC based on this draft, and not bother to merge it to the spec until we have a reasonable implementation on a branch, at which point it can go straight in as stable API.

When this is ready for review again, please remove review- from the status whiteboard and re-add the patch keyword.

*** Bug 24376 has been marked as a duplicate of this bug. ***

12:33 < ptlo> 12:22:50> smcv, wrt fd.o comment #24768, I didn't document the 
              recover flag in Observer_Info doc because that's in 
              Client.Observer (which AIUI is not to be touched with unstable 
              features), not Client.Observer.FUTURE; it didn't seem right to 
              copy/paste the entire ObserveChannels/Observer_Info doc into 
              .FUTURE just to be able to add the flag there

That's fair enough, but please provide (either on this bug, or somewhere in the branch - e.g. text headed <strong>Text to be added to Observer_Info</strong> in the FUTURE XML, or something) the text that will be *added* to Observer_Info, so that undrafting this feature is just a matter of moving text around.

(In reply to comment #9)
> 12:33 < ptlo> 12:22:50> smcv, wrt fd.o comment #24768, I didn't document the 
>               recover flag in Observer_Info doc because that's in 
>               Client.Observer (which AIUI is not to be touched with unstable 
>               features), not Client.Observer.FUTURE; it didn't seem right to 
>               copy/paste the entire ObserveChannels/Observer_Info doc into 
>               .FUTURE just to be able to add the flag there
> 
> That's fair enough, but please provide (either on this bug, or somewhere in the
> branch - e.g. text headed <strong>Text to be added to Observer_Info</strong> in
> the FUTURE XML, or something) the text that will be *added* to Observer_Info,
> so that undrafting this feature is just a matter of moving text around.

Changed according to review comments, squashed into a single commit:

http://git.collabora.co.uk/?p=user/ptlo/tp-spec-senko/.git;a=commitdiff;h=9c0ab8aa46108d63980192e9cd955e8a9f99d2ff

(In reply to comment #10)
> http://git.collabora.co.uk/?p=user/ptlo/tp-spec-senko/.git;a=commitdiff;h=9c0ab8aa46108d63980192e9cd955e8a9f99d2ff

"MailNotification: UnreadMailCount >= UnreadMails array size"? I don't think that's the patch you meant :-)

Assuming that <http://git.collabora.co.uk/?p=user/ptlo/tp-spec-senko/.git;a=shortlog;h=refs/heads/respawnable-observers> is what you meant me to review, I approve.

However, since this is such a small spec change, it may be easier to not bother merging it as a draft - we could probably just insta-undraft it, if there's a good implementation already, so I'll review the MC implementation of this feature next.

The second try at an implementation over on Bug #27309 is good enough to reassure me that this API is implementable, so I think we can go directly to an undrafted version of this.

(In reply to comment #12)
> The second try at an implementation over on Bug #27309 is good enough to
> reassure me that this API is implementable, so I think we can go directly to an
> undrafted version of this.

Here's the undrafted version for instareview:

http://git.collabora.co.uk/?p=user/ptlo/tp-spec-senko/.git;a=commitdiff;h=871718924be6e3bcaf4ffb7f0e7aafe75462b245

The undrafted version looks good to me, but please don't merge without spec-meeting approval. Thanks for getting this working!

Spec cabal approves, I'll merge it for the next spec release.

Stable API in 0.19.4.