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

[Richard Hult <richard imendio com>]

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.

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

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

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

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

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

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

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

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

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.

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

> + 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.
 
> + g_free (cmd_base->label);
> + gtk_tree_path_free (cmd->path);
> + 
> +
> + g_free (cmd);
> + cmd = NULL;

No need for the last row.

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

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

Cheers,
Richard

-- 
Richard Hult                    richard imendio com
Imendio                         http://www.imendio.com