11:26 p.m.
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