Hi guys!
The commit has been done!
Any test of the new code will be very welcome. I attach the sample files
I have used to test as I advance.
Richard, is it s good idea to put this files in a directory called
"tasks-undo" under directory tests?
Cheers
El dom, 04-04-2004 a las 16:14, Alvaro del Castillo escribió:
> 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
>
> _______________________________________________
> Planner-dev mailing list
> Planner-dev lists imendio com
> http://lists.imendio.com/mailman/listinfo/planner-dev
>
Attachment:
tasks-undo.tgz
Description: application/compressed-tar
Attachment:
signature.asc
Description: Esta parte del mensaje =?ISO-8859-1?Q?est=E1?= firmada digitalmente