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

[Richard Hult <richard imendio com>]

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.

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

Thanks,
Richard

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