Re: setting up units in an assignement

Richard Hult <[email protected]> 18 Dec 2002 00:01:43 +0100
Newsgroups gmane.comp.gnome.apps.mr-project.devel
Message-ID <1040166103.30064.84.camel@sliff>
Hi,

Thanks! The patch is looking good, except for a few minor style issues:


Index: file-modules/xml/mrp-old-xml.c 
=================================================================== 
RCS file: /cvs/gnome/libmrproject/file-modules/xml/mrp-old-xml.c,v 
retrieving revision 1.5 
diff -u -r1.5 mrp-old-xml.c 
--- file-modules/xml/mrp-old-xml.c      16 Dec 2002 23:20:35 -0000     
1.5 
+++ file-modules/xml/mrp-old-xml.c      17 Dec 2002 21:33:47 -0000 
@@ -420,6 +420,7 @@ 
  
        task_id = old_xml_get_int (tree, "task-id"); 
        resource_id = old_xml_get_int (tree, "resource-id"); 
+       assigned_units = old_xml_get_int_with_default (tree,
"units",100); 


Missing space before 100 here.

  
Index: storage-modules/mrproject-1/mrp-parser.c 
=================================================================== 
RCS file:
/cvs/gnome/libmrproject/storage-modules/mrproject-1/mrp-parser.c,v 
retrieving revision 1.67 
diff -u -r1.67 mrp-parser.c 
--- storage-modules/mrproject-1/mrp-parser.c    16 Dec 2002 23:20:37
-0000      1.67 
+++ storage-modules/mrproject-1/mrp-parser.c    17 Dec 2002 21:33:48
-0000 
@@ -444,7 +444,7 @@ 
 static void 
 mpp_read_assignment (MrpParser *parser, xmlNodePtr tree) 
 { 
-       gint   task_id, resource_id; 
+       gint   task_id, resource_id, assigned_units; 
        MrpAssignment *assignment; 
        MrpTask       *task; 
        MrpResource   *resource; 
@@ -456,7 +456,8 @@ 
  
        task_id = mpp_xml_get_int (tree, "task-id"); 
        resource_id = mpp_xml_get_int (tree, "resource-id"); 
-        
+       assigned_units = mpp_xml_get_int_with_default (tree,
"units",100); 
+ 


Likewise.


Index: src/dialogs/task-dialog/mg-assignment-model.c 
=================================================================== 
RCS file:
/cvs/gnome/mrproject/src/dialogs/task-dialog/mg-assignment-model.c,v 
retrieving revision 1.8 
diff -u -r1.8 mg-assignment-model.c 
--- src/dialogs/task-dialog/mg-assignment-model.c       8 Jul 2002
21:22:11 -0000       1.8 
+++ src/dialogs/task-dialog/mg-assignment-model.c       17 Dec 2002
21:35:03 -0000 
@@ -216,9 +221,12 @@ 
                } 
                break; 
        case RESOURCE_ASSIGNMENT_COL_ASSIGNED_UNITS: 
-               /* FIXME: Set correct value here */ 
                g_value_init (value, G_TYPE_INT); 
-               g_value_set_int (value, 100); 
+               if ((assignment = mrp_task_get_assignment
(model->priv->task, resource))) { 
+                       g_value_set_int (value,
mrp_assignment_get_units(assignment)); 
+               } else { 
+                       g_value_set_int (value, 0); 
+               } 


Missing space before ( and we usually don't have assignments in if
statements.


Index: src/dialogs/task-dialog/mg-task-dialog.c 
=================================================================== 
RCS file:
/cvs/gnome/mrproject/src/dialogs/task-dialog/mg-task-dialog.c,v 
retrieving revision 1.45 
diff -u -r1.45 mg-task-dialog.c 
--- src/dialogs/task-dialog/mg-task-dialog.c    15 Dec 2002 00:03:29
-0000      1.45 
+++ src/dialogs/task-dialog/mg-task-dialog.c    17 Dec 2002 21:35:05
-0000 
@@ -670,6 +674,39 @@ 
 } 
  
 static void  
