[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [PATCH 1/2] Add TimezoneMap widget to AnacondaWidgets



As a general comment, please try to follow the same C coding style as
I'm using in the rest of the widgets/ directory, especially where brace
placement is concerned.

> diff --git a/widgets/src/Makefile.am b/widgets/src/Makefile.am
> index acac3b0..e33e72c 100644
> --- a/widgets/src/Makefile.am
> +++ b/widgets/src/Makefile.am
> @@ -31,6 +31,8 @@ SOURCES = BaseWindow.c \
>  	 SpokeSelector.c \
>  	 SpokeWindow.c \
>  	 StandaloneWindow.c \
> +	 TimezoneMap.c \
> +	 tz.c \
>  	 lightbox.c
>  
>  HDRS = BaseWindow.h \
> @@ -39,6 +41,8 @@ HDRS = BaseWindow.h \
>  	 SpokeSelector.h \
>  	 SpokeWindow.h \
>  	 StandaloneWindow.h \
> +	 TimezoneMap.h \
> +	 tz.h \
>  	 lightbox.h
>  
>  noinst_HEADERS = gettext.h intl.h

Do you intend to have tz.c exposed beyond just TimezoneMap.c?  If not,
we're going to need to restructure these variables a bit.  By putting it
in SOURCES, gir-scanner is going to attempt to pick it up and add it to
the namespace.  We don't want that.

> +static void
> +anaconda_timezone_map_get_property (GObject    *object,
> +                              guint       property_id,
> +                              GValue     *value,
> +                              GParamSpec *pspec)
> +{
> +  switch (property_id)
> +    {
> +    default:
> +      G_OBJECT_WARN_INVALID_PROPERTY_ID (object, property_id, pspec);
> +    }
> +}
> +
> +static void
> +anaconda_timezone_map_set_property (GObject      *object,
> +                              guint         property_id,
> +                              const GValue *value,
> +                              GParamSpec   *pspec)
> +{
> +  switch (property_id)
> +    {
> +    default:
> +      G_OBJECT_WARN_INVALID_PROPERTY_ID (object, property_id, pspec);
> +    }
> +}

Do you even need these if you don't have any properties?

> diff --git a/widgets/src/TimezoneMap.h b/widgets/src/TimezoneMap.h
> new file mode 100644
> index 0000000..aa74803
> --- /dev/null
> +++ b/widgets/src/TimezoneMap.h
> @@ -0,0 +1,87 @@
> +/*
> + * Copyright (C) 2012 Red Hat, Inc
> + *
> + * Heavily based on the gnome-control-center code,
> + * Copyright (c) 2010 Intel, Inc
> + * Written by Thomas Wood <thomas wood intel com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
> + *
> + * Author: Vratislav Podzimek <vpodzime redhat com>
> + *
> + */
> +
> +#ifndef DATADIR
> +#define DATADIR "/usr/share/anaconda/tzmapdata/"
> +#endif

I think this should come out of config.h, or some other programmatic
method.  Otherwise, you're going to have a disconnect between the code
and whatever happens during an RPM build.

> diff --git a/widgets/src/tz.c b/widgets/src/tz.c
> new file mode 100644
> index 0000000..e650c22
> --- /dev/null
> +++ b/widgets/src/tz.c
> @@ -0,0 +1,482 @@
> +/* -*- Mode: C; tab-width: 8; indent-tabs-mode: t; c-basic-offset: 8 -*- */
> +/* Generic timezone utilities.
> + *
> + * Copyright (C) 2000-2001 Ximian, Inc.
> + *
> + * Authors: Hans Petter Jansson <hpj ximian com>
> + * 
> + * Largely based on Michael Fulbright's work on Anaconda.

There's something very bizarre about this.

> +#ifdef __sun
> +		if (tmpstrarr[3] && *tmpstrarr[3] == '-' && tmpstrarr[4])
> +			loc->comment = g_strdup (tmpstrarr[4]);
> +
> +		if (tmpstrarr[3] && *tmpstrarr[3] != '-' && !islower(loc->zone)) {
> +			TzLocation *locgrp;
> +
> +			/* duplicate entry */
> +			locgrp = g_new0 (TzLocation, 1);
> +			locgrp->country = g_strdup (tmpstrarr[0]);
> +			locgrp->zone = g_strdup (tmpstrarr[3]);
> +			locgrp->latitude  = convert_pos (latstr, 2);
> +			locgrp->longitude = convert_pos (lngstr, 3);
> +			locgrp->comment = (tmpstrarr[4]) ? g_strdup (tmpstrarr[4]) : NULL;
> +
> +			g_ptr_array_add (tz_db->locations, (gpointer) locgrp);
> +		}
> +#else
> +		loc->comment = (tmpstrarr[3]) ? g_strdup(tmpstrarr[3]) : NULL;
> +#endif

Can we please not have any defines that refer to "sun"?

> +#if 0
> +	tzset ();
> +#endif

Also, anything in a #if 0 can go too.

- Chris


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]