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

[Richard Hult <richard imendio com>]

On tis, 2004-04-20 at 04:10 +0100, lincoln phipps openmutual net wrote:

Hi,

> This is the latest version of this patch to address
> bugzilla enhancement,
> 
> http://bugzilla.gnome.org/show_bug.cgi?id=132346

Thanks!

Now that the fever is gone, I have a some further nagging ;)

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

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

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

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

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

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

> + dialog = g_object_get_data (G_OBJECT (priv->main_window),
> "input-tasks-dialog"

Think this should be "task-link-dialog" 

> + /* TODO: Remove the project from this. Keep count
> + */

What does this comment mean?

> + count = g_list_length (planner_task_tree_get_selected_tasks (tree));

The list of tasks is leaked here, need a g_list_free.

> + dialog = planner_task_link_dialog_new (priv->project,count);
> +
> +

Missing space and a stray empty line.

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

I'd prefer if the variables were named like direction_up_rb,
mode_fanout_rb, etc, to look more like our other cases.

In the same function:

> + if (dialog) {
> + gtk_widget_destroy (dialog);
> + }

Dialog is always set here, no need to test.

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

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

-- 
Imendio HB, http://www.imendio.com/