I would like to land my call branch to master this week. All of the recent merges have been reviewed by ocrete, but I would like to get some input from at least one other spec cabal person before I merge (it is a huge change). http://people.freedesktop.org/~alsuren/telepathy-spec-call/spec/ is the html version. http://cgit.collabora.com/git/user/alsuren/telepathy-spec.git/log/?h=call is the git repo. Open questions: 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)
(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
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.