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.
> 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.
> };
>
> 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.
> #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.
> +
> +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
> +}
> +
> +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.
> +}
Regards,
-Denis
_______________________________________________
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
