Re: [Planner Dev] Advanced link tasks version 3.

["lincoln phipps openmutual net" <lincoln phipps openmutual net>]

Richard Hult wrote:

> > @@ -682,7 +693,19 @@ gantt_view_link_tasks_cb (BonoboUICompon
> > * from the most common option of FS i.e. finish to start. Maybe
> > project-wide flag, maybe menu option
> > * or maybe click-modifiers i.e. Shift+click or Control+click.
> > */
> > - planner_task_tree_link_tasks (PLANNER_TASK_TREE (view->priv->tree),
> > MRP_RELATION_FS);
> > + planner_task_tree_link_tasks (PLANNER_TASK_TREE (view->priv->tree),
> > MRP_RELATION_FS, 0, LINK_CASCADE,LINK_DOWN);
>
>
> Missing space after the comma, and the enums should be namespaced, i.e.
> PLANNER_SOMETHING_SOMETHING, to match the name of the type. We do this
> for everything that is used not just inside of file.

OK - thought about that but let it stay - I'll prefix with PLANNER_
something TBA.

> > +++ src/planner-task-tree.c 20 Apr 2004 02:23:25 -0000
> > @@ -36,6 +36,7 @@
> > #include <gtk/gtkiconfactory.h>
> > #include <gtk/gtkstock.h>
> > #include <gtk/gtkmessagedialog.h>
> > +#include <gtk/gtk.h>
>
>
> We try to include just the necessary h files since it will slow down
> compilation considerably to include the whole thing.

This is needed for something like the GTK_OPTION_MENU / GTK_BUTTON
etc. I'll remove and see where it faileds and mark that in the comments.

> > @@ -2644,10 +2648,13 @@ planner_task_tree_unlink_task (PlannerTa
> >
> > void
> > planner_task_tree_link_tasks (PlannerTaskTree *tree,
> > -       MrpRelationType  relationship)
> > +       MrpRelationType relationship,
> > + gint lag,
> > + PlannerTaskLinkMode mode,
> > + PlannerTaskDirection direction)
> > {
> > MrpTask   *task;
> > - MrpTask   *target_task;
> > + MrpTask   *target_task = NULL ;
>
>
> Is the assignment necessary here (not obvious when just looking at the
> patch), also there's a stray space there.

Without it the compiler complains of possible use of target_task
before assignment (because target_task is used inside an if () I
think) so this fixes compiler warning message.

> > + /* The list is already reversed so if direction is TRUE then we 
> > + * reverse it back. This change affects visually how the links flow.
> > + * Direction = TRUE is top-> down, whereas direction = FALSE means
> > bottom ->up.
> > + * We can never assume that projects always flow down in time for
> > tasks.
> > + * The more complex a project the greater chance of visually lower
> > tasks
> > + * being younger tasks.
> > + */
>
>
> Looks like this comment is a bit stale, it doesn't make sense when
> looking at the code.

Yup - I'll gut these comments out - they were sort of notes for me.

> > + if (mode == LINK_CASCADE) {  /* if set then it means a cascade of
> > links. */
>
>
> Kind of stating the obvious with that comment, could do without it.

OK - will gut comments.
> > + if (direction == LINK_UP) {  /* Fanout uses the 1st task as its
> > common point so we don't reverse list */
> > + list = g_list_reverse (list);
>
>
> This comment sounds a bit weird, "don't reverse" and the reverse.

it was because the list was already reversed in Planner so I unreverse
it :) but I'll reword this.

> > + dialog = g_object_get_data (G_OBJECT (priv->main_window),
> > "input-tasks-dialog"
>
>
> Think this should be "task-link-dialog" 

Yup - thats a bug - I'll correct.

> > + /* TODO: Remove the project from this. Keep count
> > + */
>
>
> What does this comment mean?

The project pointer is passed i.e.
planner_task_link_dialog_new (priv->project,count);

as it was like that from the code I'd copied from yet I don't
think its now needed (it was used to get the tree/task/working time)
to derive the lag and now thats back in planner_task_tree_link_tasks_dialog
I don't think it was now needed to be still used. I'll remove/reword this.

> > + count = g_list_length (planner_task_tree_get_selected_tasks (tree));
>
>
> The list of tasks is leaked here, need a g_list_free.

I guess I'll therefore have to do,

list = planner_task_tree_get_selected_tasks (tree);
count = g_list_length (list);
...
g_list_free list);

I was hoping to shortcut :) - ah wants a garbage collector :)

> > + dialog = planner_task_link_dialog_new (priv->project,count);
> > +
> > +
>
>
> Missing space and a stray empty line.

OK.
> > + if (gtk_toggle_button_get_active (GTK_TOGGLE_BUTTON
> > (data->radiobutton_mode_c))) {
> > + mode = LINK_CASCADE;
> > + } 
> > +
> > + if (gtk_toggle_button_get_active (GTK_TOGGLE_BUTTON
> > (data->radiobutton_mode_fo))) {
> > + mode = LINK_FANOUT;
> > + } 
> > +
> > + if (gtk_toggle_button_get_active (GTK_TOGGLE_BUTTON
> > (data->radiobutton_mode_fi))) {
> > + mode = LINK_FANIN;
> > + } 
> > +
> > + if (gtk_toggle_button_get_active (GTK_TOGGLE_BUTTON
> > (data->radiobutton_direction_d))) {
> > + direction = LINK_DOWN;
> > + } 
> > + 
> > + if (gtk_toggle_button_get_active (GTK_TOGGLE_BUTTON
> > (data->radiobutton_direction_u))) {
> > + direction = LINK_UP;
> > + }
>
>
> It would be nice to group the ones that belong together and use if +
> else if for those so it's more obvious in the code.

OK - will do that - they are grouped as ordered already so easy change.
> I'd prefer if the variables were named like direction_up_rb,
> mode_fanout_rb, etc, to look more like our other cases.

OK - will rename the glade stuff to suit.
> In the same function:
>
>
> > + if (dialog) {
> > + gtk_widget_destroy (dialog);
> > + }
>
>
> Dialog is always set here, no need to test.

OK - I always feel uncomfortable about using the
gtk_widget_destroy() thing without checking what I'm destroying
but I'll remove.

> > +++ src/planner-task-tree.h 20 Apr 2004 02:23:25 -0000
> > @@ -50,6 +50,30 @@ struct _PlannerTaskTreeClass
> > GtkTreeViewClass  parent_class;
> > };
> > +typedef struct {
> > + MrpProject *project;
> > +
> > + GtkWidget  *link_relations; /* The relationship between tasks was
> > link_relation */
> > + GtkWidget  *lag_text; /* The lag (+/- value) */
> > + GtkWidget  *label_mode; /* the Mode text (we need this to hide it)
> > */
> > + GtkWidget  *radiobutton_mode_c; /* This means cascade (n:n+1)*/
> > + GtkWidget  *radiobutton_mode_fo; /* ""   ""  fan-out (1:n)    */
> > + GtkWidget  *radiobutton_mode_fi; /* ""   ""  fan-in (n:last)  */
> > + GtkWidget  *radiobutton_direction_d; /* The direction d means
> > downwards (visually) */
> > + GtkWidget  *radiobutton_direction_u; /* ""     ""     u means
> > upwards (visually)  */
> > +} PlannerLinkDialogData;
>
>
> This should only sit in the c-file, it's just private data, right? And
> the type could then be just LinkDialogData.

Its used by the planner-task-link-dialog.c too so I put it into the
.h file where it was first used.

> Should be fine to commit after this final round, thanks a lot,
> Richard

Cool - I'll focus back on the Multi-task edit dialog and the project
phase undo/redo first as thats 0.12 (and this was 0.13). I'd kept
onto this as it part of my learning how to get up to speed
on whats needed to be a good GNOME.

Rgds,
Lincoln.