The properties CallState and CallFlags are confusing, especially considering the CallState interface.. Why are the CallFlags flags, most of them are mutually exclusive and are actually states? Adding Pekka in CC as he knows the GSM bits.
Locally_Ringing, Queued, Locally_Held, In_Progress, Clearing are distinct states in Q.931/GSM/3G. Locally_Ringing, Queued, In_Progress, Clearing are states in SIP.
(In reply to comment #0) > The properties CallState and CallFlags are confusing, especially considering > the CallState interface.. > I've done a bit of git-blame, and discovered the following: The CallState interface was copied wholesale into an early alpha version of Call and then forked (thanks Sjoerd). Flags used to be per contact, but they were changed to be for the whole call. The Call_Member_Flags enum was added to represent remote contacts. > Why are the CallFlags flags, most of them are mutually exclusive and are > actually states? > They are flags partly because of conflated design and partly to allow for future extensibility. The things that are actually sub-states of CallState are Setup_In_Progress, Queued, Ringing (renamed from In_Progress and Locally_Ringing; substates of Pending_Receiver) and Clearing (substate of Ended). I think that these should be merged into the main state enum. I will mark sub-states as such, so that implementers can map them appropriately if they don't care. The things that are genuinely flags are Forwarded, Locally_Muted, and Locally_Held. I think that mute and hold could be represented by their own properties on their own interfaces. Forwarded is an informational flag that's controlled by the remote end. In a way it reminds me of #24901, but I guess it can also be used for hunt groups, so I'm in favour of keeping this flag. Since we're keeping one flag, we might as well keep Mute and Hold as their own flags, and the general extension mechanism of flags. (In reply to comment #1) > Locally_Ringing, Queued, Locally_Held, In_Progress, Clearing are distinct > states in Q.931/GSM/3G. > > Locally_Ringing, Queued, In_Progress, Clearing are states in SIP. I was doing a bit of googling for SIP/XMPP state machines, and found: http://tools.ietf.org/html/rfc3976#section-5 . Cross-checking to make sure I have everything that the UI might care about, I think that we should have a separate "Active" (corresponding to the ones listed here) meaning "at least one other party can hear you, and you can hear everything they're sending you". I'd suggest that Hold, Mute and connectivity problems should make the state go from Active back to Accepted, so even retarded UIs can display some useful feedback to the user without looking at Flags at all. tl;dr: add Setup_In_Progress, Queued, Ringing, Active and Clearing to CallState (in addition to Unknown, Pending_Initiator, Pending_Receiver, Accepted, and Ended). CallFlags will only contain Forwarded, Locally_Muted, and Locally_Held. If there are no objections, I will write this up.
Sounds good to me.
http://git.collabora.co.uk/?p=user/alsuren/telepathy-spec.git;a=shortlog;h=refs/heads/states (is 3 useful commits. If you want to review them independently you can, or you can review them as part of 35573)
As I said on IRC, I'm not sure what the place of Pending_Receiver is. It seems ot just be the sum of all its substates.. And the substates exist in SIP/GSM/etc, while Pending_Receiver is just a Telepathy invention.
Okay: let's try again... http://cgit.collabora.com/git/user/alsuren/telepathy-spec.git/log/?h=states-35128
"Codecs Incompatible" vs "CodecsIncompatible" or is that normal ? And same for other errors.xml changes. Anyway, the html is not generated correctly here when you refer to them. Alerting vs SetRinging .. Inconsistent naming, Why did you rename Ringing to Alerting anyway ? I dislike Streaming_Error (sounds like Media Error), you really mean "Connection_Failed_Error"
There is a link from to Media.UnsupportedType and InsufficientFunds errors, but they are not defined.
(In reply to comment #7) > "Codecs Incompatible" vs "CodecsIncompatible" or is that normal ? And same for > other errors.xml changes. Anyway, the html is not generated correctly here when > you refer to them. > Good catch. Tell my when you find a fix for that. > Alerting vs SetRinging .. Inconsistent naming, Why did you rename Ringing to > Alerting anyway ? It was to a) make it so that the CallFlags and MemberFlags' Ringing members the place to tell whether you're ringing or not if you care and b) copy-pasta from rfc3976#section-5. I think you're right though: Ringing is probably more natural. > > I dislike Streaming_Error (sounds like Media Error), you really mean > "Connection_Failed_Error" Streaming Error is better than Connection_Failed_Error. There is a tp:error with the name ConnectionFailed already. It is different from MediaError for those technically minded enough to care. If we want to avoid the name collision, we could rename MediaError to CodecError or something?
Some updates: http://cgit.collabora.com/git/user/tester/telepathy-spec.git/log/?h=states-35128
Updated again, the only missing bit is that UnsupportedType is not defined anywhere as an error.
(In reply to comment #11) > Updated again, the only missing bit is that UnsupportedType is not defined > anywhere as an error. Your changes look good. I added UnsupportedType in: http://cgit.collabora.com/git/user/alsuren/telepathy-spec.git/log/?h=states-35128 Also, I realised that my original states-35128 branch was based off the wrong thing and rebased it onto alsuren/states. It should still merge into alsuren/call and build fine.
++ ship it !
Merged to alsuren/call.
merged to master
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.