Please see questions below.
On Tue, 2012-02-07 at 11:31 -0500, Chris Lumens wrote:
> 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?
I think they are good placeholders. Should I remove them?
>
> > 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@xxxxxxxxx>
> > + *
> > + * 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@xxxxxxxxxx>
> > + *
> > + */
> > +
> > +#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.
I've made some attempts to solve this, but none was successful. If I add
AC_DEFINE to widgets/configure.ac I have this value in config.h. But
then it is not in widgets/data/tzmapdata/Makefile.am where it is also
needed. Moreover it should use $(datadir) which cannot be used in
configure.ac. So how to resolve this?
>
> > 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@xxxxxxxxxx>
> > + *
> > + * Largely based on Michael Fulbright's work on Anaconda.
>
> There's something very bizarre about this.
Should I remove this? I think it's nice example of how OSS work.
>
> > +#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
>
> _______________________________________________
> Anaconda-devel-list mailing list
> Anaconda-devel-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/anaconda-devel-list
--
Vratislav Podzimek
Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
