[PATCH] Added empty VCARD N: parameter handling

[PATCH] Added empty VCARD N: parameter handling

[ext-jablonski.radoslaw]

[-- Attachment #1: Type: text/plain, Size: 610 bytes --]

Some of the devices are expecting that N: parameter in VCARD is always filled (by example Nokia BH-903)
When this field is empty (N:;;;;) then list of dialed/incoming calls on carkit is useless - carkit then shows only blank lines and it's impossible to determine who made call ( phone number are invisible too in this case)
    
If none of the contact fields is available, then setting telephone number as the first attribute for "N:" parameter.
Carkit will see that number as contact name - it is only used in case when none of more detailed contact information is available on the phone.

BR,
Radek

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Added-empty-N-parameter-handling-in-VCARD.patch --]
[-- Type: text/x-patch; name="0001-Added-empty-N-parameter-handling-in-VCARD.patch", Size: 1946 bytes --]

From 6d88e3d7c1a5014e60ca8f53f7163e3a51148530 Mon Sep 17 00:00:00 2001
From: Radoslaw Jablonski <ext-jablonski.radoslaw@nokia.com>
Date: Wed, 7 Jul 2010 15:07:58 +0300
Subject: [PATCH] Added empty N: parameter handling in VCARD

Some of the devices are expecting that N: parameter in VCARD is always filled (by example Nokia BH-903)
When this field is empty (N:;;;;) then list of dialed/incoming calls on carkit is useless.

If none of fields is available then setting telephone number as the first attribute for "N:" parameter.
Carkit will see that number as contact name - it is only used in case when none of more detailed contact information is available on phone.
---
 plugins/vcard.c |   17 ++++++++++++++---
 1 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/plugins/vcard.c b/plugins/vcard.c
index 5948a4a..b2ab30a 100644
--- a/plugins/vcard.c
+++ b/plugins/vcard.c
@@ -136,9 +136,20 @@ static void vcard_printf_begin(GString *vcards, uint8_t format)
 static void vcard_printf_name(GString *vcards,
 					struct phonebook_contact *contact)
 {
-	vcard_printf(vcards, "N:%s;%s;%s;%s;%s", contact->family,
-				contact->given, contact->additional,
-				contact->prefix, contact->suffix);
+	/* at least one of fields is present */
+	if ((contact->family && strlen(contact->family)) ||
+		(contact->given && strlen (contact->given)) ||
+		(contact->additional && strlen(contact->additional)) ||
+		(contact->prefix && strlen (contact->prefix)) ||
+		(contact->suffix && strlen (contact->suffix)))
+		vcard_printf(vcards, "N:%s;%s;%s;%s;%s", contact->family,
+						contact->given, contact->additional,
+						contact->prefix, contact->suffix);
+	else {
+		/* if all fields are empty we're using  first phone number as name */
+		struct phonebook_number *number = contact->numbers->data;
+		vcard_printf(vcards, "N:%s;;;;", number->tel);
+	}
 }
 
 static void vcard_printf_fullname(GString *vcards, const char *text)
-- 
1.6.0.4

Re: [PATCH] Added empty VCARD N: parameter handling

[Marcel Holtmann]

Hi Radek,

using git send-email would be a bit better here. Then we have the
patches inline.

