Hi Denis,
On Wed, Nov 24, 2010 at 1:58 PM, Denis Kenzior <denk...@gmail.com> wrote:
> Hi Lucas,
>
> On 11/23/2010 12:04 PM, Lucas De Marchi wrote:
>> ---
>> Makefile.am | 2 +-
>> src/ofono.h | 2 +
>> src/text-telephony.c | 341
>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 344 insertions(+), 1 deletions(-)
>> create mode 100644 src/text-telephony.c
>>
>> diff --git a/Makefile.am b/Makefile.am
>> index fb075a0..ee1313d 100644
>> --- a/Makefile.am
>> +++ b/Makefile.am
>> +static DBusMessage *tt_get_properties_reply(DBusMessage *msg,
>> + struct ofono_text_telephony
>> *tt)
>> +{
>> + DBusMessage *reply;
>> + DBusMessageIter iter;
>> + DBusMessageIter dict;
>> + dbus_bool_t value;
>> +
>> + reply = dbus_message_new_method_return(msg);
>> + if (!reply)
>> + return NULL;
>> +
>> + dbus_message_iter_init_append(reply, &iter);
>> + dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
>> + OFONO_PROPERTIES_ARRAY_SIGNATURE,
>> + &dict);
>> +
>> + value = tt->powered;
>> + ofono_dbus_dict_append(&dict, "Powered", DBUS_TYPE_BOOLEAN, &value);
>
> As mentioned previously, please use "Enabled" here.
ok
>> +static void tt_send_properties_reply(struct ofono_text_telephony *tt)
>> +{
>> + DBusMessage *reply;
>> +
>> + tt->flags |= TEXT_TELEPHONY_FLAG_CACHED;
>> +
>> + reply = tt_get_properties_reply(tt->pending, tt);
>> + __ofono_dbus_pending_reply(&tt->pending, reply);
>
> I actually prefer to fold this function into powered_callback
ok
>
>> +}
>> +
>> +static void tt_query_powered_callback(const struct ofono_error *error,
>> + ofono_bool_t enable, void
>> *data)
>> +{
>> + struct ofono_text_telephony *tt = data;
>> +
>> + if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
>> + DBusMessage *reply;
>> +
>> + ofono_debug("Error during powered query");
>> +
>> + reply = __ofono_error_failed(tt->pending);
>> + __ofono_dbus_pending_reply(&tt->pending, reply);
>> +
>> + return;
>> + }
>> +
>> + tt_set_powered(tt, enable);
>> + tt_send_properties_reply(tt);
>
> The oFono convention is to reply to the pending D-Bus method call and
> then signal the property changed signals.
>
The examples I found for query methods use exactly the opposite. See,
cw_ss_query_callback() in call-settings.c and
radio_fast_dormancy_query_callback() in radio-settings.c.
This is because when calling the "tt_get_properties_reply(tt->pending,
tt)" we need the new value in tt->powered in order to generate the
reply correctly. Otherwise, we would need to save the value elsewhere,
set with the new value, generate the message, restore the previous
value and then call tt_set_powered(tt, enable).
If we do this, maybe we have to change the other places too.
>
>> +int ofono_text_telephony_driver_register(const struct
>> ofono_text_telephony_driver *d)
>> +{
>> + DBG("driver: %p, name: %s", d, d->name);
>> +
>> + if (!d || !d->probe)
>> + return -EINVAL;
>> +
>> + g_drivers = g_slist_prepend(g_drivers, (void *)d);
>> +
>> + return 0;
>> +}
>> +
>> +void ofono_text_telephony_driver_unregister(const struct
>> ofono_text_telephony_driver *d)
>> +{
>> + DBG("driver: %p, name: %s", d, d->name);
>> +
>> + if (!d)
>
> The preferred style is to compare to NULL, e.g. if (d == NULL)
Ok. I changed all the other places as well.
>
>> + return;
>> +
>> + g_drivers = g_slist_remove(g_drivers, (void *)d);
>> +}
>> +
>
> <snip>
>
>> +struct ofono_text_telephony *ofono_text_telephony_create(struct ofono_modem
>> *modem,
>> + unsigned int vendor,
>> + const char *driver,
>> + void *data)
>> +{
>> + struct ofono_text_telephony *tt;
>> + GSList *l;
>
> Please add a newline here
ok
>
>> + if (!driver)
>> + return NULL;
>> +
>> + tt = g_try_new0(struct ofono_text_telephony, 1);
>> + if (!tt)
>> + return NULL;
>> +
>> + tt->atom = __ofono_modem_add_atom(modem,
>> OFONO_ATOM_TYPE_TEXT_TELEPHONY,
>> + text_telephony_remove, tt);
>> +
>> + tt->powered = -1;
>
> I think it is safe to default this to 0 (off).
ok
>
>> +
>> + for (l = g_drivers; l; l = l->next) {
>> + const struct ofono_text_telephony_driver *drv = l->data;
>> +
>> + if (g_strcmp0(drv->name, driver) != 0)
>> + continue;
>> +
>> + if (drv->probe(tt, vendor, data) < 0)
>> + continue;
>> +
>> + tt->driver = drv;
>> + break;
>> + }
>> +
>> + return tt;
>> +}
>> +
>
> Otherwise, looks good.
regards,
Lucas De Marchi
_______________________________________________
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
