2012-08-24 13:59:49

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: [PATCH obexd 1/7] client: Add filters to Phonebook.PullAll

From: Luiz Augusto von Dentz <[email protected]>

This avoid D-Bus round trips and is more aligned with what has been
proposed for MessageAccess interface.
---
client/pbap.c | 254 +++++++++++++++++++++++++++++++++++++++++++++++-----------
1 file changed, 208 insertions(+), 46 deletions(-)

diff --git a/client/pbap.c b/client/pbap.c
index ebd6320..0c1336c 100644
--- a/client/pbap.c
+++ b/client/pbap.c
@@ -351,18 +351,189 @@ send:
pending_request_free(request);
}

+static GObexApparam *parse_format(GObexApparam *apparam, DBusMessageIter *iter)
+{
+ const char *string;
+ guint8 format;
+
+ if (dbus_message_iter_get_arg_type(iter) != DBUS_TYPE_STRING)
+ return NULL;
+
+ dbus_message_iter_get_basic(iter, &string);
+
+ if (!string || g_str_equal(string, ""))
+ format = FORMAT_VCARD21;
+ else if (!g_ascii_strcasecmp(string, "vcard21"))
+ format = FORMAT_VCARD21;
+ else if (!g_ascii_strcasecmp(string, "vcard30"))
+ format = FORMAT_VCARD30;
+ else
+ return NULL;
+
+ return g_obex_apparam_set_uint8(apparam, FORMAT_TAG, format);
+}
+
+static GObexApparam *parse_order(GObexApparam *apparam, DBusMessageIter *iter)
+{
+ const char *string;
+ guint8 order;
+
+ if (dbus_message_iter_get_arg_type(iter) != DBUS_TYPE_STRING)
+ return NULL;
+
+ dbus_message_iter_get_basic(iter, &string);
+
+ if (!string || g_str_equal(string, ""))
+ order = ORDER_INDEXED;
+ else if (!g_ascii_strcasecmp(string, "indexed"))
+ order = ORDER_INDEXED;
+ else if (!g_ascii_strcasecmp(string, "alphanumeric"))
+ order = ORDER_ALPHANUMERIC;
+ else if (!g_ascii_strcasecmp(string, "phonetic"))
+ order = ORDER_PHONETIC;
+ else
+ return NULL;
+
+ return g_obex_apparam_set_uint8(apparam, ORDER_TAG, order);
+}
+
+static GObexApparam *parse_offset(GObexApparam *apparam, DBusMessageIter *iter)
+{
+ guint16 num;
+
+ if (dbus_message_iter_get_arg_type(iter) != DBUS_TYPE_UINT16)
+ return NULL;
+
+ dbus_message_iter_get_basic(iter, &num);
+
+ return g_obex_apparam_set_uint16(apparam, LISTSTARTOFFSET_TAG, num);
+}
+
+static GObexApparam *parse_items(GObexApparam *apparam, DBusMessageIter *iter)
+{
+ guint16 num;
+
+ if (dbus_message_iter_get_arg_type(iter) != DBUS_TYPE_UINT16)
+ return NULL;
+
+ dbus_message_iter_get_basic(iter, &num);
+
+ return g_obex_apparam_set_uint16(apparam, MAXLISTCOUNT_TAG, num);
+}
+
+static uint64_t get_filter_mask(const char *filterstr)
+{
+ int i, bit = -1;
+
+ if (!filterstr)
+ return 0;
+
+ if (!g_ascii_strcasecmp(filterstr, "ALL"))
+ return FILTER_ALL;
+
+ for (i = 0; filter_list[i] != NULL; i++)
+ if (!g_ascii_strcasecmp(filterstr, filter_list[i]))
+ return 1ULL << i;
+
+ if (strlen(filterstr) < 4 || strlen(filterstr) > 5
+ || g_ascii_strncasecmp(filterstr, "bit", 3) != 0)
+ return 0;
+
+ sscanf(&filterstr[3], "%d", &bit);
+ if (bit >= 0 && bit <= FILTER_BIT_MAX)
+ return 1ULL << bit;
+ else
+ return 0;
+}
+
+static int set_field(guint64 *filter, const char *filterstr)
+{
+ guint64 mask;
+
+ mask = get_filter_mask(filterstr);
+
+ if (mask == 0)
+ return -EINVAL;
+
+ *filter |= mask;
+ return 0;
+}
+
+static GObexApparam *parse_fields(GObexApparam *apparam, DBusMessageIter *iter)
+{
+ DBusMessageIter array;
+ guint64 filter = 0;
+
+ if (dbus_message_iter_get_arg_type(iter) != DBUS_TYPE_ARRAY)
+ return NULL;
+
+ dbus_message_iter_recurse(iter, &array);
+
+ while (dbus_message_iter_get_arg_type(&array) == DBUS_TYPE_STRING) {
+ const char *string;
+
+ dbus_message_iter_get_basic(&array, &string);
+
+ if (set_field(&filter, string) < 0)
+ return NULL;
+
+ dbus_message_iter_next(&array);
+ }
+
+ return g_obex_apparam_set_uint64(apparam, FILTER_TAG, filter);
+}
+static GObexApparam *parse_filters(GObexApparam *apparam,
+ DBusMessageIter *iter)
+{
+ DBusMessageIter array;
+
+ if (dbus_message_iter_get_arg_type(iter) != DBUS_TYPE_ARRAY)
+ return NULL;
+
+ dbus_message_iter_recurse(iter, &array);
+
+ while (dbus_message_iter_get_arg_type(&array) == DBUS_TYPE_DICT_ENTRY) {
+ const char *key;
+ DBusMessageIter value, entry;
+
+ dbus_message_iter_recurse(&array, &entry);
+ dbus_message_iter_get_basic(&entry, &key);
+
+ dbus_message_iter_next(&entry);
+ dbus_message_iter_recurse(&entry, &value);
+
+ if (strcasecmp(key, "Format") == 0) {
+ if (parse_format(apparam, &value) == NULL)
+ return NULL;
+ } else if (strcasecmp(key, "Order") == 0) {
+ if (parse_order(apparam, &value) == NULL)
+ return NULL;
+ } else if (strcasecmp(key, "Offset") == 0) {
+ if (parse_offset(apparam, &value) == NULL)
+ return NULL;
+ } else if (strcasecmp(key, "Items") == 0) {
+ if (parse_items(apparam, &value) == NULL)
+ return NULL;
+ } else if (strcasecmp(key, "Fields") == 0) {
+ if (parse_fields(apparam, &value) == NULL)
+ return NULL;
+ }
+
+ dbus_message_iter_next(&array);
+ }
+
+ return apparam;
+}
+
static struct obc_transfer *pull_phonebook(struct pbap_data *pbap,
DBusMessage *message,
guint8 type, const char *name,
const char *targetfile,
- uint64_t filter, guint8 format,
- guint16 maxlistcount,
- guint16 liststartoffset,
+ GObexApparam *apparam,
GError **err)
{
struct pending_request *request;
struct obc_transfer *transfer;
- GObexApparam *apparam;
guint8 buf[32];
gsize len;
session_callback_t func;
@@ -371,13 +542,6 @@ static struct obc_transfer *pull_phonebook(struct pbap_data *pbap,
if (transfer == NULL)
return NULL;

- apparam = g_obex_apparam_set_uint64(NULL, FILTER_TAG, filter);
- apparam = g_obex_apparam_set_uint8(apparam, FORMAT_TAG, format);
- apparam = g_obex_apparam_set_uint16(apparam, MAXLISTCOUNT_TAG,
- maxlistcount);
- apparam = g_obex_apparam_set_uint16(apparam, LISTSTARTOFFSET_TAG,
- liststartoffset);
-
switch (type) {
case PULLPHONEBOOK:
func = NULL;
@@ -396,8 +560,6 @@ static struct obc_transfer *pull_phonebook(struct pbap_data *pbap,

obc_transfer_set_params(transfer, buf, len);

- g_obex_apparam_free(apparam);
-
if (!obc_session_queue(pbap->session, transfer, func, request, err)) {
if (request != NULL)
pending_request_free(request);
@@ -490,31 +652,6 @@ static int set_order(struct pbap_data *pbap, const char *orderstr)
return 0;
}

-static uint64_t get_filter_mask(const char *filterstr)
-{
- int i, bit = -1;
-
- if (!filterstr)
- return 0;
-
- if (!g_ascii_strcasecmp(filterstr, "ALL"))
- return FILTER_ALL;
-
- for (i = 0; filter_list[i] != NULL; i++)
- if (!g_ascii_strcasecmp(filterstr, filter_list[i]))
- return 1ULL << i;
-
- if (strlen(filterstr) < 4 || strlen(filterstr) > 5
- || g_ascii_strncasecmp(filterstr, "bit", 3) != 0)
- return 0;
-
- sscanf(&filterstr[3], "%d", &bit);
- if (bit >= 0 && bit <= FILTER_BIT_MAX)
- return 1ULL << bit;
- else
- return 0;
-}
-
static int add_filter(struct pbap_data *pbap, const char *filterstr)
{
uint64_t mask;
@@ -618,25 +755,41 @@ static DBusMessage *pbap_pull_all(DBusConnection *connection,
struct obc_transfer *transfer;
const char *targetfile;
char *name;
+ GObexApparam *apparam;
GError *err = NULL;
+ DBusMessageIter args;

if (!pbap->path)
return g_dbus_create_error(message,
ERROR_INTERFACE ".Forbidden",
"Call Select first of all");

- if (dbus_message_get_args(message, NULL,
- DBUS_TYPE_STRING, &targetfile,
- DBUS_TYPE_INVALID) == FALSE)
+ dbus_message_iter_init(message, &args);
+
+ if (dbus_message_iter_get_arg_type(&args) != DBUS_TYPE_STRING)
return g_dbus_create_error(message,
ERROR_INTERFACE ".InvalidArguments", NULL);

+ dbus_message_iter_get_basic(&args, &targetfile);
+ dbus_message_iter_next(&args);
+
+ apparam = g_obex_apparam_set_uint16(NULL, MAXLISTCOUNT_TAG,
+ DEFAULT_COUNT);
+ apparam = g_obex_apparam_set_uint16(apparam, LISTSTARTOFFSET_TAG,
+ DEFAULT_OFFSET);
+
+ if (parse_filters(apparam, &args) == NULL) {
+ g_obex_apparam_free(apparam);
+ return g_dbus_create_error(message,
+ ERROR_INTERFACE ".InvalidArguments", NULL);
+ }
+
name = g_strconcat(pbap->path, ".vcf", NULL);

transfer = pull_phonebook(pbap, message, PULLPHONEBOOK, name,
- targetfile, pbap->filter, pbap->format,
- DEFAULT_COUNT, DEFAULT_OFFSET, &err);
+ targetfile, apparam, &err);
g_free(name);
+ g_obex_apparam_free(apparam);

if (transfer == NULL) {
DBusMessage *reply = g_dbus_create_error(message,
@@ -754,20 +907,28 @@ static DBusMessage *pbap_get_size(DBusConnection *connection,
DBusMessage *reply;
struct obc_transfer *transfer;
char *name;
+ GObexApparam *apparam;
GError *err = NULL;
+ DBusMessageIter args;

if (!pbap->path)
return g_dbus_create_error(message,
ERROR_INTERFACE ".Forbidden",
"Call Select first of all");

+ dbus_message_iter_init(message, &args);
+
name = g_strconcat(pbap->path, ".vcf", NULL);

+ apparam = g_obex_apparam_set_uint16(NULL, MAXLISTCOUNT_TAG, 0);
+ apparam = g_obex_apparam_set_uint16(apparam, LISTSTARTOFFSET_TAG,
+ DEFAULT_OFFSET);
+
transfer = pull_phonebook(pbap, message, GETPHONEBOOKSIZE, name, NULL,
- pbap->filter, pbap->format, 0,
- DEFAULT_OFFSET, &err);
+ apparam, &err);

g_free(name);
+ g_obex_apparam_free(apparam);

if (transfer != NULL)
return NULL;
@@ -891,7 +1052,8 @@ static const GDBusMethodTable pbap_methods[] = {
GDBUS_ARGS({ "location", "s" }, { "phonebook", "s" }),
NULL, pbap_select) },
{ GDBUS_METHOD("PullAll",
- GDBUS_ARGS({ "targetfile", "s" }),
+ GDBUS_ARGS({ "targetfile", "s" },
+ { "filters", "a{sv}" }),
GDBUS_ARGS({ "transfer", "o" },
{ "properties", "a{sv}" }),
pbap_pull_all) },
--
1.7.11.4



2012-08-28 12:03:30

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH obexd 7/7] client-doc: Update documentation of PhonebookAccess interface

Hi Patrick,

On Tue, Aug 28, 2012 at 12:33 PM, Patrick Ohly <[email protected]> wrote:
> The user of a specific device would have to know that.
>
> SyncEvolution's "exclude fields" logic is based on the assumption that
> requesting everything, including the custom bits, will result in
> everything the device supports, just like not specifying any filter at
> all.

So there is a possibility that we detect what kind of extra bits the
remote stack implements based on vendor id/product id, but I would
like to avoid specific behavior per device. Besides it seems what we
want is all custom bits.

>> > Should the API spec really specify the full list? IMHO the documentation
>> > should make it clear that the list is not exhaustive - more fields might
>> > be added in future releases of the PBAP standard and obexd, without
>> > changing the obexd API.
>>
>> If we can discover what the bits mean then yes, but unfortunately this
>> is hardcoded in the PBAP spec (see page 30), so we are only defining
>> the one we know the meaning.
>
> It's okay to only document those, but it should still allow the use of
> the other things. Otherwise a user of the API would have to patch obexd
> to use the extensions.
>
>> > In SyncEvolution, I use ListFilterFields() to a) do sanity checks on
>> > user input and to b) build a list of all fields from which the user can
>> > exclude specific fields. Because the field list is not hard-coded, that
>> > will work for fields which are not defined yet.
>>
>> As you can see from the spec this is pretty static so adding a round
>> trip to discover the list sounds wrong to me,
>
> Hard-coding it in SyncEvolution seems equally wrong to me. If the list
> changes, then only obexd needs to be updated, not "obexd +
> SyncEvolution".
>
> It's only one round-trip per session. As a tradeoff between performance
> and being future-proof I think doing the call is worth it.

Fair enough so Im keeping ListFilterFields with original names to be
direct match to the vcard fields, I hope this is really useful for
someone.

--
Luiz Augusto von Dentz

2012-08-28 09:33:01

by Patrick Ohly

[permalink] [raw]
Subject: Re: [PATCH obexd 7/7] client-doc: Update documentation of PhonebookAccess interface

On Tue, 2012-08-28 at 10:52 +0300, Luiz Augusto von Dentz wrote:
> On Tue, Aug 28, 2012 at 9:56 AM, Patrick Ohly <[email protected]> wrote:
> > What about selecting the bits which don't have a named property
> > associated with them? They are reported by ListFilterFields() and thus
> > seem to be supported.
>
> ListFilterFields is removed in this case,

I find it useful, please don't remove it.

> anyway setting bits is still
> supported but Im not so sure I would keep it since apparently there is
> no way to discover what they mean which could be different from stack
> to stack.

The user of a specific device would have to know that.

SyncEvolution's "exclude fields" logic is based on the assumption that
requesting everything, including the custom bits, will result in
everything the device supports, just like not specifying any filter at
all.

> > Should the API spec really specify the full list? IMHO the documentation
> > should make it clear that the list is not exhaustive - more fields might
> > be added in future releases of the PBAP standard and obexd, without
> > changing the obexd API.
>
> If we can discover what the bits mean then yes, but unfortunately this
> is hardcoded in the PBAP spec (see page 30), so we are only defining
> the one we know the meaning.

It's okay to only document those, but it should still allow the use of
the other things. Otherwise a user of the API would have to patch obexd
to use the extensions.

> > In SyncEvolution, I use ListFilterFields() to a) do sanity checks on
> > user input and to b) build a list of all fields from which the user can
> > exclude specific fields. Because the field list is not hard-coded, that
> > will work for fields which are not defined yet.
>
> As you can see from the spec this is pretty static so adding a round
> trip to discover the list sounds wrong to me,

Hard-coding it in SyncEvolution seems equally wrong to me. If the list
changes, then only obexd needs to be updated, not "obexd +
SyncEvolution".

It's only one round-trip per session. As a tradeoff between performance
and being future-proof I think doing the call is worth it.

--
Best Regards

Patrick Ohly
Senior Software Engineer

Intel GmbH
Open Source Technology Center
Pützstr. 5 Phone: +49-228-2493652
53129 Bonn
Germany


2012-08-28 09:24:32

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH obexd 7/7] client-doc: Update documentation of PhonebookAccess interface

