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

[Lincoln Phipps <lincoln phipps openmutual net>]

Richard Hult wrote:
> 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.

"General" Page is simply taking the title of the current task dialog.
It was to keep this all consistent. If we change it here we need to
change the Task Dialog too.


> 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.

OK - I'll see how I remove all this stuff. Being a lot of cut+paste
I'm really cloning whats already been done.

> > 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

You'd mentioned in a previous email to use enums as I originally had it as
gint values and used a #define to label the pages to something meaningful.
The pages of a  gtknotebook number from 0 upwards as integer values
i.e. they are not accessed by hash value/name/label.

I sort of like how I've done it now as it doesn't need as much code
as a set of switch+case. You simply pass a label e.g. like TASK_PAGE_GENERAL
to the planner_task_tree_edit_task() function and it switches to that
when the dialog is displayed. This is easy for others to use if they
want to.


> 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.

If we play with the Task dialog dynamically i.e. drop in/take out pages
on-the-fly, then we'd have problems. But I doubt we need to do that
so I like how the current typedef enum works (see the top of the
planner-window.c as thats where I stuck it ).

> > +			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?

OK - will clean that up. The gint result = ... was copied from another
code sample - not Planner but a GNOME API example I found somewhere
when searching for examples of modal dialogs (GPL'd).
It compiles fine and works, so yes its probaly where gcc allows you
to shortcut a bit.

The casts are me forgetting gint is integer and thus %i is OK
with gint.

> Other than that the patch should be fine.

> Thanks a lot,
> Richard


This is good - learnt a lot doing this one. I'm confident that
people will find the feature useful especially on bigger
projects where its so tedious visiting each task one at
a time.

Will rebuild and re-issue.

Rgds,
Lincoln.