> From 6d88e3d7c1a5014e60ca8f53f7163e3a51148530 Mon Sep 17 00:00:00 2001
> From: Radoslaw Jablonski <ext-jablonski.radoslaw@nokia.com>
> Date: Wed, 7 Jul 2010 15:07:58 +0300
> Subject: [PATCH] Added empty N: parameter handling in VCARD
> 
> Some of the devices are expecting that N: parameter in VCARD is always filled (by example Nokia BH-903)
> When this field is empty (N:;;;;) then list of dialed/incoming calls on carkit is useless.
> 
> If none of fields is available then setting telephone number as the first attribute for "N:" parameter.
> Carkit will see that number as contact name - it is only used in case when none of more detailed contact information is available on phone.
> ---
>  plugins/vcard.c |   17 ++++++++++++++---
>  1 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/plugins/vcard.c b/plugins/vcard.c
> index 5948a4a..b2ab30a 100644
> --- a/plugins/vcard.c
> +++ b/plugins/vcard.c
> @@ -136,9 +136,20 @@ static void vcard_printf_begin(GString *vcards, uint8_t format)
>  static void vcard_printf_name(GString *vcards,
>                                         struct phonebook_contact *contact)
>  {
> -       vcard_printf(vcards, "N:%s;%s;%s;%s;%s", contact->family,
> -                               contact->given, contact->additional,
> -                               contact->prefix, contact->suffix);
> +       /* at least one of fields is present */
> +       if ((contact->family && strlen(contact->family)) ||
> +               (contact->given && strlen (contact->given)) ||
> +               (contact->additional && strlen(contact->additional)) ||
> +               (contact->prefix && strlen (contact->prefix)) ||
> +               (contact->suffix && strlen (contact->suffix)))
> +               vcard_printf(vcards, "N:%s;%s;%s;%s;%s", contact->family,
> +                                               contact->given, contact->additional,
> +                                               contact->prefix, contact->suffix);

The extra if clauses require two level of indentation. Otherwise you
can't tell which is part of the if clause and which is code.

Also it is strlen(...). No extra space.

And I would prefer to split this into an extra function just doing this
test. This if clause is too big.

> +       else {
> +               /* if all fields are empty we're using  first phone number as name */
> +               struct phonebook_number *number = contact->numbers->data;
> +               vcard_printf(vcards, "N:%s;;;;", number->tel);
> +       }
>  }

So I prefer the whole thing like this:

	/* if all fields are empty we're using  first phone number as name */
	if (required_fields_present(contact) == FALSE) {
		struct phonebook_number *number = contact->numbers->data;
		vcard_printf(vcards, "N:%s;;;;", number->tel);
		return;
	}

	vcard_printf(vcards, "N:%s;%s;%s;%s;%s", contact->family,
		...

This way it is a lot easier to read and understand what is the special
case and what would be the default.

Regards

Marcel

RE: [PATCH] Added empty VCARD N: parameter handling

[ext-jablonski.radoslaw]

Hi!
I've made necessary changes to the code. Now it should look much better:)
New version of patch is inserted below.

>From 45dfcf8f9dfacd8937a1a1d14146bd0da04eca25 Mon Sep 17 00:00:00 2001
From: Radoslaw Jablonski <ext-jablonski.radoslaw@nokia.com>
Date: Wed, 7 Jul 2010 17:18:31 +0300
Subject: [PATCH] Added empty VCARD N: parameter handling

Some of the devices are expecting that N: parameter in VCARD is always fill=
ed (by example Nokia BH-903)
When this field is empty (N:;;;;) then list of dialed/incoming calls on car=
kit is useless - carkit then shows only blank lines and it's impossible to =
determine who made call ( phone number are invisible too in this case)

If none of the contact fields is available, then setting telephone number a=
s the first attribute for "N:" parameter.
Carkit will see that number as contact name - it is only used in case when =
none of more detailed contact information is available on the phone.
---
 plugins/vcard.c |   21 +++++++++++++++++++++
 1 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/plugins/vcard.c b/plugins/vcard.c
index 5948a4a..ab1349c 100644
--- a/plugins/vcard.c
+++ b/plugins/vcard.c
@@ -123,6 +123,20 @@ static void add_slash(char *dest, const char *src, int=
 len_max, int len)
 	return;
 }
