7:30 a.m.
Hi Denis.
...
> +static void at_hangup_active(struct ofono_voicecall *vc,
> + ofono_voicecall_cb_t cb, void *data)
> +{
> + /* Hangup currently active call */
It might be safer to only implement hangup_active in the generic driver.
However, since this driver is only for testing anyway, the change is fine.
Ok, I might change that then.
...
> +/* HACK: for temporary backwards compatibility */
> +#define hangup_all hangup
> + void (*hangup_all)(struct ofono_voicecall *vc,
> + ofono_voicecall_cb_t cb, void *data);
> + void (*hangup_active)(struct ofono_voicecall *vc,
We'll need to figure out how to migrate to the new driver API a bit more
sanely.
Should we s/hangup/hangup_active in all drivers and skip this hack then?
Or did you have something else in mind?
....
static void voicecalls_release_queue(struct ofono_voicecall *vc,
GSList *calls)
> {
> GSList *l;
> + struct voicecall *call;
>
> g_slist_free(vc->release_list);
> vc->release_list = NULL;
>
> - for (l = calls; l; l = l->next)
> - vc->release_list = g_slist_prepend(vc->release_list,
l->data);
> + for (l = calls; l; l = l->next) {
> + call = l->data;
> +
> + if (call->call->status != CALL_STATUS_WAITING)
> + vc->release_list =
> + g_slist_prepend(vc->release_list, call);
> + }
Actually I prefer this to be done in manager_hangup_all. This function
is used by the multiparty release path which can never include waiting
calls.
OK, I'll look into that.
...
> @@ -1131,9 +1165,17 @@ static DBusMessage
*manager_hangup_all(DBusConnection *conn,
>
> vc->pending = dbus_message_ref(msg);
>
> - voicecalls_release_queue(vc, vc->call_list);
> - voicecalls_release_next(vc);
> + if (vc->driver->hangup_all != NULL)
> + vc->driver->hangup_all(vc, generic_callback, vc);
>
> + /* In case of INCOMING call, assume we only have this one call */
> + else if (voicecalls_have_incoming(vc) &&
> + vc->driver->hangup_active != NULL)
> + vc->driver->hangup_active(vc, generic_callback, vc);
Now that I think about it, the else if clause is not needed. You take
care of incoming calls with hangup_active in voicecalls_release_next.
Right?
Yes, agree.
...
> @@ -1370,8 +1412,12 @@ static DBusMessage
*multiparty_hangup(DBusConnection *conn,
>
> /* Fall back to the old-fashioned way */
> vc->flags |= VOICECALLS_FLAG_MULTI_RELEASE;
> - voicecalls_release_queue(vc, vc->multiparty_list);
> - voicecalls_release_next(vc);
> + if (vc->driver->hangup_all != NULL)
> + vc->driver->hangup_all(vc, generic_callback, vc);
> + else {
> + voicecalls_release_queue(vc, vc->call_list);
> + voicecalls_release_next(vc);
> + }
This doesn't look right. If we get to this part then we either have
waiting + active, waiting + held, waiting + active + held or active +
held. Calling hangup_all would be all right for the first two cases,
but not the other two.
For multiparty we need to fall back to release_specific. The original
code was fine.
OK, I'll revert this.
How do we proceede with this change. I am very happy if you carry on from here,
or would you like me to to submit a patch-set with these changes,
including s/hangup/hangup_active in drivers/*/voicecall.c?
Regards
Sjur