Hi Niall, New patch was posted at http://cr.opensolaris.org/~jedy/bug-758/. Please see my comments in line. On Thu, 2008-03-13 at 19:06 -0700, Niall Power wrote:
> Hi Jedy. > > First of all, let me say thanks for an excellent job on this. > > I have a few minor comments on the code. > > map.c: > ----- > draw_point (): The MapPrivate *priv variable is unused. > map_draw_timezones (): The "area" parameter is unused in this function > do_redraw (): The "area" parameter is unused in this function. Updated. > > timezone.c: > --------- > on_all_timezones_added(): The prototype and the actual function spec are > inconsistent: > > 80 static void on_all_timezones_added(GtkWidget *widget, gpointer > user_data1, > 81 gpointer user_data); > > 133 static void > 134 on_all_timezones_added(GtkWidget *widget, gpointer user_data1, > 135 gpointer user_data2) Now the prototype is static void on_all_timezones_added(GtkWidget *widget, gpointer user_data); > > Also, in on_alltimezones_added(), user_data1 is never used, and looking at > the g_signal_emit() call that generates this signal in map.c, it appears that > the second paramater (user_data1) is not user data but instead a > continent_item. Now that user_data1 is never used, continent_item parameter should be removed. > > Nits: Line 533 and 540: Indentation is inconsistent. Updated, including other indentation problem. > > I think there is some visual adjustment needed also, but I think this can be > easily corrected later and I don't it should block the approval of this code > review and getting the code integrated into the workspace. > There should be more vertical padding between the map and the timezone combo > boxes. I think it should be 6 or 12 pixels. I added 6 pixels between map widget and the combo boxes. I think this value looks OK. Regards, Jedy -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://mail.opensolaris.org/pipermail/caiman-discuss/attachments/20080318/758ed6d8/attachment.html>
