Re: [Planner Dev] Planner task edit multiple-dialog enhancement patch (132453)

[Richard Hult <richard imendio com>]

Hi,

Looked some more at the patch:

About the task dialog submenu, IF we are to put in that, we at least
need to come up with a better string for "General page" :) I don't have
any better suggestion right now though.

I would also prefer not to add the stock icons, since I think they look
a bit too much different from the style used for the other icons.

> Index: src/planner-task-dialog.c
> ===================================================================
> RCS file: /cvs/gnome/planner/src/planner-task-dialog.c,v
> retrieving revision 1.6
> diff -u -b -B -p -r1.6 planner-task-dialog.c
> --- src/planner-task-dialog.c	6 Apr 2004 21:26:39 -0000	1.6
> +++ src/planner-task-dialog.c	11 Apr 2004 02:47:58 -0000
> @@ -1714,7 +1714,8 @@ task_dialog_destroy_cb (GtkWidget  *pare
>  
>  GtkWidget *
>  planner_task_dialog_new (PlannerWindow *window,
> -		    MrpTask      *task)
> +		    MrpTask      *task,
> +		    EditTaskPage page)
>  {
>  	DialogData   *data;
>  	GladeXML     *glade;
> @@ -1770,6 +1771,9 @@ planner_task_dialog_new (PlannerWindow *
>  			  G_CALLBACK (task_dialog_task_removed_cb),
>  			  data);
>  
> +	w = glade_xml_get_widget (glade, "task_notebook");
> +	gtk_notebook_set_current_page( GTK_NOTEBOOK (w), (gint) page);

Should be ...page (GTK_... and no (gint) needed. If we do it like this the enum
needs to have numbers assigned, but I usually prefer to not use
enums as an int and instead use a switch (page) { ..., keeps the interface
clean and we can move around/remove/add pages without depending on magic numbers.

> +			err_dialog = gtk_message_dialog_new (NULL,
> +                                 GTK_DIALOG_DESTROY_WITH_PARENT,
> +                                 GTK_MESSAGE_ERROR,
> +                                 GTK_BUTTONS_OK,
> +                                 _("You cannot open all of these %i tasks. Please select less than %i tasks."), (int) count, (int) MAX_TASK_DIALOGS);
> + 			gint result = gtk_dialog_run (GTK_DIALOG (err_dialog));

You can't declare a variable after code like that (not valid C even if
gcc says OK). "gint result" should be moved up a bit. Also the casts to
int shouldn't be necessary I think?

Other than that the patch should be fine.

Thanks a lot,
Richard

-- 
Imendio HB, http://www.imendio.com/