Return-Path: <cyrus@holtmann.org>
Subject: Re: [PATCH] Added empty VCARD N: parameter handling
From: Marcel Holtmann <marcel@holtmann.org>
To: ext-jablonski.radoslaw@nokia.com
Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <090FE800A758CA439B2752C082AC3DEF037828C920@NOK-EUMSG-06.mgdnok.nokia.com>
References: <090FE800A758CA439B2752C082AC3DEF037828C920@NOK-EUMSG-06.mgdnok.nokia.com>
Content-Type: text/plain; charset="UTF-8"
Date: Wed, 07 Jul 2010 10:16:53 -0300
Message-ID: <1278508613.2789.114.camel@localhost.localdomain>
Mime-Version: 1.0
Sender: linux-bluetooth-owner@vger.kernel.org
List-ID: <linux-bluetooth.vger.kernel.org>

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