+task_dialog_resource_units_cell_edited (GtkCellRendererText *cell, 
+                                       gchar               *path_str, 
+                                       gchar               *new_text, 
+                                       DialogData          *data) 
+{ 
+       GtkTreeView  *tree; 
+       GtkTreeModel *model; 
+       GtkTreePath  *path; 
+       GtkTreeIter   iter; 
+       MrpResource  *resource; 
+       MrpAssignment *assignment; 
+ 
+       tree = GTK_TREE_VIEW (data->resource_list); 
+        
+       model = gtk_tree_view_get_model (tree); 
+ 
+       path = gtk_tree_path_new_from_string (path_str); 
+        
+       gtk_tree_model_get_iter (model, &iter, path); 
+ 
+       gtk_tree_path_free (path); 
+ 
+       resource = MRP_RESOURCE (((GList *)iter.user_data)->data); 


I don't think you need that MRP_RESOURCE cast.

+       assignment = mrp_task_get_assignment (data->task, resource); 
+ 
+       if (assignment) { 
+              
g_object_set(G_OBJECT(assignment),"units",atoi(new_text),NULL); 


You don't need that G_OBJECT cast, and some missing spaces.

I know we've used atoi in other places but we probably shouldn't. It
can't detect errors like an extra character at the end like a percentage
sign. I think we should use strol or g_strol or similar.


Index: src/views/gantt/mg-gantt-row.c 
=================================================================== 
RCS file: /cvs/gnome/mrproject/src/views/gantt/mg-gantt-row.c,v 
retrieving revision 1.73 
diff -u -r1.73 mg-gantt-row.c 
--- src/views/gantt/mg-gantt-row.c      17 Dec 2002 09:37:04 -0000     
1.73 
+++ src/views/gantt/mg-gantt-row.c      17 Dec 2002 21:35:06 -0000 
@@ -597,12 +598,15 @@ 
 gantt_row_update_resources (MgGanttRow *row) 
 { 
        MgGanttRowPriv *priv; 
-       GList          *resources, *l; 
+       GList          *l; 
+       GList          *assignments; 
+       MrpAssignment  *assignment; 
        MrpResource    *resource; 
        gchar          *name, *tmp_str; 
        gchar          *text = NULL; 
        PangoRectangle  rect; 
        gint            spacing, x; 
+       gchar           value[256]; 


Please use a dynamically allocated string instead.

@@ -614,10 +618,11 @@ 
        spacing = rect.width / PANGO_SCALE; 
         
        x = 0; 
-       resources = mrp_task_get_assigned_resources (priv->task); 
+       assignments = mrp_task_get_assignments (priv->task); 
                 
-       for (l = resources; l; l = l->next) { 
-               resource = MRP_RESOURCE (l->data); 
+       for (l = assignments; l; l = l->next) { 
+               assignment = MRP_ASSIGNMENT(l->data); 
+               resource = mrp_assignment_get_resource(assignment); 

Missing spaces, plus we don't need the cast here.

@@ -631,21 +636,22 @@ 
                x += spacing; 
                         
                if (text == NULL) { /* First resource */ 
-                       text = g_strdup (name); 
+                      
sprintf(value,"(%i)",mrp_assignment_get_units(assignment)); 
+                       text = g_strconcat (name, value, NULL); 

We should use g_strdup_printf and allocate the string.

-                
-               tmp_str = g_strconcat (text, ", ", name, NULL); 
+              
sprintf(value,"(%i)",mrp_assignment_get_units(assignment)); 
+               tmp_str = g_strconcat (text, ", ", name, value, NULL); 

Dito.

  
-       if (resources) { 
-               g_list_free (resources); 
+       if (assignments) { 
+               g_list_free (assignments); 

Even though we had this check before, it's not necessary, NULL is a
valid list.

I'm looking forward to getting this feature, it will be great together
with the new task manager code I recently wrote.


Regards,
Richard

-- 
Richard Hult                    [email protected]
CodeFactory AB                  http://www.codefactory.se/