Re: [RFC PATCH 2/3] voicecall: add SSN handling functions

[Andras Domokos <andras.domokos@nokia.com>]

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

Hi Denis,

On 03/03/2011 10:03 PM, ext Denis Kenzior wrote:
> Hi Andras,
>
> On 03/03/2011 10:48 AM, Andras Domokos wrote:
>> ---
>>   include/types.h     |    2 +
>>   include/voicecall.h |    6 ++
>>   src/voicecall.c     |  156 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 164 insertions(+), 0 deletions(-)
>>
> Please make sure to split this patch into several in accordance to our
> patch submission guidelines.  See HACKING document, specifically the
> 'Submitting Patches' section.

OK, I'll split up the patch according to the HACKING principles.

>> diff --git a/include/types.h b/include/types.h
>> index d25f409..b639c8a 100644
>> --- a/include/types.h
>> +++ b/include/types.h
>> @@ -96,6 +96,8 @@ struct ofono_call {
>>   	char name[OFONO_MAX_CALLER_NAME_LENGTH + 1];
>>   	int clip_validity;
>>   	int cnap_validity;
>> +	ofono_bool_t remote_held;
>> +	ofono_bool_t remote_multiparty;
> I really don't like these being here.  Lets put them onto the struct
> voicecall object in src/voicecall.c.  The logic for setting remote_held
> and remote_multiparty is something that belongs in the core and should
> not be exposed to the driver.

You are right, this informations have little to do with the drivers,
makes more sense to have them in the voicecall struct.

>>   };
>>
>>   struct ofono_network_time {
>> diff --git a/include/voicecall.h b/include/voicecall.h
>> index f00eb08..5e6da02 100644
>> --- a/include/voicecall.h
>> +++ b/include/voicecall.h
>> @@ -160,6 +160,12 @@ void ofono_voicecall_set_data(struct ofono_voicecall *vc, void *data);
>>   void *ofono_voicecall_get_data(struct ofono_voicecall *vc);
>>   int ofono_voicecall_get_next_callid(struct ofono_voicecall *vc);
>>
>> +void ofono_voicecall_ssn_mo_notify(struct ofono_voicecall *vc, unsigned int id,
>> +					int code, int index);
>> +void ofono_voicecall_ssn_mt_notify(struct ofono_voicecall *vc, unsigned int id,
>> +					int code, int index,
>> +					const struct ofono_phone_number *ph);
>> +
> Looks good, but as I mentioned this should be a separate patch.
>

I'll create a separate patch for each top level directory.

>>   #ifdef __cplusplus
>>   }
>>   #endif
>> diff --git a/src/voicecall.c b/src/voicecall.c
>> index ec001c0..e5936f5 100644
>> --- a/src/voicecall.c
>> +++ b/src/voicecall.c
>> @@ -400,6 +400,12 @@ static void append_voicecall_properties(struct voicecall *v,
>>
>>   	ofono_dbus_dict_append(dict, "Multiparty", DBUS_TYPE_BOOLEAN,&mpty);
>>
>> +	ofono_dbus_dict_append(dict, "RemoteHeld", DBUS_TYPE_BOOLEAN,
>> +				&call->remote_held);
>> +
>> +	ofono_dbus_dict_append(dict, "RemoteMultiparty", DBUS_TYPE_BOOLEAN,
>> +				&call->remote_multiparty);
>> +
>>   	if (v->message)
>>   		ofono_dbus_dict_append(dict, "Information",
>>   						DBUS_TYPE_STRING,&v->message);
>> @@ -1869,6 +1875,8 @@ static GDBusMethodTable manager_methods[] = {
>>   };
>>
>>   static GDBusSignalTable manager_signals[] = {
>> +	{ "Forwarded",	 	 "s" },
>> +	{ "BarringActive",	 "s" },
>>   	{ "PropertyChanged",	"sv" },
>>   	{ "CallAdded",		"oa{sv}" },
>>   	{ "CallRemoved",	"o" },
>> @@ -2684,3 +2692,151 @@ void __ofono_voicecall_tone_cancel(struct ofono_voicecall *vc, int id)
>>   		tone_request_run(vc);
>>   	}
>>   }
>> +
>> +static void ssn_mt_forwarded_notify(struct ofono_voicecall *vc,
>> +					unsigned int id, int code,
>> +					const struct ofono_phone_number *ph)
>> +{
>> +	DBusConnection *conn = ofono_dbus_get_connection();
>> +	const char *path = __ofono_atom_get_path(vc->atom);
>> +	char *info = "incoming";
>> +
>> +	g_dbus_emit_signal(conn, path, OFONO_VOICECALL_MANAGER_INTERFACE,
>> +				"Forwarded",
>> +				DBUS_TYPE_STRING,&info,
>> +				DBUS_TYPE_INVALID);
>> +}
>> +
>> +static struct voicecall *voicecall_select(struct ofono_voicecall *vc,
>> +						unsigned int id, int code)
>> +{
>> +	struct voicecall *v = NULL;
>> +	GSList *l;
>> +
>> +	for (l = vc->call_list; l; l = l->next) {
>> +		struct voicecall *v1 = l->data;
>> +
>> +		if (id == 0&&  g_slist_length(vc->call_list) == 1) {
>> +			if (code == SS_MT_VOICECALL_RETRIEVED&&
>> +					v1->call->remote_held == TRUE) {
>> +				v = v1;
>> +				break;
>> +			} else if (code == SS_MT_VOICECALL_ON_HOLD&&
>> +					v1->call->remote_held == FALSE) {
>> +				v = v1;
>> +				break;
>> +			} else if (code == SS_MT_MULTIPARTY_VOICECALL&&
>> +					v1->call->remote_multiparty == FALSE) {
>> +				v = v1;
>> +				break;
>> +			}
>> +		} else if (v1->call->id == id) {
>> +			v = v1;
>> +			break;
>> +		}
>> +	}
>> +
>> +	return v;
>> +}
> I suspect this might be easier to understand if it was re-written to use
> g_slist_find_custom instead of jumbling unrelated logic (with a similar
> implementation) into a single function.