Hi Johan,

On Tue, Aug 28, 2012 at 10:00 AM, Luiz Augusto von Dentz
<[email protected]> wrote:
>>> + array{string} Fields:
>>> +
>>> + Item vcard fields, default is all
>>> + values.
>>> +
>>> + Possible values:
>>> +
>>> + "VERSION",
>>> + "FN",
>>> + "N",
>>> + "PHOTO",
>>> + "BDAY",
>>> + "ADR",
>>> + "LABEL",
>>> + "TEL",
>>> + "EMAIL",
>>> + "MAILER",
>>> + "TZ",
>>> + "GEO",
>>> + "TITLE",
>>> + "ROLE",
>>> + "LOGO",
>>> + "AGENT",
>>> + "ORG",
>>> + "NOTE",
>>> + "REV",
>>> + "SOUND",
>>> + "URL",
>>> + "UID",
>>> + "KEY",
>>> + "NICKNAME",
>>> + "CATEGORIES",
>>> + "PROID",
>>> + "CLASS",
>>> + "SORT-STRING",
>>> + "X-IRMC-CALL-DATETIME"
>>
>> Make these lower case (I know it's upper case in the spec. but let's
>> keep the consistency with the rest of the D-Bus API).

So what about the following:

Possible values:

"version",
"formatted-name",
"name",
"photo",
"birthday",
"address",
"label",
"telephone",
"email",
"mailer",
"time-zone",
"location",
"title",
"role",
"logo",
"agent",
"organization",
"note",
"revision",
"source",
"url",
"uid",
"key",
"nickname",
"categories",
"product",
"class",
"sort-string",
"timestamp"

