Hi Lukasz,
On Mon, Aug 23, 2010, Lukasz Pawlik wrote:
> From: Lukasz Pawlik <[email protected]>
> Date: Mon, 23 Aug 2010 15:25:20 +0200
> Subject: [PATCH] Fix issues with emails category
>
> Previously all emails sent during phonebook pull had the same
> category INTERNET. Now email are sent with valid category name
> (INTERNET;HOME or INTERNET;WORK)
> ---
> plugins/phonebook-tracker.c | 31 +++++++++++++++++--------
> plugins/vcard.c | 53 ++++++++++++++++++++++++++++++++++---------
> plugins/vcard.h | 11 +++++++++
> 3 files changed, 74 insertions(+), 21 deletions(-)
Thanks. The patch is now pushed upstream, but I had to make a few coding
style fixes before that:
> +static struct phonebook_email *find_email(GSList *emails, const char *address,
> + int type)
Over 80 column line.
> + for (l = emails; l; l = l->next) {
> + struct phonebook_email *email;
> + email = l->data;
You don't really need the variable definition and assignment on
different lines here.
> +static void add_email(struct phonebook_contact *contact, const char *address,
> + int type)
The continuation line should be indented as much as possible as long as
you don't go beyond 80 columns. (not sure if this is a kernel coding
style thingie but at least Marcel wants it that way).
> +static void vcard_printf_email(GString *vcards, uint8_t format,
> + const char *address,
> + enum phonebook_email_type category)
Way over 80 column lines. Are you using some other tab-width than 8?
Johan
Hallo Lukasz,
On Mon, Aug 23, 2010 at 01:18:11PM +0200, Lukasz Pawlik wrote:
> From a537ade2e108a91552e0d9a6c4ff71ca35388e10 Mon Sep 17 00:00:00 2001
> From: Lukasz Pawlik <[email protected]>
> Date: Mon, 23 Aug 2010 12:57:40 +0200
> Subject: [PATCH] Fix issues with emails category
>
> Previously all emails sent during phonebook pull had the same
> category INTERNET. Now email are sent with valid category name
> (INTERNET;HOME or INTERNET;WORK)
> ---
> plugins/phonebook-tracker.c | 30 +++++++++++++++++-------
> plugins/vcard.c | 53 ++++++++++++++++++++++++++++++++++---------
> plugins/vcard.h | 11 +++++++++
> 3 files changed, 74 insertions(+), 20 deletions(-)
>
> diff --git a/plugins/phonebook-tracker.c b/plugins/phonebook-tracker.c
> index 3f63dcb..069e324 100644
> --- a/plugins/phonebook-tracker.c
> +++ b/plugins/phonebook-tracker.c
> @@ -687,27 +687,39 @@ static void add_phone_number(struct phonebook_contact *contact,
> contact->numbers = g_slist_append(contact->numbers, number);
> }
>
> -static gchar *find_email(GSList *emails, const char *email)
> +static struct phonebook_email *find_email(GSList *emails, const char *email,
> + int type)
I assume your tabsize is < 8, because for me "int type" is after the
comma on the preceeding line. I don't know for sure, but I assume bluez
uses the same convention as the kernel, so tabsize is 8. (The functions
below need the same fix.)
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Hi Lukasz,
On Mon, Aug 23, 2010, Lukasz Pawlik wrote:
> + struct phonebook_email *email_addr;
> +
> + for (l = emails; l; l = l->next) {
> + email_addr = l->data;
Please always define variables in the smallest possible scope. In this
case email_addr can be defined in the beginning of the for-loop instead
of the beginning of the function.
> + if (g_strcmp0(email_addr->email, email) == 0
> + && email_addr->type == type)
> + return email_addr;
Continuation lines should be indented by at least two tabs more than the
original so that they stand out clearly from the actual code that's part
of the subsection. Also, the && goes on the preceeding line.
> -static void add_email(struct phonebook_contact *contact, const char *email)
> +static void add_email(struct phonebook_contact *contact, const char *email,
> + int type)
> {
> + struct phonebook_email *email_addr;
> if (email == NULL || strlen(email) == 0)
> return;
>
> /* Not adding email if there is already added with the same value */
> - if (find_email(contact->emails, email))
> + if (find_email(contact->emails, email, type))
> return;
>
> - contact->emails = g_slist_append(contact->emails, g_strdup(email));
> + email_addr = g_new0(struct phonebook_email, 1);
> + email_addr->email = g_strdup(email);
> + email_addr->type = type;
> +
> + contact->emails = g_slist_append(contact->emails, email_addr);
I'd do some renaming of variables here to make it more readable:
s/email/address/
s/email_address/email/
s/email->email/email->address/
> +struct phonebook_email {
> + char *email;
As mentioned above I'd do a s/email/address/ here.
Johan