Maybe a "g_slist_find_custom" function base implementation would make
the code look more neat. I'll make the changes.

>> +
>> +static void ssn_mt_remote_held_notify(struct ofono_voicecall *vc,
>> +					unsigned int id, int code,
>> +					const struct ofono_phone_number *ph)
>> +{
>> +	struct voicecall *v = voicecall_select(vc, id, code);
>> +	DBusConnection *conn = ofono_dbus_get_connection();
>> +	const char *path;
>> +
>> +	if (v == NULL)
>> +		return;
>> +
>> +	if (code == SS_MT_VOICECALL_ON_HOLD)
>> +		v->call->remote_held = TRUE;
>> +	else
>> +		v->call->remote_held = FALSE;
>> +
>> +	path = voicecall_build_path(vc, v->call);
>> +
>> +	ofono_dbus_signal_property_changed(conn, path,
>> +						OFONO_VOICECALL_INTERFACE,
>> +						"RemoteHeld", DBUS_TYPE_BOOLEAN,
>> +						&v->call->remote_held);
>> +}
>> +
>> +static void ssn_mt_remote_multiparty_notify(struct ofono_voicecall *vc,
>> +					unsigned int id, int code,
>> +					const struct ofono_phone_number *ph)
>> +{
>> +	struct voicecall *v = voicecall_select(vc, id, code);
>> +	DBusConnection *conn = ofono_dbus_get_connection();
>> +	const char *path;
>> +
>> +	if (v == NULL)
>> +		return;
>> +
>> +	v->call->remote_multiparty = TRUE;
>> +
>> +	path = voicecall_build_path(vc, v->call);
>> +
>> +	ofono_dbus_signal_property_changed(conn, path,
>> +					OFONO_VOICECALL_INTERFACE,
>> +					"RemoteMultiparty", DBUS_TYPE_BOOLEAN,
>> +					&v->call->remote_multiparty);
>> +}
>> +
>> +void ofono_voicecall_ssn_mt_notify(struct ofono_voicecall *vc,
>> +					unsigned int id, int code, int index,
>> +					const struct ofono_phone_number *ph)
>> +{
>> +
>> +	if (code == SS_MT_CALL_FORWARDED)
>> +		ssn_mt_forwarded_notify(vc, id, code, ph);
>> +	else if (code == SS_MT_VOICECALL_ON_HOLD)
>> +		ssn_mt_remote_held_notify(vc, id, code, ph);
>> +	else if (code == SS_MT_VOICECALL_RETRIEVED)
>> +		ssn_mt_remote_held_notify(vc, id, code, ph);
>> +	else if (code == SS_MT_MULTIPARTY_VOICECALL)
>> +		ssn_mt_remote_multiparty_notify(vc, id, code, ph);
> Please use a switch/case here
>

OK, no problem.

>> +}
>> +
>> +static void ssn_mo_call_barred_notify(struct ofono_voicecall *vc,
>> +					unsigned int id, int code)
>> +{
>> +	DBusConnection *conn = ofono_dbus_get_connection();
>> +	const char *path = __ofono_atom_get_path(vc->atom);
>> +	const char *info;
>> +
>> +	if (code == SS_MO_INCOMING_BARRING)
>> +		info = "remote";
>> +	else
>> +		info = "local";
>> +
>> +	g_dbus_emit_signal(conn, path, OFONO_VOICECALL_MANAGER_INTERFACE,
>> +				"BarringActive",
>> +				DBUS_TYPE_STRING,&info,
>> +				DBUS_TYPE_INVALID);
>> +}
>> +
>> +static void ssn_mo_forwarded_notify(struct ofono_voicecall *vc,
>> +					unsigned int id, int code)
>> +{
>> +	DBusConnection *conn = ofono_dbus_get_connection();
>> +	const char *path = __ofono_atom_get_path(vc->atom);
>> +	char *info = "outgoing";
>> +
>> +	g_dbus_emit_signal(conn, path, OFONO_VOICECALL_MANAGER_INTERFACE,
>> +				"Forwarded",
>> +				DBUS_TYPE_STRING,&info,
>> +				DBUS_TYPE_INVALID);
>> +}
>> +
>> +void ofono_voicecall_ssn_mo_notify(struct ofono_voicecall *vc,
>> +					unsigned int id, int code, int index)
>> +{
>> +	if (code == SS_MO_OUTGOING_BARRING)
>> +		ssn_mo_call_barred_notify(vc, id, code);
>> +	else if (code == SS_MO_INCOMING_BARRING)
>> +		ssn_mo_call_barred_notify(vc, id, code);
>> +	else if (code == SS_MO_CALL_FORWARDED)
>> +		ssn_mo_forwarded_notify(vc, id, code);
> Please use a switch/case here.  It is much easier to read and extend.

OK, I'll do it.

>> +}
> Regards,
> -Denis

Regards,
Andras