Re: [Planner Dev] Fourth patch for task undo removal: assingments also - Problem with gantt widget?

[Alvaro del Castillo <acs lambdaux com>]

Hi!

El sáb, 03-04-2004 a las 21:53, Richard Hult escribió:
> Hi,
> 
> Thanks a lot for this patch, it's a lot of work :)
> 
> The patch looks great, I have just a few comments, mostly style issues.
> 

Great!

> > @@ -177,11 +179,25 @@ typedef struct {
> > } TaskCmdInsert;
> > 
> > static void
> > +task_cmd_insert_free (PlannerCmd *cmd_base)
> > +{
> > + TaskCmdInsert *cmd;
> > +
> > + cmd = (TaskCmdInsert*) cmd_base;
> > +
> > + gtk_tree_path_free (cmd->path);
> > + g_object_unref (cmd->task);
> > + cmd->task = NULL;
> > + g_free (cmd);
> > + cmd = NULL;
> > +}
> 
> No need to set cmd to NULL there.
> 

Ok, I am a bit paranoid about memory issues ;-)


> > +static void
> > task_cmd_insert_do (PlannerCmd *cmd_base)
> > {
> > TaskCmdInsert *cmd;
> > GtkTreePath   *path;
> > - MrpTask       *task, *parent;
> > + MrpTask       *parent;
> > gint           depth;
> > gint           position;
> > 
> > @@ -201,18 +217,17 @@ task_cmd_insert_do (PlannerCmd *cmd_base
> > 
> > gtk_tree_path_free (path);
> > 
> > - task = g_object_new (MRP_TYPE_TASK,
> > + if (cmd->task == NULL)
> > + cmd->task = g_object_new (MRP_TYPE_TASK,
> >      "work", cmd->work,
> >      "duration", cmd->duration,
> > -      "name", cmd->name ? cmd->name : "",
> >      NULL);
> 
> Looks like there are some { } missing here.

Done!

> 
> > @@ -245,10 +258,11 @@ task_cmd_insert (PlannerTaskTree *tree,
> > cmd_base->label = g_strdup (_("Insert task"));
> > cmd_base->do_func = task_cmd_insert_do;
> > cmd_base->undo_func = task_cmd_insert_undo;
> > - cmd_base->free_func = NULL; /* FIXME */
> > + cmd_base->free_func = task_cmd_insert_free;
> > 
> > cmd->tree = tree;
> > cmd->project = task_tree_get_project (tree);
> > + cmd->task = NULL;
> 
> No need to set task to NULL, it's already NULL.
> 

Done! I think when I coded it I was not sure that the compiler init to
NULL pointers.

> > @@ -350,6 +364,357 @@ task_cmd_edit_property (PlannerTaskTree 
> > return cmd_base;
> > }
> > 
> > +typedef struct {
> > + PlannerCmd       base;
> > +
> > + PlannerTaskTree *tree;
> > + MrpProject      *project;
> > +
> > + GtkTreePath     *path;
> > + MrpTask         *task;
> > + GList           *childs;
> > + GList           *successors;
> > + GList           *predecessors;
> > + GList           *assignments;
> > +} TaskCmdRemove;
> > +
> > +static gboolean is_task_in_project (MrpTask *task, PlannerTaskTree
> > *tree)
> 
> Should be:
> 
> static gboolean
> is_...

Sure!!!

> 
> > 
> > +{
> > + PlannerGanttModel *model; 
> > + GtkTreePath       *path;
> > +
> > + model = PLANNER_GANTT_MODEL (gtk_tree_view_get_model (GTK_TREE_VIEW
> > (tree)));
> > + path = planner_gantt_model_get_path_from_task (model, task);
> > +
> > + if (path != NULL) {
> > + gtk_tree_path_free (path);
> > + return TRUE;
> > + } else { 
> > + return FALSE;
> > + }
> > +}
> >
> > +static void
> > +task_cmd_save_assignments (TaskCmdRemove *cmd)
> > +{
> > + GList *l;
> > +
> > + cmd->assignments = g_list_copy (mrp_task_get_assignments
> > (cmd->task));
> > + for (l = cmd->assignments; l; l = l->next) {
> > + if (g_getenv ("PLANNER_DEBUG_UNDO_TASK"))
> > + g_message ("Assignment save %s is done by %s (%d)", 
> > +    mrp_task_get_name (cmd->task), 
> > +    mrp_resource_get_name (mrp_assignment_get_resource (l->data)),
> > +    mrp_assignment_get_units (l->data)); 
> > + g_object_ref (l->data);
> > + }
> > +}
> > +
> > +static void
> > +task_cmd_restore_assignments (TaskCmdRemove *cmd)
> > +{
> > + GList         *l;
> > + MrpAssignment *assignment;
> > + MrpResource   *resource; 
> > +
> > + for (l = cmd->assignments; l; l = l->next) {
> > + assignment = l->data;
> > + resource = mrp_assignment_get_resource (assignment);
> > +
> > + // if (!is_task_in_project (rel_task, cmd->tree)) continue;
> 
> is_task_in_project should probably just be used for debugging, if it
> fails then there is a bug somewhere.

Hmm, not sure. If you delete a task, all the subtasks are deleted. But
you could select also the subtasks to be removed, so when you try to
remove them, the task isn't in the project. I was trying to find  a
better way to test if a task was in a project. Now, there are tasks that
exist but not in any projects. For example, removed tasks in a remove
undo command.


> 
> + if (g_getenv ("PLANNER_DEBUG_UNDO_TASK"))
> > + g_message ("Resource recover: %s is done by %s",
> > +    mrp_task_get_name (cmd->task),
> > +    mrp_resource_get_name (mrp_assignment_get_resource (l->data)));
> > + 
> > + mrp_resource_assign (resource, cmd->task,
> > +      mrp_assignment_get_units (assignment));
> > + }
> > +}
> > +
> > +
> > +static void
> > +task_cmd_save_relations (TaskCmdRemove *cmd)
> > +{
> > + GList *l;
> > +
> > + cmd->predecessors = g_list_copy (mrp_task_get_predecessor_relations
> > (cmd->task));
> > + for (l = cmd->predecessors; l; l = l->next) {
> > + if (g_getenv ("PLANNER_DEBUG_UNDO_TASK"))
> > + g_message ("Predecessor save %s -> %s", 
> > +    mrp_task_get_name (mrp_relation_get_predecessor (l->data)), 
> > +    mrp_task_get_name (mrp_relation_get_successor (l->data)));
> > + 
> > + g_object_ref (l->data);
> > + }
> > +
> > + cmd->successors = g_list_copy (mrp_task_get_successor_relations
> > (cmd->task));
> > + for (l = cmd->successors; l; l = l->next) {
> > + if (g_getenv ("PLANNER_DEBUG_UNDO_TASK")) 
> > + g_message ("Successor save %s -> %s", 
> > +    mrp_task_get_name (mrp_relation_get_predecessor (l->data)), 
> > +    mrp_task_get_name (mrp_relation_get_successor (l->data)));
> > + g_object_ref (l->data);
> > + }
> > +}
> > +
> > +static void
> > +task_cmd_restore_relations (TaskCmdRemove *cmd)
> > +{
> > + GList       *l;
> > + MrpRelation *relation;
> > + MrpTask     *rel_task;
> > + 
> > +
> > + for (l = cmd->predecessors; l; l = l->next) {
> > + relation = l->data;
> > + rel_task = mrp_relation_get_predecessor (relation);
> > +
> > + if (!is_task_in_project (rel_task, cmd->tree)) continue;
> 
> As above.

Ok, I have checked all the "if's" in the file to be sure no style
mistakes survive ;-)

> 
> > + if (g_getenv ("PLANNER_DEBUG_UNDO_TASK"))
> > + g_message ("Predecessor recover: %s -> %s",
> > +    mrp_task_get_name (mrp_relation_get_predecessor (l->data)),
> > +    mrp_task_get_name (mrp_relation_get_successor (l->data)));
> > + 
> > + mrp_task_add_predecessor (cmd->task, rel_task, 
> > +   mrp_relation_get_relation_type (relation),
> > +   mrp_relation_get_lag (relation), NULL);
> > + }
> > +
> > + for (l = cmd->successors; l; l = l->next) {
> > + relation = l->data;
> > + rel_task = mrp_relation_get_successor (relation);
> > +
> > + if (!is_task_in_project (rel_task, cmd->tree)) continue; 
> 
> Again.

Done

> 
> > + if (g_getenv ("PLANNER_DEBUG_UNDO_TASK"))
> > + g_message ("Successor recover: %s -> %s",
> > +    mrp_task_get_name (mrp_relation_get_predecessor (l->data)),
> > +    mrp_task_get_name (mrp_relation_get_successor (l->data)));
> > + 
> > + mrp_task_add_predecessor (rel_task, cmd->task,
> > +   mrp_relation_get_relation_type (relation),
> > +   mrp_relation_get_lag (relation), NULL);
> > + }
> > +}
> > +
> > +static void 
> > +task_cmd_save_childs (TaskCmdRemove *cmd)
> 
> Should be children, I believe :) Likewise for the other places.
> 

Ups, my poor english :) Corrected in all places.

> It would also be better to use mrp_task_get_first_child and then iterate
> with mrp_task_get_next_sibling, should be much more efficient.

Ok, I will look at it before doing the commit.

> 
> > +{
> > + gint childs, i;
> > + 
> > + childs = mrp_task_get_n_children (cmd->task);
> > +
> > + for (i = 0; i < childs; i++) {
> > + MrpTask           *task;
> > + TaskCmdRemove     *cmd_child;
> > + GtkTreePath       *path;
> > + PlannerGanttModel *model;
> > + 
> > + model = PLANNER_GANTT_MODEL (gtk_tree_view_get_model (GTK_TREE_VIEW
> > (cmd->tree)));
> > + task = mrp_task_get_nth_child (cmd->task, i);
> > + 
> > + path = planner_gantt_model_get_path_from_task (model, task);
> > + 
> > + /* We don't insert this command in the undo manager */
> > + cmd_child = g_new0 (TaskCmdRemove, 1);
> > + cmd_child->tree = cmd->tree;
> > + cmd_child->project = task_tree_get_project (cmd->tree); 
> > + cmd_child->path = gtk_tree_path_copy (path); 
> > + cmd_child->task = g_object_ref (task);
> > + task_cmd_save_relations (cmd_child);
> > + task_cmd_save_assignments (cmd_child);
> > + 
> > + cmd->childs = g_list_append (cmd->childs, cmd_child);
> > + 
> > + task_cmd_save_childs (cmd_child);
> > + }
> > +
> > + if (g_getenv ("PLANNER_DEBUG_UNDO_TASK")) {
> > + if (cmd->childs != NULL) {
> > + GList *l;
> > + for (l = cmd->childs; l; l = l->next) {
> > + TaskCmdRemove *cmd_child = l->data;
> > + g_message ("Child saved: %s", mrp_task_get_name (cmd_child->task));
> > + }
> > + }
> > + }
> > + 
> > +}
> > +
> > +static void
> > +task_cmd_restore_childs (TaskCmdRemove *cmd)
> > +{
> > + PlannerGanttModel *model;
> > + gint               position, depth;
> > + GtkTreePath       *path;
> > + MrpTask           *parent;
> > + GList             *l;
> > +
> > + for (l = cmd->childs; l; l = l->next) {
> > + TaskCmdRemove *cmd_child; 
> > + 
> > + cmd_child = l->data;
> > +
> > + path = gtk_tree_path_copy (cmd_child->path);
> > + model = PLANNER_GANTT_MODEL (gtk_tree_view_get_model 
> > +      (GTK_TREE_VIEW (cmd_child->tree)));
> > + 
> > + depth = gtk_tree_path_get_depth (path);
> > + position = gtk_tree_path_get_indices (path)[depth - 1];
> > + 
> > + if (depth > 1) {
> > + gtk_tree_path_up (path);
> > + parent = task_tree_get_task_from_path (cmd_child->tree, path);
> > + } else {
> > + parent = NULL;
> > + }
> > + 
> > + gtk_tree_path_free (path);
> > + 
> > + mrp_project_insert_task (cmd_child->project,
> > + parent,
> > + position,
> > + cmd_child->task);
> > + 
> > + task_cmd_restore_childs (cmd_child);
> > + task_cmd_restore_relations (cmd_child);
> > + task_cmd_restore_assignments (cmd_child);
> > + }
> > +}
> > +
> > +static void
> > +task_cmd_remove_do (PlannerCmd *cmd_base)
> > +{
> > + TaskCmdRemove *cmd;
> > + gint           childs;
> > +
> > + cmd = (TaskCmdRemove*) cmd_base;
> > +
> > + task_cmd_save_relations (cmd);
> > + task_cmd_save_assignments (cmd);
> > +
> > + childs = mrp_task_get_n_children (cmd->task);
> > +
> > + if (childs > 0 && cmd->childs == NULL) task_cmd_save_childs (cmd);
> 
> Just checking cmd->childs should be enough here.
> 

Yes! And also, the { and } ;-) Corrected before.


> > + g_message ("Removing the task : %p", cmd->task);
> > +
> > + mrp_project_remove_task (cmd->project, cmd->task);
> > +}
> > +
> > +static void
> > +task_cmd_remove_undo (PlannerCmd *cmd_base)
> > +{
> > + PlannerGanttModel *model;
> > + TaskCmdRemove     *cmd;
> > + gint               position, depth;
> > + GtkTreePath       *path;
> > + MrpTask           *parent;
> > + MrpTask           *child_parent;
> > + 
> > + cmd = (TaskCmdRemove*) cmd_base;
> > +
> > + path = gtk_tree_path_copy (cmd->path);
> > + model = PLANNER_GANTT_MODEL (gtk_tree_view_get_model (GTK_TREE_VIEW
> > (cmd->tree)));
> > +
> > + depth = gtk_tree_path_get_depth (path);
> > + position = gtk_tree_path_get_indices (path)[depth - 1];
> > +
> > + if (depth > 1) {
> > + gtk_tree_path_up (path);
> > + parent = task_tree_get_task_from_path (cmd->tree, path);
> > + } else {
> > + parent = NULL;
> > + }
> > + 
> > + gtk_tree_path_free (path);
> > +
> > + g_message ("Recovering the task : %p", cmd->task);
> > +
> > + mrp_project_insert_task (cmd->project,
> > + parent,
> > + position,
> > + cmd->task);
> > +
> > + child_parent = planner_gantt_model_get_indent_task_target (model,
> > cmd->task);
> > +
> > + if (cmd->childs != NULL) task_cmd_restore_childs (cmd);
> > +
> > + task_cmd_restore_relations (cmd);
> > + task_cmd_restore_assignments (cmd);
> > +}
> > +
> > +static void
> > +task_cmd_remove_free (PlannerCmd *cmd_base)
> > +{
> > + TaskCmdRemove *cmd;
> > + GList         *l;
> > +
> > + cmd = (TaskCmdRemove*) cmd_base;
> > +
> > + for (l = cmd->childs; l; l = l->next)
> > + task_cmd_remove_free (l->data);
> > +
> > + g_object_unref (cmd->task);
> > +
> > + g_list_free (cmd->childs);
> > +
> > + for (l = cmd->predecessors; l; l = l->next) 
> > + g_object_unref (l->data);
> > + g_list_free (cmd->predecessors);
> > +
> > + for (l = cmd->successors; l; l = l->next)
> > + g_object_unref (l->data);
> > + g_list_free (cmd->successors);
> > +
> > + for (l = cmd->assignments; l; l = l->next)
> > + g_object_unref (l->data);
> > + g_list_free (cmd->assignments);
> 
> I would do g_list_foreach (cmd->foo, (GFunc) g_object_unref, NULL) here,
> and the same above when the objects are reffed.

Ok, done!

>  
> > + g_free (cmd_base->label);
> > + gtk_tree_path_free (cmd->path);
> > + 
> > +
> > + g_free (cmd);
> > + cmd = NULL;
> 
> No need for the last row.
> 

Ok!

> > +}
> > +
> > +static PlannerCmd *
> > +task_cmd_remove (PlannerTaskTree *tree,
> > + GtkTreePath     *path,
> > + MrpTask         *task)
> > +{
> > + PlannerTaskTreePriv *priv = tree->priv;
> > + PlannerCmd          *cmd_base;
> > + TaskCmdRemove       *cmd;
> > +
> > + g_return_val_if_fail (MRP_IS_TASK (task), NULL);
> 
> No need to check this in the internal functions (we're probably not
> consistent with this).
> 

This was a debugging code I have used but no deleted. Thanks!

> Thanks again, this looks very nice! I'd say it's ready to be committed
> after fixing the small details that I mentioned.
> 

Ok, all the fixes done except the sibling one. I will look at it right
now and it is easy as it seems, I will commit in the next hours all the
work.

Thanks for que quick response to review lots of code.

Cheers

> Cheers,
> Richard