Bug 38986 (Call2)

(In reply to comment #0)
> I have renamed everything to DRAFT2. Do I also need to go around and do
> <tp:added/> and <tp:changed/>, or can we just do tp:added when we undraft?
> (tp:added is probably not that much work. tp:changed would be a lot of work,
> because so much has changed since DRAFT 1)

As said on IRC: I would not bother. Maybe have a vague summary of changes in the NEWS? And I'd drop the DRAFT from the name and just version the interface name directly.

Okay, so Call becomes the guinea-pig for versioned dbus interfaces. This is going to be fun.

I have made the name change from Call.DRAFT2 to Call1 and pushed to alsuren/call

When porting telepathy-yell to this, I noticed that we ended up with function names like "tpy_svc_channel_type_call1_implement_hangup", which requires a huge sed of all of our source code. To me, this is unacceptable.

I was talking to ocrete and he suggested that we should produce headers with the version name of the supported interfaces (e.g. <telepathy-glib/svc-channel-type-call-2.h> and make CM-authors include these instead)
This way, when we bump the version number:
+ CM authors get an easy-to-understand compilation error that the header no longer exists
+ No sedding is required, so git blame still works, and there will be fewer spurious merge conflicts when backporting.
- We need to change makefiles when we change a version number (but changing a version number is a rare thing, so that's okay)


Note that the existing (unversioned) svc-*.h files will still exist for interfaces that haven't been version-bumped yet, and we can version-bump our interfaces one at a time if we want.

We will still need to upgrade things in lockstep though, and we still need to have a mechanism (probably in MC) for:
* "Hey user: The version of [channel.type.call, channel.type.text] supported by the newly-installed [empathy-call, empathy-chat] is newer than that supported by the currently-running [gabble, salut, ...]. Click this button to restart these services. This will cause you to be disconnected from the network briefly"
* "Hey empathy-chat. A new version of you has appeared in my list of activatible services. It supports a newer version of channel.interface.messages (required for chatting with the following accounts: [...]). Can you warn your user that you need to restart, and then do so please?

Should I open a separate bug for this?

For the updating, I would just do a big "Restart everything" thing instead of trying to be smart and do it fine grained.

Just pushed another change to my call branch. The last 4 commits require review.

Note that when porting to tp-glib, I had to strip the causes-havoc tags temporarily, but that's just a code generation thing that I will fix before I ship my tp-glib code changes.

(In reply to comment #2)
> We will still need to upgrade things in lockstep though, and we still need to
> have a mechanism (probably in MC) for:
> * "Hey user: The version of [channel.type.call, channel.type.text] supported by
> the newly-installed [empathy-call, empathy-chat] is newer than that supported
> by the currently-running [gabble, salut, ...]. Click this button to restart
> these services. This will cause you to be disconnected from the network
> briefly"
> * "Hey empathy-chat. A new version of you has appeared in my list of
> activatible services. It supports a newer version of channel.interface.messages
> (required for chatting with the following accounts: [...]). Can you warn your
> user that you need to restart, and then do so please?

(In reply to comment #3)
> For the updating, I would just do a big "Restart everything" thing instead of
> trying to be smart and do it fine grained.

I think the only viable way is logging out and in of your desktop session, really. We could add a way to have (say) MC tell all your CMs and clients to die/reload themselves … and some clients like Gnome Shell can restart itself without losing too much state, but the user might be a bit annoyed if they upgrade Telepathy and AbiWord (say) commits seppuku in sympathy.

So I don't think it's entirely obvious how to solve this—but note that it's not entirely a new problem, as we already have issues with MC not noticing new CMs (bug 30210 and bug 24511). Definitely separate bug territory, and maybe should only strike end-users on distro version upgrades, which we can assume will involve logging out and in (or maybe a reboot)?

From poking at the tp-glib codegen, it looks like we could put <xi:include/> directives for all the Call-related interfaces into telepathy-glib/call-1.xml, and this would generate tp-svc-call-1.[ch] and tp-cli-call-1{,-body,-gtk-doc}.h as David and Olivier suggest. So yay, no codegen changes needed for that.

I have fixed some nitpicks on the spec XML in <http://cgit.collabora.com/git/user/wjt/telepathy-spec-wjt.git/log/?h=call>. Here are some others:

+    <method name="Reject" tp:name-for-bindings="Reject">
+      <tp:docstring>
+        Reject the proposed update to the remote description
+        FIXME add error codes and strings here
+      </tp:docstring>
+    </method>

Here lies a FIXME.

+    <tp:enum name="Local_Mute_State" type="u">
+      <tp:docstring>
+        The mute state of a channel.
+      </tp:docstring>

This should presumably say “channel, content or stream”. Ditto some of the other docs here?

+        to <tp:type>Call_State</tp:type>_Initialising, which signifies
+        that the call is waiting for the network to do
+        something (When the CM has information about when the remote contact is
+        notified about the call (or the network is known not to convey such
+        information) it should also change to
+        <tp:type>Call_State</tp:type>_Ringing. All changes to
+        <tp:member-ref>CallState</tp:member-ref> property are signalled using
+        the <tp:member-ref>CallStateChanged</tp:member-ref> signal.</p>

I think the first " (W" should be ". W"?

Your changes all looked good, so I based my revised http://cgit.collabora.com/git/user/alsuren/telepathy-spec.git/log/?h=call on it. Hopefully, we can tag-team this until we get it merged?

(note that I'm in theory on holiday on Friday and Monday, but I will probably be hacking on the road).


> Here lies a FIXME.

Thanks. Fixed.
> 
> +    <tp:enum name="Local_Mute_State" type="u">
> +      <tp:docstring>
> +        The mute state of a channel.
> +      </tp:docstring>
> 
> This should presumably say “channel, content or stream”. Ditto some of the
> other docs here?
> 
Yeah: I re-worked that section

> +        to <tp:type>Call_State</tp:type>_Initialising, which signifies
> +        that the call is waiting for the network to do
> +        something (When the CM has information about when the remote contact
> is
> +        notified about the call (or the network is known not to convey such
> +        information) it should also change to
> +        <tp:type>Call_State</tp:type>_Ringing. All changes to
> +        <tp:member-ref>CallState</tp:member-ref> property are signalled using
> +        the <tp:member-ref>CallStateChanged</tp:member-ref> signal.</p>
> 
> I think the first " (W" should be ". W"?

Correct, and there were other things wrong with that paragraph, so I changed it.

I also had a strong suspicion that many references to tp:enumvalues were wrong, so I went on a yak-shave and made it possible to check for them. If you'd rather I didn't add the extra xml element type (or have better names/semantics) I'd be happy to re-work those commits and/or delete them and only ship the fixes for the problems they reveal.

(In reply to comment #8)
> Your changes all looked good, so I based my revised
> http://cgit.collabora.com/git/user/alsuren/telepathy-spec.git/log/?h=call on
> it. Hopefully, we can tag-team this until we get it merged?

Sounds good. I pushed a couple more fixes.

> > Here lies a FIXME.
> 
> Thanks. Fixed.

+      <arg name="Reason" type="(uuss)" tp:type="Call_State_Reason"
+        direction="out">
+        <tp:docstring>
+          A structured reason for the rejection.
+        </tp:docstring>
+      </arg>

The actor is always going to be us, and the reason will always be Media_Error presumably? I'm just wondering whether this should just be a D-Bus error plus message string—if this is a case of “the whole Call API uses this struct consistently” then no problem.

Heh, I didn't notice the over-zealous sedding. Whoops, sorry.

> I also had a strong suspicion that many references to tp:enumvalues were wrong,
> so I went on a yak-shave and made it possible to check for them. If you'd
> rather I didn't add the extra xml element type (or have better names/semantics)
> I'd be happy to re-work those commits and/or delete them and only ship the
> fixes for the problems they reveal.

I really like the revised style where the contents of <tp:value-ref> is always what's displayed in the document, and fully endorse your implementation. I've pushed a couple of improvements to the text interface's markup (since the sed touched that, and I noticed it would look wrong).

> > > Here lies a FIXME.
> > 
> > Thanks. Fixed.
> 
> +      <arg name="Reason" type="(uuss)" tp:type="Call_State_Reason"
> +        direction="out">
> +        <tp:docstring>
> +          A structured reason for the rejection.
> +        </tp:docstring>
> +      </arg>
> 
> The actor is always going to be us, and the reason will always be Media_Error
> presumably? I'm just wondering whether this should just be a D-Bus error plus
> message string—if this is a case of “the whole Call API uses this struct
> consistently” then no problem.

It is indeed a consistency thing.


> > I also had a strong suspicion that many references to tp:enumvalues were wrong,
> > so I went on a yak-shave and made it possible to check for them. If you'd
> > rather I didn't add the extra xml element type (or have better names/semantics)
> > I'd be happy to re-work those commits and/or delete them and only ship the
> > fixes for the problems they reveal.
> 
> I really like the revised style where the contents of <tp:value-ref> is always
> what's displayed in the document, and fully endorse your implementation. I've
> pushed a couple of improvements to the text interface's markup (since the sed
> touched that, and I noticed it would look wrong).

I think your changes look good. Do you want to ship it, or should I do so tomorrow?

has been merged a while ago