[PATCH] voicecall: send CallRemoved for calls when voicecall atom is unregistered

[PATCH] voicecall: send CallRemoved for calls when voicecall atom is unregistered

[Mike Brudevold]

[-- Attachment #1: Type: text/plain, Size: 906 bytes --]

From: Michael Brudevold <michael.brudevold@logicpd.com>

The CallRemoved signal was not being sent for existing calls when the voicecall
atom was unregistered. An example of this is the lack of a CallRemoved signal
when an HFP AG is disconnected due to SLC timeout while a call is active.
---
 src/voicecall.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/src/voicecall.c b/src/voicecall.c
index 104b600..263f1ee 100644
--- a/src/voicecall.c
+++ b/src/voicecall.c
@@ -2699,8 +2699,11 @@ static void voicecall_unregister(struct ofono_atom *atom)
 	if (vc->dial_req)
 		dial_request_finish(vc);
 
-	for (l = vc->call_list; l; l = l->next)
+	for (l = vc->call_list; l; l = l->next) {
+		voicecalls_emit_call_removed(vc, l->data);
+
 		voicecall_dbus_unregister(vc, l->data);
+	}
 
 	g_slist_free(vc->call_list);
 	vc->call_list = NULL;
-- 
1.7.5.4

Re: [PATCH] voicecall: send CallRemoved for calls when voicecall atom is unregistered

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1354 bytes --]

Hi Mike,

On 04/18/2012 05:02 PM, Mike Brudevold wrote:
> From: Michael Brudevold <michael.brudevold@logicpd.com>
> 
> The CallRemoved signal was not being sent for existing calls when the voicecall
> atom was unregistered. An example of this is the lack of a CallRemoved signal
> when an HFP AG is disconnected due to SLC timeout while a call is active.
> ---
>  src/voicecall.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/src/voicecall.c b/src/voicecall.c
> index 104b600..263f1ee 100644
> --- a/src/voicecall.c
> +++ b/src/voicecall.c
> @@ -2699,8 +2699,11 @@ static void voicecall_unregister(struct ofono_atom *atom)
>  	if (vc->dial_req)
>  		dial_request_finish(vc);
>  
> -	for (l = vc->call_list; l; l = l->next)
> +	for (l = vc->call_list; l; l = l->next) {
> +		voicecalls_emit_call_removed(vc, l->data);
> +
>  		voicecall_dbus_unregister(vc, l->data);
> +	}
>  

In some respect this is on purpose, the application should listen to the
PropertyChanged on the main Modem object and keep track of Interfaces
property.  e.g. if the VoiceCallManager goes away the application can
pretty much assume that all calls are gone.

I could use some more convincing that this is really needed...

Regards,
-Denis

>  	g_slist_free(vc->call_list);
>  	vc->call_list = NULL;

Re: [PATCH] voicecall: send CallRemoved for calls when voicecall atom is unregistered

[Mike]

[-- Attachment #1: Type: text/plain, Size: 2031 bytes --]

Hi Denis,

On Wed, Apr 18, 2012 at 10:52 AM, Denis Kenzior <denkenz@gmail.com> wrote:
> Hi Mike,
>
> On 04/18/2012 05:02 PM, Mike Brudevold wrote:
>> From: Michael Brudevold <michael.brudevold@logicpd.com>
>>
>> The CallRemoved signal was not being sent for existing calls when the voicecall
>> atom was unregistered. An example of this is the lack of a CallRemoved signal
>> when an HFP AG is disconnected due to SLC timeout while a call is active.
>> ---
>>  src/voicecall.c |    5 ++++-
>>  1 files changed, 4 insertions(+), 1 deletions(-)
>>
>> diff --git a/src/voicecall.c b/src/voicecall.c
>> index 104b600..263f1ee 100644
>> --- a/src/voicecall.c
>> +++ b/src/voicecall.c
>> @@ -2699,8 +2699,11 @@ static void voicecall_unregister(struct ofono_atom *atom)
>>       if (vc->dial_req)
>>               dial_request_finish(vc);
>>
>> -     for (l = vc->call_list; l; l = l->next)
>> +     for (l = vc->call_list; l; l = l->next) {
>> +             voicecalls_emit_call_removed(vc, l->data);
>> +
>>               voicecall_dbus_unregister(vc, l->data);
>> +     }
>>
>
> In some respect this is on purpose, the application should listen to the
> PropertyChanged on the main Modem object and keep track of Interfaces
> property.  e.g. if the VoiceCallManager goes away the application can
> pretty much assume that all calls are gone.
>
> I could use some more convincing that this is really needed...

The only reason I added it is 4 hours of debugging my GUI and ofono to
figure out why the GUI didn't notice the D-Bus path for the voice call
was no longer around.  The documentation for CallRemoved says "Signal
that is sent when a voice call has been released."  This implies it
should be sent every time it is released.  If you update the
documentation to make it clear that this is not always the case, then
it's fine by me.

Mike

>
> Regards,
> -Denis
>
>>       g_slist_free(vc->call_list);
>>       vc->call_list = NULL;
>

Re: [PATCH] voicecall: send CallRemoved for calls when voicecall atom is unregistered

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 555 bytes --]

Hi Mike,

> 
> The only reason I added it is 4 hours of debugging my GUI and ofono to
> figure out why the GUI didn't notice the D-Bus path for the voice call
> was no longer around.  The documentation for CallRemoved says "Signal
> that is sent when a voice call has been released."  This implies it
> should be sent every time it is released.  If you update the
> documentation to make it clear that this is not always the case, then
> it's fine by me.
> 

Please check that commit 7f51df4d satisfies your requirements.

Regards,
-Denis

Re: [PATCH] voicecall: send CallRemoved for calls when voicecall atom is unregistered

[Mike]

[-- Attachment #1: Type: text/plain, Size: 697 bytes --]

Hi Denis,

On Thu, Apr 19, 2012 at 9:02 AM, Denis Kenzior <denkenz@gmail.com> wrote:
> Hi Mike,
>
>>
>> The only reason I added it is 4 hours of debugging my GUI and ofono to
>> figure out why the GUI didn't notice the D-Bus path for the voice call
>> was no longer around.  The documentation for CallRemoved says "Signal
>> that is sent when a voice call has been released."  This implies it
>> should be sent every time it is released.  If you update the
>> documentation to make it clear that this is not always the case, then
>> it's fine by me.
>>
>
> Please check that commit 7f51df4d satisfies your requirements.

Looks good, thanks!

Mike

>
> Regards,
> -Denis