--
Luiz Augusto von Dentz

2012-08-28 07:52:27

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH obexd 7/7] client-doc: Update documentation of PhonebookAccess interface

Hi Patrick,

On Tue, Aug 28, 2012 at 9:56 AM, Patrick Ohly <[email protected]> wrote:
> On Fri, 2012-08-24 at 16:59 +0300, Luiz Augusto von Dentz wrote:
>> + array{string} Fields:
>> +
>> + Item vcard fields, default is all
>> + values.
>> +
>> + Possible values:
>> +
>> + "VERSION",
>> + "FN",
>> + "N",
>> + "PHOTO",
>> + "BDAY",
>> + "ADR",
>> + "LABEL",
>> + "TEL",
>> + "EMAIL",
>> + "MAILER",
>> + "TZ",
>> + "GEO",
>> + "TITLE",
>> + "ROLE",
>> + "LOGO",
>> + "AGENT",
>> + "ORG",
>> + "NOTE",
>> + "REV",
>> + "SOUND",
>> + "URL",
>> + "UID",
>> + "KEY",
>> + "NICKNAME",
>> + "CATEGORIES",
>> + "PROID",
>> + "CLASS",
>> + "SORT-STRING",
>> + "X-IRMC-CALL-DATETIME"
>
> What about selecting the bits which don't have a named property
> associated with them? They are reported by ListFilterFields() and thus
> seem to be supported.

ListFilterFields is removed in this case, anyway setting bits is still
supported but Im not so sure I would keep it since apparently there is
no way to discover what they mean which could be different from stack
to stack.

> Should the API spec really specify the full list? IMHO the documentation
> should make it clear that the list is not exhaustive - more fields might
> be added in future releases of the PBAP standard and obexd, without
> changing the obexd API.

If we can discover what the bits mean then yes, but unfortunately this
is hardcoded in the PBAP spec (see page 30), so we are only defining
the one we know the meaning.

> In SyncEvolution, I use ListFilterFields() to a) do sanity checks on
> user input and to b) build a list of all fields from which the user can
> exclude specific fields. Because the field list is not hard-coded, that
> will work for fields which are not defined yet.

As you can see from the spec this is pretty static so adding a round
trip to discover the list sounds wrong to me, but perhaps you have
some way to discover the meaning of the bits not defined in the spec?

--
Luiz Augusto von Dentz

2012-08-28 07:00:45

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH obexd 7/7] client-doc: Update documentation of PhonebookAccess interface

Hi Johan,

On Tue, Aug 28, 2012 at 4:17 AM, Johan Hedberg <[email protected]> wrote:
> Hi Luiz,
>
> On Fri, Aug 24, 2012, Luiz Augusto von Dentz wrote:
>> ---
>> doc/client-api.txt | 180 ++++++++++++++++++++++++++++++++++++++++++-----------
>> 1 file changed, 145 insertions(+), 35 deletions(-)
>
> I had a chat with Marcel about this API which resulted with the
> following change proposals:
>
>> + string Order:
>> +
>> + Items order
>> +
>> + Possible values: "indexed" (default),
>> + "alphanumeric" or "phonetic"
>> +
>> + uint16 Offset:
>> +
>> + Offset of the first item, default is 0
>> +
>> + uint16 Items:
>> +
>> + Maximum number of items, default is
>> + unlimited (65535)
>
> Call this MaxCount or Count to make it clearer.

Will do it.

>> +
>> + array{string} Fields:
>> +
>> + Item vcard fields, default is all
>> + values.
>> +
>> + Possible values:
>> +
>> + "VERSION",
>> + "FN",
>> + "N",
>> + "PHOTO",
>> + "BDAY",
>> + "ADR",
>> + "LABEL",
>> + "TEL",
>> + "EMAIL",
>> + "MAILER",
>> + "TZ",
>> + "GEO",
>> + "TITLE",
>> + "ROLE",
>> + "LOGO",
>> + "AGENT",
>> + "ORG",
>> + "NOTE",
>> + "REV",
>> + "SOUND",
>> + "URL",
>> + "UID",
>> + "KEY",
>> + "NICKNAME",
>> + "CATEGORIES",
>> + "PROID",
>> + "CLASS",
>> + "SORT-STRING",
>> + "X-IRMC-CALL-DATETIME"
>
> Make these lower case (I know it's upper case in the spec. but let's
> keep the consistency with the rest of the D-Bus API).

Sure

>> @@ -244,8 +325,54 @@ Methods void Select(string location, string phonebook)
>> The properties of this transfer are also returned along
>> with the object path, to avoid a call to GetProperties.
>>
>> + filters:
>> +
>> + string Format:
>> +
>> + Items vcard format
>> +
>> + Possible values: "vcard21" (default) or
>> + "vcard30"
>> +
>> + array{string} Fields:
>> +
>> + Item vcard fields, default is all
>> + values.
>> +
>> + Possible values:
>> +
>> + "VERSION",
>> + "FN",
>> + "N",
>> + "PHOTO",
>> + "BDAY",
>> + "ADR",
>> + "LABEL",
>> + "TEL",
>> + "EMAIL",
>> + "MAILER",
>> + "TZ",
>> + "GEO",
>> + "TITLE",
>> + "ROLE",
>> + "LOGO",
>> + "AGENT",
>> + "ORG",
>> + "NOTE",
>> + "REV",
>> + "SOUND",
>> + "URL",
>> + "UID",
>> + "KEY",
>> + "NICKNAME",
>> + "CATEGORIES",
>> + "PROID",
>> + "CLASS",
>> + "SORT-STRING",
>> + "X-IRMC-CALL-DATETIME"
>
> Is this a duplicate of the previous section. Maybe just move this out to
> a common section and the refer to it from the places that need it.