=20
+/* checks if there is at least one present contact field with personal dat=
a */
+static gboolean contact_fields_present( struct phonebook_contact * contact=
)
+{
+	if ((contact->family && strlen(contact->family)) ||
+			(contact->given && strlen(contact->given)) ||
+			(contact->additional && strlen(contact->additional)) ||
+			(contact->prefix && strlen(contact->prefix)) ||
+			(contact->suffix && strlen(contact->suffix)))
+		return TRUE;
+
+	/* none of the personal data fields is present*/
+	return FALSE;
+}
+
 static void vcard_printf_begin(GString *vcards, uint8_t format)
 {
 	vcard_printf(vcards, "BEGIN:VCARD");
@@ -136,6 +150,13 @@ static void vcard_printf_begin(GString *vcards, uint8_=
t format)
 static void vcard_printf_name(GString *vcards,
 					struct phonebook_contact *contact)
 {
+	if (contact_fields_present(contact) =3D=3D FALSE) {
+		/* if all fields are empty we're using first phone number as name */
+		struct phonebook_number *number =3D contact->numbers->data;
+		vcard_printf(vcards, "N:%s;;;;", number->tel);
+		return;
+	}
+
 	vcard_printf(vcards, "N:%s;%s;%s;%s;%s", contact->family,
 				contact->given, contact->additional,
 				contact->prefix, contact->suffix);
--=20
1.6.0.4


BR,
Radek
________________________________________
From: ext Marcel Holtmann [marcel@holtmann.org]
Sent: Wednesday, July 07, 2010 4:16 PM
To: Radoslaw Jablonski (EXT-Comarch/Helsinki)
Cc: linux-bluetooth@vger.kernel.org
Subject: Re: [PATCH] Added empty VCARD N: parameter handling

Hi Radek,

using git send-email would be a bit better here. Then we have the
patches inline.

> From 6d88e3d7c1a5014e60ca8f53f7163e3a51148530 Mon Sep 17 00:00:00 2001
> From: Radoslaw Jablonski <ext-jablonski.radoslaw@nokia.com>
> Date: Wed, 7 Jul 2010 15:07:58 +0300
> Subject: [PATCH] Added empty N: parameter handling in VCARD
>
> Some of the devices are expecting that N: parameter in VCARD is always fi=
lled (by example Nokia BH-903)
> When this field is empty (N:;;;;) then list of dialed/incoming calls on c=
arkit is useless.
>
> If none of fields is available then setting telephone number as the first=
 attribute for "N:" parameter.
> Carkit will see that number as contact name - it is only used in case whe=
n none of more detailed contact information is available on phone.
> ---
>  plugins/vcard.c |   17 ++++++++++++++---
>  1 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/plugins/vcard.c b/plugins/vcard.c
> index 5948a4a..b2ab30a 100644
> --- a/plugins/vcard.c
> +++ b/plugins/vcard.c
> @@ -136,9 +136,20 @@ static void vcard_printf_begin(GString *vcards, uint=
8_t format)
>  static void vcard_printf_name(GString *vcards,
>                                         struct phonebook_contact *contact=
)
>  {
> -       vcard_printf(vcards, "N:%s;%s;%s;%s;%s", contact->family,
> -                               contact->given, contact->additional,
> -                               contact->prefix, contact->suffix);
> +       /* at least one of fields is present */
> +       if ((contact->family && strlen(contact->family)) ||
> +               (contact->given && strlen (contact->given)) ||
> +               (contact->additional && strlen(contact->additional)) ||
> +               (contact->prefix && strlen (contact->prefix)) ||
> +               (contact->suffix && strlen (contact->suffix)))
> +               vcard_printf(vcards, "N:%s;%s;%s;%s;%s", contact->family,
> +                                               contact->given, contact->=
additional,
> +                                               contact->prefix, contact-=
>suffix);

The extra if clauses require two level of indentation. Otherwise you
can't tell which is part of the if clause and which is code.

Also it is strlen(...). No extra space.

And I would prefer to split this into an extra function just doing this
test. This if clause is too big.

> +       else {
> +               /* if all fields are empty we're using  first phone numbe=
r as name */
> +               struct phonebook_number *number =3D contact->numbers->dat=
a;
> +               vcard_printf(vcards, "N:%s;;;;", number->tel);
> +       }
>  }

So I prefer the whole thing like this:

        /* if all fields are empty we're using  first phone number as name =
*/
        if (required_fields_present(contact) =3D=3D FALSE) {
                struct phonebook_number *number =3D contact->numbers->data;
                vcard_printf(vcards, "N:%s;;;;", number->tel);
                return;
        }

        vcard_printf(vcards, "N:%s;%s;%s;%s;%s", contact->family,
                ...

This way it is a lot easier to read and understand what is the special
case and what would be the default.

Regards

Marcel

RE: [PATCH] Added empty VCARD N: parameter handling

[Marcel Holtmann]

Hi,

> I've made necessary changes to the code. Now it should look much better:)
> New version of patch is inserted below.

and please use git send-email. Otherwise we can't apply it with git am.

> From 45dfcf8f9dfacd8937a1a1d14146bd0da04eca25 Mon Sep 17 00:00:00 2001
> From: Radoslaw Jablonski <ext-jablonski.radoslaw@nokia.com>
> Date: Wed, 7 Jul 2010 17:18:31 +0300
> Subject: [PATCH] Added empty VCARD N: parameter handling
> 
> Some of the devices are expecting that N: parameter in VCARD is always filled (by example Nokia BH-903)
> When this field is empty (N:;;;;) then list of dialed/incoming calls on carkit is useless - carkit then shows only blank lines and it's impossible to determine who made call ( phone number are invisible too in this case)
> 
> If none of the contact fields is available, then setting telephone number as the first attribute for "N:" parameter.
> Carkit will see that number as contact name - it is only used in case when none of more detailed contact information is available on the phone.
> ---
>  plugins/vcard.c |   21 +++++++++++++++++++++
>  1 files changed, 21 insertions(+), 0 deletions(-)
> 
> diff --git a/plugins/vcard.c b/plugins/vcard.c
> index 5948a4a..ab1349c 100644
> --- a/plugins/vcard.c
> +++ b/plugins/vcard.c
> @@ -123,6 +123,20 @@ static void add_slash(char *dest, const char *src, int len_max, int len)
>  	return;
>  }
>  
> +/* checks if there is at least one present contact field with personal data */
> +static gboolean contact_fields_present( struct phonebook_contact * contact)
> +{

It is (struct ... contact *contact). See other code for proper placement
of whitespaces.

> +	if ((contact->family && strlen(contact->family)) ||
> +			(contact->given && strlen(contact->given)) ||
> +			(contact->additional && strlen(contact->additional)) ||
> +			(contact->prefix && strlen(contact->prefix)) ||
> +			(contact->suffix && strlen(contact->suffix)))
> +		return TRUE;
> +
> +	/* none of the personal data fields is present*/
> +	return FALSE;
> +}

	if (contact->family && strlen(contact->family) > 0)
		return TRUE;

	if (contact->given && strlen(contact->given) > 0)
		return TRUE;

	...

	return FALSE;

That is a more readable version of this function.

> +
>  static void vcard_printf_begin(GString *vcards, uint8_t format)
>  {
>  	vcard_printf(vcards, "BEGIN:VCARD");
> @@ -136,6 +150,13 @@ static void vcard_printf_begin(GString *vcards, uint8_t format)
>  static void vcard_printf_name(GString *vcards,
>  					struct phonebook_contact *contact)
>  {
> +	if (contact_fields_present(contact) == FALSE) {
> +		/* if all fields are empty we're using first phone number as name */
> +		struct phonebook_number *number = contact->numbers->data;
> +		vcard_printf(vcards, "N:%s;;;;", number->tel);
> +		return;
> +	}
> +
>  	vcard_printf(vcards, "N:%s;%s;%s;%s;%s", contact->family,
>  				contact->given, contact->additional,
>  				contact->prefix, contact->suffix);

Regards

Marcel

RE: [PATCH] Added empty VCARD N: parameter handling

[ext-jablonski.radoslaw]

Hi!
I've realized that previous solution was little conflicted with PBAP spec:)
Now I've found real source of the problem with generating VCARDS( for empty=
 N: parameter no unnecessary characters should be inserted - and in current=
 code there was 4 semicolons in that place)
For BH-903 it works great - now I need to make some testing with various ca=
rkits.=20
If everything will work as expected then final patch will be available tomm=
orow morning.

BR,
Radek
________________________________________
From: ext Marcel Holtmann [marcel@holtmann.org]
Sent: Wednesday, July 07, 2010 11:25 PM
To: Radoslaw Jablonski (EXT-Comarch/Helsinki)
Cc: linux-bluetooth@vger.kernel.org
Subject: RE: [PATCH] Added empty VCARD N: parameter handling

Hi,

> I've made necessary changes to the code. Now it should look much better:)
> New version of patch is inserted below.

and please use git send-email. Otherwise we can't apply it with git am.

> From 45dfcf8f9dfacd8937a1a1d14146bd0da04eca25 Mon Sep 17 00:00:00 2001
> From: Radoslaw Jablonski <ext-jablonski.radoslaw@nokia.com>
> Date: Wed, 7 Jul 2010 17:18:31 +0300
> Subject: [PATCH] Added empty VCARD N: parameter handling
>
> Some of the devices are expecting that N: parameter in VCARD is always fi=
lled (by example Nokia BH-903)
> When this field is empty (N:;;;;) then list of dialed/incoming calls on c=
arkit is useless - carkit then shows only blank lines and it's impossible t=
o determine who made call ( phone number are invisible too in this case)
>
> If none of the contact fields is available, then setting telephone number=
 as the first attribute for "N:" parameter.
> Carkit will see that number as contact name - it is only used in case whe=
n none of more detailed contact information is available on the phone.
> ---
>  plugins/vcard.c |   21 +++++++++++++++++++++
>  1 files changed, 21 insertions(+), 0 deletions(-)
>
> diff --git a/plugins/vcard.c b/plugins/vcard.c
> index 5948a4a..ab1349c 100644
> --- a/plugins/vcard.c
> +++ b/plugins/vcard.c
> @@ -123,6 +123,20 @@ static void add_slash(char *dest, const char *src, i=
nt len_max, int len)
>       return;
>  }
>
> +/* checks if there is at least one present contact field with personal d=
ata */
> +static gboolean contact_fields_present( struct phonebook_contact * conta=
ct)
> +{

It is (struct ... contact *contact). See other code for proper placement
of whitespaces.

> +     if ((contact->family && strlen(contact->family)) ||
> +                     (contact->given && strlen(contact->given)) ||
> +                     (contact->additional && strlen(contact->additional)=
) ||
> +                     (contact->prefix && strlen(contact->prefix)) ||
> +                     (contact->suffix && strlen(contact->suffix)))
> +             return TRUE;
> +
> +     /* none of the personal data fields is present*/
> +     return FALSE;
> +}

        if (contact->family && strlen(contact->family) > 0)
                return TRUE;

        if (contact->given && strlen(contact->given) > 0)
                return TRUE;

        ...

        return FALSE;

That is a more readable version of this function.

> +
>  static void vcard_printf_begin(GString *vcards, uint8_t format)
>  {
>       vcard_printf(vcards, "BEGIN:VCARD");
> @@ -136,6 +150,13 @@ static void vcard_printf_begin(GString *vcards, uint=
8_t format)
>  static void vcard_printf_name(GString *vcards,
>                                       struct phonebook_contact *contact)
>  {
> +     if (contact_fields_present(contact) =3D=3D FALSE) {
> +             /* if all fields are empty we're using first phone number a=
s name */
> +             struct phonebook_number *number =3D contact->numbers->data;
> +             vcard_printf(vcards, "N:%s;;;;", number->tel);
> +             return;
> +     }
> +
>       vcard_printf(vcards, "N:%s;%s;%s;%s;%s", contact->family,
>                               contact->given, contact->additional,
>                               contact->prefix, contact->suffix);

Regards

Marcel=