Re: Re: [Planner Dev] Add "Assigned to" column to the tasks display

[<chmorgan charter net>]

> 
> From: Richard Hult <richard imendio com>
> Date: 2004/07/21 Wed AM 04:02:44 EDT
> To: Planner Dev <planner-dev lists imendio com>
> Subject: Re: [Planner Dev] Add "Assigned to" column to the tasks display
> 
> On mån, 2004-07-19 at 21:04 -0400, Chris Morgan wrote:
> > Just wanted to thank the developers of planner for an excellent and very 
> > useful program.
> > 
> > I wrote up this patch to add a column to the task view with the resources a 
> > task is assigned to.  This seemed quite useful to have and allows for knowing 
> > which tasks have no one assigned to at the moment.  This is my first planner 
> > patch so please tell me what things I need to clean up to get this into cvs.
> 
> Hi,
> 
> The patch looks like a useful addition! I have a few comments on the
> code, mostly style issues. We try to use a very consistent style to make
> the code easy to read and maintain:
> 
> The indentation seems to be off in a few places, e.g.:
> 
> +       gchar  *assigned_to;
> +       GList  *resources;
> +       MrpAssignment  *assignment;
> +       MrpResource    *resource;
> 
> Those should be aligned.
> 

Of course, I'll take care of that and the rest of the formatting issues.


> +       resources = mrp_task_get_assigned_resources(task);
> +
> +        for (l = resources; l; l = l->next) {
> 
> Here as well.
> 
> +               if(assigned_to)
> +               {
> 
> The brace should be on the same line as the if, same with else 
> (} else {).
> 
> newstr = g_strdup_printf("%s, %s", assigned_to, name);
> 
> We use spaces before "(".
> 
> Other than that, the patch looks fine. I have one objection should, that
> is that we should really cache the string instead of building if in the
> data_func. data_func is run every time the cell is redrawn, like when
> you scroll or move the mouse over it.
> 

Are there other data functions in that code that cache their values rather than retrieve them each time?  Is that the purpose for the task tree view?  

> Thanks,
> Richard
> 
> -- 
> Imendio HB, http://www.imendio.com/
> 
> _______________________________________________
> Planner-dev mailing list
> Planner-dev lists imendio com
> http://lists.imendio.com/mailman/listinfo/planner-dev
> 
> 

Thanks,
Chris