Depending on the method some parameters may not be valid, but maybe we
can have a section explaining the filters parameter then in each of
method we just enumerate what are the possible parameters for its
filter.

--
Luiz Augusto von Dentz

2012-08-28 06:56:54

by Patrick Ohly

[permalink] [raw]
Subject: Re: [PATCH obexd 7/7] client-doc: Update documentation of PhonebookAccess interface

On Fri, 2012-08-24 at 16:59 +0300, Luiz Augusto von Dentz wrote:
> + array{string} Fields:
> +
> + Item vcard fields, default is all
> + values.
> +
> + Possible values:
> +
> + "VERSION",
> + "FN",
> + "N",
> + "PHOTO",
> + "BDAY",
> + "ADR",
> + "LABEL",
> + "TEL",
> + "EMAIL",
> + "MAILER",
> + "TZ",
> + "GEO",
> + "TITLE",
> + "ROLE",
> + "LOGO",
> + "AGENT",
> + "ORG",
> + "NOTE",
> + "REV",
> + "SOUND",
> + "URL",
> + "UID",
> + "KEY",
> + "NICKNAME",
> + "CATEGORIES",
> + "PROID",
> + "CLASS",
> + "SORT-STRING",
> + "X-IRMC-CALL-DATETIME"

What about selecting the bits which don't have a named property
associated with them? They are reported by ListFilterFields() and thus
seem to be supported.

Should the API spec really specify the full list? IMHO the documentation
should make it clear that the list is not exhaustive - more fields might
be added in future releases of the PBAP standard and obexd, without
changing the obexd API.

In SyncEvolution, I use ListFilterFields() to a) do sanity checks on
user input and to b) build a list of all fields from which the user can
exclude specific fields. Because the field list is not hard-coded, that
will work for fields which are not defined yet.

--
Best Regards, Patrick Ohly

The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.



2012-08-28 01:17:08

by Johan Hedberg

[permalink] [raw]
Subject: Re: [PATCH obexd 7/7] client-doc: Update documentation of PhonebookAccess interface

Hi Luiz,

On Fri, Aug 24, 2012, Luiz Augusto von Dentz wrote:
> ---
> doc/client-api.txt | 180 ++++++++++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 145 insertions(+), 35 deletions(-)

I had a chat with Marcel about this API which resulted with the
following change proposals:

> + string Order:
> +
> + Items order
> +
> + Possible values: "indexed" (default),
> + "alphanumeric" or "phonetic"
> +
> + uint16 Offset:
> +
> + Offset of the first item, default is 0
> +
> + uint16 Items:
> +
> + Maximum number of items, default is
> + unlimited (65535)

Call this MaxCount or Count to make it clearer.

> +
> + array{string} Fields:
> +
> + Item vcard fields, default is all
> + values.
> +
> + Possible values:
> +
> + "VERSION",
> + "FN",
> + "N",
> + "PHOTO",
> + "BDAY",
> + "ADR",
> + "LABEL",
> + "TEL",
> + "EMAIL",
> + "MAILER",
> + "TZ",
> + "GEO",
> + "TITLE",
> + "ROLE",
> + "LOGO",
> + "AGENT",
> + "ORG",
> + "NOTE",
> + "REV",
> + "SOUND",
> + "URL",
> + "UID",
> + "KEY",
> + "NICKNAME",
> + "CATEGORIES",
> + "PROID",
> + "CLASS",
> + "SORT-STRING",
> + "X-IRMC-CALL-DATETIME"

Make these lower case (I know it's upper case in the spec. but let's
keep the consistency with the rest of the D-Bus API).

> @@ -244,8 +325,54 @@ Methods void Select(string location, string phonebook)
> The properties of this transfer are also returned along
> with the object path, to avoid a call to GetProperties.
>
> + filters:
> +
> + string Format:
> +
> + Items vcard format
> +
> + Possible values: "vcard21" (default) or
> + "vcard30"
> +
> + array{string} Fields:
> +
> + Item vcard fields, default is all
> + values.
> +
> + Possible values:
> +
> + "VERSION",
> + "FN",
> + "N",
> + "PHOTO",
> + "BDAY",
> + "ADR",
> + "LABEL",
> + "TEL",
> + "EMAIL",
> + "MAILER",
> + "TZ",
> + "GEO",
> + "TITLE",
> + "ROLE",
> + "LOGO",
> + "AGENT",
> + "ORG",
> + "NOTE",
> + "REV",
> + "SOUND",
> + "URL",
> + "UID",
> + "KEY",
> + "NICKNAME",
> + "CATEGORIES",
> + "PROID",
> + "CLASS",
> + "SORT-STRING",
> + "X-IRMC-CALL-DATETIME"

Is this a duplicate of the previous section. Maybe just move this out to
a common section and the refer to it from the places that need it.

Johan

2012-08-24 13:59:50

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: [PATCH obexd 2/7] client: Add filters to PhonebookAccess.Pull

From: Luiz Augusto von Dentz <[email protected]>

This avoid D-Bus round trips and is more aligned with what has been
proposed for MessageAccess interface.
---
client/pbap.c | 79 +++++++++++++++++++++++++++++++++++++++--------------------
1 file changed, 53 insertions(+), 26 deletions(-)

diff --git a/client/pbap.c b/client/pbap.c
index 0c1336c..0c7c08a 100644
--- a/client/pbap.c
+++ b/client/pbap.c
@@ -610,6 +610,7 @@ static DBusMessage *pull_vcard_listing(struct pbap_data *pbap,
pending_request_free(request);

fail:
+ g_obex_apparam_free(apparam);
reply = g_dbus_create_error(message, ERROR_INTERFACE ".Failed", "%s",
err->message);
g_error_free(err);
@@ -802,43 +803,25 @@ static DBusMessage *pbap_pull_all(DBusConnection *connection,
return obc_transfer_create_dbus_reply(transfer, message);
}

-static DBusMessage *pbap_pull_vcard(DBusConnection *connection,
- DBusMessage *message, void *user_data)
+static DBusMessage *pull_vcard(struct pbap_data *pbap, DBusMessage *message,
+ const char *name, const char *targetfile,
+ GObexApparam *apparam)
{
- struct pbap_data *pbap = user_data;
struct obc_transfer *transfer;
- GObexApparam *apparam;
- guint8 buf[32];
- gsize len;
- const char *name, *targetfile;
DBusMessage *reply;
GError *err = NULL;
+ guint8 buf[32];
+ gsize len;

- if (!pbap->path)
- return g_dbus_create_error(message,
- ERROR_INTERFACE ".Forbidden",
- "Call Select first of all");
-
- if (dbus_message_get_args(message, NULL,
- DBUS_TYPE_STRING, &name,
- DBUS_TYPE_STRING, &targetfile,
- DBUS_TYPE_INVALID) == FALSE)
- return g_dbus_create_error(message,
- ERROR_INTERFACE ".InvalidArguments", NULL);
+ len = g_obex_apparam_encode(apparam, buf, sizeof(buf));
+ g_obex_apparam_free(apparam);

transfer = obc_transfer_get("x-bt/vcard", name, targetfile, &err);
if (transfer == NULL)
goto fail;

- apparam = g_obex_apparam_set_uint64(NULL, FILTER_TAG, pbap->filter);
- apparam = g_obex_apparam_set_uint8(apparam, FORMAT_TAG, pbap->format);
-
- len = g_obex_apparam_encode(apparam, buf, sizeof(buf));
-
obc_transfer_set_params(transfer, buf, len);

- g_obex_apparam_free(apparam);
-
if (!obc_session_queue(pbap->session, transfer, NULL, NULL, &err))
goto fail;

@@ -851,6 +834,49 @@ fail:
return reply;
}

+static DBusMessage *pbap_pull_vcard(DBusConnection *connection,
+ DBusMessage *message, void *user_data)
+{
+ struct pbap_data *pbap = user_data;
+ GObexApparam *apparam;
+ const char *name, *targetfile;
+ DBusMessageIter args;
+
+ if (!pbap->path)
+ return g_dbus_create_error(message,
+ ERROR_INTERFACE ".Forbidden",
+ "Call Select first of all");
+
+ dbus_message_iter_init(message, &args);
+
+ if (dbus_message_iter_get_arg_type(&args) != DBUS_TYPE_STRING)
+ return g_dbus_create_error(message,
+ ERROR_INTERFACE ".InvalidArguments", NULL);
+
+ dbus_message_iter_get_basic(&args, &name);
+ dbus_message_iter_next(&args);
+
+ if (dbus_message_iter_get_arg_type(&args) != DBUS_TYPE_STRING)
+ return g_dbus_create_error(message,
+ ERROR_INTERFACE ".InvalidArguments", NULL);
+
+ dbus_message_iter_get_basic(&args, &targetfile);
+ dbus_message_iter_next(&args);
+
+ apparam = g_obex_apparam_set_uint16(NULL, MAXLISTCOUNT_TAG,
+ DEFAULT_COUNT);
+ apparam = g_obex_apparam_set_uint16(apparam, LISTSTARTOFFSET_TAG,
+ DEFAULT_OFFSET);
+
+ if (parse_filters(apparam, &args) == NULL) {
+ g_obex_apparam_free(apparam);
+ return g_dbus_create_error(message,
+ ERROR_INTERFACE ".InvalidArguments", NULL);
+ }
+
+ return pull_vcard(pbap, message, name, targetfile, apparam);
+}
+
static DBusMessage *pbap_list(DBusConnection *connection,
DBusMessage *message, void *user_data)
{
@@ -1058,7 +1084,8 @@ static const GDBusMethodTable pbap_methods[] = {
{ "properties", "a{sv}" }),
pbap_pull_all) },
{ GDBUS_METHOD("Pull",
- GDBUS_ARGS({ "vcard", "s" }, { "targetfile", "s" }),
+ GDBUS_ARGS({ "vcard", "s" }, { "targetfile", "s" },
+ { "filters", "a{sv}" }),
GDBUS_ARGS({ "transfer", "o" },
{ "properties", "a{sv}" }),
pbap_pull_vcard) },
--
1.7.11.4


