Bug 24935

Summary:	Emit StreamError(..., Codec_Negotiation_Failed, ..) in more cases
Product:	Telepathy	Reporter:	Will Thompson <will>
Component:	gabble	Assignee:	Eitan Isaacson <eitan.isaacson>
Status:	RESOLVED FIXED	QA Contact:	Telepathy bugs list <telepathy-bugs>
Severity:	normal
Priority:	medium
Version:	unspecified
Hardware:	Other
OS:	All
URL:	http://git.collabora.co.uk/?p=user/eitan/telepathy-gabble.git;a=shortlog;h=refs/heads/more-stream-errors
Whiteboard:
i915 platform:		i915 features:

Description Will Thompson 2009-11-05 02:16:25 UTC

If you try to add video to an existing audio call, but I don't support any of the codecs you propose, TpFs calls Error(Codec_Negotiation_Failed, ...) on the stream, and my Gabble correctly emits StreamError(..., Codec_Negotiation_Failed, ...) for that stream. However, my Gabble just sends you a content-remove with no reason.

Firstly, it should send a content-reject.

Secondly, as per <http://mail.jabber.org/pipermail/jingle/2009-November/001112.html> it should include <reason><failed-application/></reason> in the content-reject.

Thirdly, it should also include <failed-application/> when terminating a session because none of the codecs in the initiate were supported.

Fourthly, it should react to session-terminate with <failed-application/> by emitting StreamError(..., Codec_Negotiation_Failed, ...) for each stream before ending the call.

This would allow the UI to tell the user what happened in more cases than it currently can.

Comment 1 Eitan Isaacson 2010-12-03 16:14:18 UTC

This branch has all the aforementioned behavior implemented. Empathy is happy.

I will make sure all tests pass and write new tests tomorrow.

Comment 2 Eitan Isaacson 2010-12-04 22:50:36 UTC

OK! Passes all tests. Still need to actually write tests for the added features, but the actual code is ready for review.

Comment 3 Eitan Isaacson 2010-12-06 16:36:08 UTC

OK! Tests added. They might be a tad excessive. I started adding assertions to existing tests, that might be the way to go.

Comment 4 Vivek Dasmohapatra 2010-12-07 09:30:16 UTC

trivia (don't have to be fixed for a merge):

http://git.collabora.co.uk/?p=user/eitan/telepathy-gabble.git;a=commitdiff;h=ace3ba397ca79e4b78a2cf211fd89079f2142a3a

misc indentation change in first chunk

--

http://git.collabora.co.uk/?p=user/eitan/telepathy-gabble.git;a=commitdiff;h=ce3d6bec79e2db2f8e225e5af3881e74cc33a405

can use g_str_equal rather then !tp_strdiff since txt is guaranteed not-NULL

----

trivia:

switch (…) {

not sure these switch()es are in house style, but check up on that,
I think the { should be on the next line

----

Comment 5 Eitan Isaacson 2010-12-08 11:34:18 UTC

(In reply to comment #4)
> trivia (don't have to be fixed for a merge):
> 
> http://git.collabora.co.uk/?p=user/eitan/telepathy-gabble.git;a=commitdiff;h=ace3ba397ca79e4b78a2cf211fd89079f2142a3a
> 
> misc indentation change in first chunk

Fixed styling, e5c82a9

> 
> --
> 
> http://git.collabora.co.uk/?p=user/eitan/telepathy-gabble.git;a=commitdiff;h=ce3d6bec79e2db2f8e225e5af3881e74cc33a405
> 
> can use g_str_equal rather then !tp_strdiff since txt is guaranteed not-NULL
> 

Using a GEnum mapping instead, as peeps said on #telepathy, 0dea816

> ----
> 
> trivia:
> 
> switch (…) {
> 
> not sure these switch()es are in house style, but check up on that,
> I think the { should be on the next line

Yeah, you are right about it. e5c82a9

Comment 6 Eitan Isaacson 2010-12-10 13:34:53 UTC

Vivek gives his thumbs up. Merged.

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.