2012-08-24 13:59:53

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: [PATCH obexd 5/7] test: Update pbap-client to work with changes in PhonebookAcess

From: Luiz Augusto von Dentz <[email protected]>

---
test/pbap-client | 21 +++++++++------------
1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/test/pbap-client b/test/pbap-client
index 498f8a3..7dd54ef 100755
--- a/test/pbap-client
+++ b/test/pbap-client
@@ -70,16 +70,16 @@ class PbapClient:
print "Transfer finished with error %s: %s" % (code, message)
mainloop.quit()

- def pull(self, vcard, func):
+ def pull(self, vcard, params, func):
req = Transfer(func)
- self.pbap.Pull(vcard, "",
+ self.pbap.Pull(vcard, "", params,
reply_handler=lambda r: self.register(r, req),
error_handler=self.error)
self.transfers += 1

- def pull_all(self, func):
+ def pull_all(self, params, func):
req = Transfer(func)
- self.pbap.PullAll("",
+ self.pbap.PullAll("", params,
reply_handler=lambda r: self.register(r, req),
error_handler=self.error)
self.transfers += 1
@@ -135,18 +135,15 @@ if __name__ == '__main__':
print "Size = %d\n" % (ret)

print "\n--- List vCard ---\n"
- ret = pbap_client.interface().List()
+ ret = pbap_client.interface().List(dbus.Dictionary())
+ params = dbus.Dictionary({ "Format" : "vcard30",
+ "Fields" : [ "VERSION", "FN", "TEL"] })
for item in ret:
print "%s : %s" % (item[0], item[1])
- pbap_client.interface().SetFormat("vcard30")
- pbap_client.interface().SetFilter(["VERSION", "FN",
- "TEL"]);
- pbap_client.pull(item[0],
+ pbap_client.pull(item[0], params,
lambda x: process_result(x, None))

- pbap_client.interface().SetFormat("vcard30")
- pbap_client.interface().SetFilter(["VERSION", "FN", "TEL"]);
- pbap_client.pull_all(lambda x: process_result(x,
+ pbap_client.pull_all(params, lambda x: process_result(x,
"\n--- PullAll ---\n"))

pbap_client.flush_transfers(lambda: test_paths(paths[1:]))
--
1.7.11.4


2012-08-24 13:59:54

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: [PATCH obexd 6/7] client: Move common code to pull_phonebook

From: Luiz Augusto von Dentz <[email protected]>

---
client/pbap.c | 79 +++++++++++++++++++++++------------------------------------
1 file changed, 30 insertions(+), 49 deletions(-)

diff --git a/client/pbap.c b/client/pbap.c
index 3e9301a..b460987 100644
--- a/client/pbap.c
+++ b/client/pbap.c
@@ -522,22 +522,29 @@ static GObexApparam *parse_filters(GObexApparam *apparam,
return apparam;
}

-static struct obc_transfer *pull_phonebook(struct pbap_data *pbap,
+static DBusMessage *pull_phonebook(struct pbap_data *pbap,
DBusMessage *message,
- guint8 type, const char *name,
+ guint8 type,
const char *targetfile,
- GObexApparam *apparam,
- GError **err)
+ GObexApparam *apparam)
{
struct pending_request *request;
struct obc_transfer *transfer;
+ char *name;
guint8 buf[32];
gsize len;
session_callback_t func;
+ DBusMessage *reply;
+ GError *err = NULL;
+
+ name = g_strconcat(pbap->path, ".vcf", NULL);
+
+ len = g_obex_apparam_encode(apparam, buf, sizeof(buf));
+ g_obex_apparam_free(apparam);

- transfer = obc_transfer_get("x-bt/phonebook", name, targetfile, err);
+ transfer = obc_transfer_get("x-bt/phonebook", name, targetfile, &err);
if (transfer == NULL)
- return NULL;
+ goto fail;

switch (type) {
case PULLPHONEBOOK:
@@ -553,19 +560,28 @@ static struct obc_transfer *pull_phonebook(struct pbap_data *pbap,
return NULL;
}

- len = g_obex_apparam_encode(apparam, buf, sizeof(buf));
-
obc_transfer_set_params(transfer, buf, len);

- if (!obc_session_queue(pbap->session, transfer, func, request, err)) {
+ if (!obc_session_queue(pbap->session, transfer, func, request, &err)) {
if (request != NULL)
pending_request_free(request);

- return NULL;
+ goto fail;
}

+ g_free(name);
+
+ if (targetfile == NULL)
+ return NULL;

- return transfer;
+ return obc_transfer_create_dbus_reply(transfer, message);
+
+fail:
+ g_free(name);
+ reply = g_dbus_create_error(message, ERROR_INTERFACE ".Failed", "%s",
+ err->message);
+ g_error_free(err);
+ return reply;
}

static DBusMessage *pull_vcard_listing(struct pbap_data *pbap,
@@ -654,11 +670,8 @@ static DBusMessage *pbap_pull_all(DBusConnection *connection,
DBusMessage *message, void *user_data)
{
struct pbap_data *pbap = user_data;
- struct obc_transfer *transfer;
const char *targetfile;
- char *name;
GObexApparam *apparam;
- GError *err = NULL;
DBusMessageIter args;

if (!pbap->path)
@@ -686,22 +699,8 @@ static DBusMessage *pbap_pull_all(DBusConnection *connection,
ERROR_INTERFACE ".InvalidArguments", NULL);
}

- name = g_strconcat(pbap->path, ".vcf", NULL);
-
- transfer = pull_phonebook(pbap, message, PULLPHONEBOOK, name,
- targetfile, apparam, &err);
- g_free(name);
- g_obex_apparam_free(apparam);
-
- if (transfer == NULL) {
- DBusMessage *reply = g_dbus_create_error(message,
- ERROR_INTERFACE ".Failed", "%s",
- err->message);
- g_error_free(err);
- return reply;
- }
-
- return obc_transfer_create_dbus_reply(transfer, message);
+ return pull_phonebook(pbap, message, PULLPHONEBOOK, targetfile,
+ apparam);
}

static DBusMessage *pull_vcard(struct pbap_data *pbap, DBusMessage *message,
@@ -877,11 +876,7 @@ static DBusMessage *pbap_get_size(DBusConnection *connection,
DBusMessage *message, void *user_data)
{
struct pbap_data *pbap = user_data;
- DBusMessage *reply;
- struct obc_transfer *transfer;
- char *name;
GObexApparam *apparam;
- GError *err = NULL;
DBusMessageIter args;

if (!pbap->path)
@@ -891,25 +886,11 @@ static DBusMessage *pbap_get_size(DBusConnection *connection,

dbus_message_iter_init(message, &args);

- name = g_strconcat(pbap->path, ".vcf", NULL);
-
apparam = g_obex_apparam_set_uint16(NULL, MAXLISTCOUNT_TAG, 0);
apparam = g_obex_apparam_set_uint16(apparam, LISTSTARTOFFSET_TAG,
DEFAULT_OFFSET);

- transfer = pull_phonebook(pbap, message, GETPHONEBOOKSIZE, name, NULL,
- apparam, &err);
-
- g_free(name);
- g_obex_apparam_free(apparam);
-
- if (transfer != NULL)
- return NULL;
-
- reply = g_dbus_create_error(message, ERROR_INTERFACE ".Failed", "%s",
- err->message);
- g_error_free(err);
- return reply;
+ return pull_phonebook(pbap, message, GETPHONEBOOKSIZE, NULL, apparam);
}

static const GDBusMethodTable pbap_methods[] = {
--
1.7.11.4


2012-08-24 13:59:55

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: [PATCH obexd 7/7] client-doc: Update documentation of PhonebookAccess interface

From: Luiz Augusto von Dentz <[email protected]>

---
doc/client-api.txt | 180 ++++++++++++++++++++++++++++++++++++++++++-----------
1 file changed, 145 insertions(+), 35 deletions(-)

diff --git a/doc/client-api.txt b/doc/client-api.txt
index 839a78c..adbb12d 100644
--- a/doc/client-api.txt
+++ b/doc/client-api.txt
@@ -206,7 +206,7 @@ Methods void Select(string location, string phonebook)
"mch": missing call history
"cch": combination of ich och mch

- object, dict PullAll(string targetfile)
+ object, dict PullAll(string targetfile, dict filters)

Return the entire phonebook object from the PSE server
in plain string with vcard format, and store it in
@@ -222,14 +222,95 @@ Methods void Select(string location, string phonebook)
The properties of this transfer are also returned along
with the object path, to avoid a call to GetProperties.

- array{string vcard, string name} List()
+ filters:
+
+ string Format:
+
+ Items vcard format
+
+ Possible values: "vcard21" (default) or
+ "vcard30"
+
+ string Order:
+
+ Items order
+
+ Possible values: "indexed" (default),
+ "alphanumeric" or "phonetic"
+
+ uint16 Offset:
+
+ Offset of the first item, default is 0
+
+ uint16 Items:
+
+ Maximum number of items, default is
+ unlimited (65535)
+
+ array{string} Fields:
+
+ Item vcard fields, default is all
+ values.
+
+ Possible values:
+
+ "VERSION",
+ "FN",
+ "N",
+ "PHOTO",
+ "BDAY",
+ "ADR",
+ "LABEL",
+ "TEL",
+ "EMAIL",
+ "MAILER",
+ "TZ",
+ "GEO",
+ "TITLE",
+ "ROLE",
+ "LOGO",
+ "AGENT",
+ "ORG",
+ "NOTE",
+ "REV",
+ "SOUND",
+ "URL",
+ "UID",
+ "KEY",
+ "NICKNAME",
+ "CATEGORIES",
+ "PROID",
+ "CLASS",
+ "SORT-STRING",
+ "X-IRMC-CALL-DATETIME"
+
+ array{string vcard, string name} List(dict filters)

Return an array of vcard-listing data where every entry
consists of a pair of strings containing the vcard
handle and the contact name. For example:
"1.vcf" : "John"

- object, dict Pull(string vcard, string targetfile)
+ filters:
+
+ string Order:
+
+ Items order
+
+ Possible values: "indexed" (default),
+ "alphanumeric" or "phonetic"
+
+ uint16 Offset:
+
+ Offset of the first item, default is 0
+
+ uint16 Items:
+
+ Maximum number of items, default is
+ unlimited (65535)
+
+ object, dict
+ Pull(string vcard, string targetfile, dict filters)

Given a vcard handle, retrieve the vcard in the current
phonebook object and store it in a local file.
@@ -244,8 +325,54 @@ Methods void Select(string location, string phonebook)
The properties of this transfer are also returned along
with the object path, to avoid a call to GetProperties.

+ filters:
+
+ string Format:
+
+ Items vcard format
+
+ Possible values: "vcard21" (default) or
+ "vcard30"
+
+ array{string} Fields:
+
+ Item vcard fields, default is all
+ values.
+
+ Possible values:
+
+ "VERSION",
+ "FN",
+ "N",
+ "PHOTO",
+ "BDAY",
+ "ADR",
+ "LABEL",
+ "TEL",
+ "EMAIL",
+ "MAILER",
+ "TZ",
+ "GEO",
+ "TITLE",
+ "ROLE",
+ "LOGO",
+ "AGENT",
+ "ORG",
+ "NOTE",
+ "REV",
+ "SOUND",
+ "URL",
+ "UID",
+ "KEY",
+ "NICKNAME",
+ "CATEGORIES",
+ "PROID",
+ "CLASS",
+ "SORT-STRING",
+ "X-IRMC-CALL-DATETIME"
+
array{string vcard, string name}
- Search(string field, string value)
+ Search(string field, string value, dict filters)

Search for entries matching the given condition and
return an array of vcard-listing data where every entry
@@ -258,47 +385,30 @@ Methods void Select(string location, string phonebook)
{ "name" (default) | "number" | "sound" }
value : the string value to search for

- uint16 GetSize()
-
- Return the number of entries in the selected phonebook
- object that are actually used (i.e. indexes that
- correspond to non-NULL entries).
-
- void SetFormat(string format)
-
- Indicate the format of the vcard that should be return
- by related methods.
-
- format : { "vcard21" (default) | "vcard30" }
-
- void SetOrder(string order)
+ filters:

- Indicate the sorting method of the vcard-listing data
- returned by List and Search methods.
+ string Order:

- order : { "indexed" (default) | "alphanumeric" |
- "phonetic" }
+ Items order

- void SetFilter(array{string})
+ Possible values: "indexed" (default),
+ "alphanumeric" or "phonetic"

- Indicate fields that should be contained in vcards
- return by related methods.
+ uint16 Offset:

- Give an empty array will clear the filter and return
- all fields available in vcards. And this is the default
- behavior.
+ Offset of the first item, default is 0

- Possible filter fields : "VERSION", "FN", ..., "ALL",
- "bit[0-63]"
+ uint16 Items:

- array{string} ListFilterFields()
+ Maximum number of items, default is
+ unlimited (65535)

- Return All Available fields that can be used in
- SetFilter method.
+ uint16 GetSize()

- array{string} GetFilter()
+ Return the number of entries in the selected phonebook
+ object that are actually used (i.e. indexes that
+ correspond to non-NULL entries).

- Return the current filter setting

Synchronization hierarchy
=======================
--
1.7.11.4


2012-08-24 13:59:52

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: [PATCH obexd 4/7] client: Remove SetOrder, SetFormat and SetFilter from PhonebookAccess

From: Luiz Augusto von Dentz <[email protected]>

Those methods are no longer necessary as other methods now take them
as parameters to avoid round trips.
---
client/pbap.c | 211 ----------------------------------------------------------
1 file changed, 211 deletions(-)

diff --git a/client/pbap.c b/client/pbap.c
index fcc339b..3e9301a 100644
--- a/client/pbap.c
+++ b/client/pbap.c
@@ -116,9 +116,6 @@ static const char *filter_list[] = {
struct pbap_data {
struct obc_session *session;
char *path;
- guint8 format;
- guint8 order;
- uint64_t filter;
};

struct pending_request {
@@ -606,91 +603,6 @@ fail:
return reply;
}

-static int set_format(struct pbap_data *pbap, const char *formatstr)
-{
- if (!formatstr || g_str_equal(formatstr, "")) {
- pbap->format = FORMAT_VCARD21;
- return 0;
- }
-
- if (!g_ascii_strcasecmp(formatstr, "vcard21"))
- pbap->format = FORMAT_VCARD21;
- else if (!g_ascii_strcasecmp(formatstr, "vcard30"))
- pbap->format = FORMAT_VCARD30;
- else
- return -EINVAL;
-
- return 0;
-}
-
-static int set_order(struct pbap_data *pbap, const char *orderstr)
-{
- if (!orderstr || g_str_equal(orderstr, "")) {
- pbap->order = ORDER_INDEXED;
- return 0;
- }
-
- if (!g_ascii_strcasecmp(orderstr, "indexed"))
- pbap->order = ORDER_INDEXED;
- else if (!g_ascii_strcasecmp(orderstr, "alphanumeric"))
- pbap->order = ORDER_ALPHANUMERIC;
- else if (!g_ascii_strcasecmp(orderstr, "phonetic"))
- pbap->order = ORDER_PHONETIC;
- else
- return -EINVAL;
-
- return 0;
-}
-
-static int add_filter(struct pbap_data *pbap, const char *filterstr)
-{
- uint64_t mask;
-
- mask = get_filter_mask(filterstr);
-
- if (mask == 0)
- return -EINVAL;
-
- pbap->filter |= mask;
- return 0;
-}
-
-static int remove_filter(struct pbap_data *pbap, const char *filterstr)
-{
- uint64_t mask;
-
- mask = get_filter_mask(filterstr);
-
- if (mask == 0)
- return -EINVAL;
-
- pbap->filter &= ~mask;
- return 0;
-}
-
-static gchar **get_filter_strs(uint64_t filter, gint *size)
-{
- gchar **list, **item;
- gint i;
- gint filter_list_size = sizeof(filter_list) / sizeof(filter_list[0]) - 1;
-
- list = g_malloc0(sizeof(gchar **) * (FILTER_BIT_MAX + 2));
-
- item = list;
-
- for (i = 0; i < filter_list_size; i++)
- if (filter & (1ULL << i))
- *(item++) = g_strdup(filter_list[i]);
-
- for (i = filter_list_size; i <= FILTER_BIT_MAX; i++)
- if (filter & (1ULL << i))
- *(item++) = g_strdup_printf("%s%d", "BIT", i);
-
- *item = NULL;
- *size = item - list;
- return list;
-}
-
static DBusMessage *pbap_select(DBusConnection *connection,
DBusMessage *message, void *user_data)
{
@@ -1000,114 +912,6 @@ static DBusMessage *pbap_get_size(DBusConnection *connection,
return reply;
}

-static DBusMessage *pbap_set_format(DBusConnection *connection,
- DBusMessage *message, void *user_data)
-{
- struct pbap_data *pbap = user_data;
- const char *format;
-
- if (dbus_message_get_args(message, NULL,
- DBUS_TYPE_STRING, &format,
- DBUS_TYPE_INVALID) == FALSE)
- return g_dbus_create_error(message,
- ERROR_INTERFACE ".InvalidArguments", NULL);
-
- if (set_format(pbap, format) < 0)
- return g_dbus_create_error(message,
- ERROR_INTERFACE ".InvalidArguments",
- "InvalidFormat");
-
- return dbus_message_new_method_return(message);
-}
-
-static DBusMessage *pbap_set_order(DBusConnection *connection,
- DBusMessage *message, void *user_data)
-{
- struct pbap_data *pbap = user_data;
- const char *order;
-
- if (dbus_message_get_args(message, NULL,
- DBUS_TYPE_STRING, &order,
- DBUS_TYPE_INVALID) == FALSE)
- return g_dbus_create_error(message,
- ERROR_INTERFACE ".InvalidArguments", NULL);
-
- if (set_order(pbap, order) < 0)
- return g_dbus_create_error(message,
- ERROR_INTERFACE ".InvalidArguments",
- "InvalidFilter");
-
- return dbus_message_new_method_return(message);
-}
-
-static DBusMessage *pbap_set_filter(DBusConnection *connection,
- DBusMessage *message, void *user_data)
-{
- struct pbap_data *pbap = user_data;
- char **filters, **item;
- gint size;
- uint64_t oldfilter = pbap->filter;
-
- if (dbus_message_get_args(message, NULL, DBUS_TYPE_ARRAY,
- DBUS_TYPE_STRING, &filters, &size,
- DBUS_TYPE_INVALID) == FALSE)
- return g_dbus_create_error(message,
- ERROR_INTERFACE ".InvalidArguments", NULL);
-
- remove_filter(pbap, "ALL");
- if (size == 0)
- goto done;
-
- for (item = filters; *item; item++) {
- if (add_filter(pbap, *item) < 0) {
- pbap->filter = oldfilter;
- g_strfreev(filters);
- return g_dbus_create_error(message,
- ERROR_INTERFACE ".InvalidArguments",
- "InvalidFilters");
- }
- }
-
-done:
- g_strfreev(filters);
- return dbus_message_new_method_return(message);
-}
-
-static DBusMessage *pbap_get_filter(DBusConnection *connection,
- DBusMessage *message, void *user_data)
-{
- struct pbap_data *pbap = user_data;
- gchar **filters = NULL;
- gint size;
- DBusMessage *reply;
-
- filters = get_filter_strs(pbap->filter, &size);
- reply = dbus_message_new_method_return(message);
- dbus_message_append_args(reply, DBUS_TYPE_ARRAY,
- DBUS_TYPE_STRING, &filters, size,
- DBUS_TYPE_INVALID);
-
- g_strfreev(filters);
- return reply;
-}
-
-static DBusMessage *pbap_list_filter_fields(DBusConnection *connection,
- DBusMessage *message, void *user_data)
-{
- gchar **filters = NULL;
- gint size;
- DBusMessage *reply;
-
- filters = get_filter_strs(FILTER_ALL, &size);
- reply = dbus_message_new_method_return(message);
- dbus_message_append_args(reply, DBUS_TYPE_ARRAY,
- DBUS_TYPE_STRING, &filters, size,
- DBUS_TYPE_INVALID);
-
- g_strfreev(filters);
- return reply;
-}
-
static const GDBusMethodTable pbap_methods[] = {
{ GDBUS_ASYNC_METHOD("Select",
GDBUS_ARGS({ "location", "s" }, { "phonebook", "s" }),
@@ -1136,21 +940,6 @@ static const GDBusMethodTable pbap_methods[] = {
{ GDBUS_ASYNC_METHOD("GetSize",
NULL, GDBUS_ARGS({ "size", "q" }),
pbap_get_size) },
- { GDBUS_METHOD("SetFormat",
- GDBUS_ARGS({ "format", "s" }), NULL,
- pbap_set_format) },
- { GDBUS_METHOD("SetOrder",
- GDBUS_ARGS({ "order", "s" }), NULL,
- pbap_set_order) },
- { GDBUS_METHOD("SetFilter",
- GDBUS_ARGS({ "fields", "as" }), NULL,
- pbap_set_filter) },
- { GDBUS_METHOD("GetFilter",
- NULL, GDBUS_ARGS({ "fields", "as" }),
- pbap_get_filter) },
- { GDBUS_METHOD("ListFilterFields",
- NULL, GDBUS_ARGS({ "fields", "as" }),
- pbap_list_filter_fields) },
{ }
};

--
1.7.11.4


2012-08-24 13:59:51

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: [PATCH obexd 3/7] client: Add filters to PhonebookAccess.List and PhonebookAccess.Search

From: Luiz Augusto von Dentz <[email protected]>

This avoid D-Bus round trips and is more aligned with what has been
proposed for MessageAccess interface.
---
client/pbap.c | 115 ++++++++++++++++++++++++++++++++++++++--------------------
1 file changed, 76 insertions(+), 39 deletions(-)

diff --git a/client/pbap.c b/client/pbap.c
index 0c7c08a..fcc339b 100644
--- a/client/pbap.c
+++ b/client/pbap.c
@@ -573,35 +573,24 @@ static struct obc_transfer *pull_phonebook(struct pbap_data *pbap,

static DBusMessage *pull_vcard_listing(struct pbap_data *pbap,
DBusMessage *message, const char *name,
- guint8 order, char *searchval, guint8 attrib,
- guint16 count, guint16 offset)
+ GObexApparam *apparam)
{
struct pending_request *request;
struct obc_transfer *transfer;
guint8 buf[272];
gsize len;
GError *err = NULL;
- GObexApparam *apparam;
DBusMessage *reply;

+ len = g_obex_apparam_encode(apparam, buf, sizeof(buf));
+ g_obex_apparam_free(apparam);
+
transfer = obc_transfer_get("x-bt/vcard-listing", name, NULL, &err);
if (transfer == NULL)
goto fail;

- apparam = g_obex_apparam_set_uint8(NULL, ORDER_TAG, order);
- apparam = g_obex_apparam_set_uint8(apparam, SEARCHATTRIB_TAG, attrib);
- apparam = g_obex_apparam_set_string(apparam, SEARCHVALUE_TAG,
- searchval);
- apparam = g_obex_apparam_set_uint16(apparam, MAXLISTCOUNT_TAG, count);
- apparam = g_obex_apparam_set_uint16(apparam, LISTSTARTOFFSET_TAG,
- offset);
-
- len = g_obex_apparam_encode(apparam, buf, sizeof(buf));
-
obc_transfer_set_params(transfer, buf, len);

- g_obex_apparam_free(apparam);
-
request = pending_request_new(pbap, message);
if (obc_session_queue(pbap->session, transfer,
pull_vcard_listing_callback, request, &err))
@@ -881,34 +870,33 @@ static DBusMessage *pbap_list(DBusConnection *connection,
DBusMessage *message, void *user_data)
{
struct pbap_data *pbap = user_data;
+ GObexApparam *apparam;
+ DBusMessageIter args;

if (!pbap->path)
return g_dbus_create_error(message,
ERROR_INTERFACE ".Forbidden",
"Call Select first of all");

- return pull_vcard_listing(pbap, message, "", pbap->order, "",
- ATTRIB_NAME, DEFAULT_COUNT, DEFAULT_OFFSET);
-}
+ dbus_message_iter_init(message, &args);

-static DBusMessage *pbap_search(DBusConnection *connection,
- DBusMessage *message, void *user_data)
-{
- struct pbap_data *pbap = user_data;
- char *field, *value;
- guint8 attrib;
+ apparam = g_obex_apparam_set_uint16(NULL, MAXLISTCOUNT_TAG,
+ DEFAULT_COUNT);
+ apparam = g_obex_apparam_set_uint16(apparam, LISTSTARTOFFSET_TAG,
+ DEFAULT_OFFSET);

- if (dbus_message_get_args(message, NULL,
- DBUS_TYPE_STRING, &field,
- DBUS_TYPE_STRING, &value,
- DBUS_TYPE_INVALID) == FALSE)
+ if (parse_filters(apparam, &args) == NULL) {
+ g_obex_apparam_free(apparam);
return g_dbus_create_error(message,
ERROR_INTERFACE ".InvalidArguments", NULL);
+ }

- if (!pbap->path)
- return g_dbus_create_error(message,
- ERROR_INTERFACE ".Forbidden",
- "Call Select first of all");
+ return pull_vcard_listing(pbap, message, "", apparam);
+}
+
+static GObexApparam *parse_attribute(GObexApparam *apparam, const char *field)
+{
+ guint8 attrib;

if (!field || g_str_equal(field, ""))
attrib = ATTRIB_NAME;
@@ -919,11 +907,58 @@ static DBusMessage *pbap_search(DBusConnection *connection,
else if (!g_ascii_strcasecmp(field, "sound"))
attrib = ATTRIB_SOUND;
else
+ return NULL;
+
+ return g_obex_apparam_set_uint8(apparam, SEARCHATTRIB_TAG, attrib);
+}
+
+static DBusMessage *pbap_search(DBusConnection *connection,
+ DBusMessage *message, void *user_data)
+{
+ struct pbap_data *pbap = user_data;
+ char *field, *value;
+ GObexApparam *apparam;
+ DBusMessageIter args;
+
+ if (!pbap->path)
+ return g_dbus_create_error(message,
+ ERROR_INTERFACE ".Forbidden",
+ "Call Select first of all");
+
+ dbus_message_iter_init(message, &args);
+
+ if (dbus_message_iter_get_arg_type(&args) != DBUS_TYPE_STRING)
+ return g_dbus_create_error(message,
+ ERROR_INTERFACE ".InvalidArguments", NULL);
+
+ dbus_message_iter_get_basic(&args, &field);
+ dbus_message_iter_next(&args);
+
+ apparam = parse_attribute(NULL, field);
+ if (apparam == NULL)
+ return g_dbus_create_error(message,
+ ERROR_INTERFACE ".InvalidArguments", NULL);
+
+ if (dbus_message_iter_get_arg_type(&args) != DBUS_TYPE_STRING)
+ return g_dbus_create_error(message,
+ ERROR_INTERFACE ".InvalidArguments", NULL);
+
+ dbus_message_iter_get_basic(&args, &value);
+ dbus_message_iter_next(&args);
+
+ apparam = g_obex_apparam_set_uint16(apparam, MAXLISTCOUNT_TAG,
+ DEFAULT_COUNT);
+ apparam = g_obex_apparam_set_uint16(apparam, LISTSTARTOFFSET_TAG,
+ DEFAULT_OFFSET);
+ apparam = g_obex_apparam_set_string(apparam, SEARCHVALUE_TAG, value);
+
+ if (parse_filters(apparam, &args) == NULL) {
+ g_obex_apparam_free(apparam);
return g_dbus_create_error(message,
ERROR_INTERFACE ".InvalidArguments", NULL);
+ }

- return pull_vcard_listing(pbap, message, "", pbap->order, value,
- attrib, DEFAULT_COUNT, DEFAULT_OFFSET);
+ return pull_vcard_listing(pbap, message, "", apparam);
}

static DBusMessage *pbap_get_size(DBusConnection *connection,
@@ -1090,12 +1125,14 @@ static const GDBusMethodTable pbap_methods[] = {
{ "properties", "a{sv}" }),
pbap_pull_vcard) },
{ GDBUS_ASYNC_METHOD("List",
- NULL, GDBUS_ARGS({ "vcard_listing", "a(ss)" }),
- pbap_list) },
+ GDBUS_ARGS({ "filters", "a{sv}" }),
+ GDBUS_ARGS({ "vcard_listing", "a(ss)" }),
+ pbap_list) },
{ GDBUS_ASYNC_METHOD("Search",
- GDBUS_ARGS({ "field", "s" }, { "value", "s" }),
- GDBUS_ARGS({ "vcard_listing", "a(ss)" }),
- pbap_search) },
+ GDBUS_ARGS({ "field", "s" }, { "value", "s" },
+ { "filters", "a{sv}" }),
+ GDBUS_ARGS({ "vcard_listing", "a(ss)" }),
+ pbap_search) },
{ GDBUS_ASYNC_METHOD("GetSize",
NULL, GDBUS_ARGS({ "size", "q" }),
pbap_get_size) },
--
1.7.11.4