Re: [Planner Dev] Planner enhancements and patches up to 31st March 2004 from Lincoln sandbox.

[Richard Hult <richard imendio com>]

Hi,

I finally got the time to comment on this, sorry for the delay.

In the future, it would be great if you could try and keep a separate
source tree for each patch, since it's almost impossible to review a
combined one like this.

Some comments (there are also comments in bugzilla on some of the bugs):

> http://bugzilla.gnome.org/show_bug.cgi?id=137544
> (Task priority spinner not effective.)

I noticed there's a "deadline" property in the dtd, we shouldn't add
things to it before we know if/how it's going to be implemented,
otherwise we will end up with a dtd with unused/misused things.

Plus I don't think the user guide change should be done, the user guide
we ship should cover the shipped version.

> http://bugzilla.gnome.org/show_bug.cgi?id=138595
> (Task up and down should work on selections.)
> Task selections can now be moved up and down.
> before only individual tasks could be moved. Very handly when
> rearranging your project schedules.
> 
> http://bugzilla.gnome.org/show_bug.cgi?id=132453
> (Should be able to set Resources for multiple tasks at once.)
> NOTE: it doesn't do this fully. But I'll close this bugzilla.
> This is a multiple dialog feature which allows you to select
> select multiple tasks and open the edit dialog on whatever
> page you are interested on for all those tasks at once.
> Its the best we're going to get for this for now.

We need to think about the part that adds a submenu with all the task
dialog pages. I'm not sure it really adds so much that it justifies the
extra UI clutter.

The command names in the UI file and code should be "EditTaskSomething"
to make it clearer what they do.

> http://bugzilla.gnome.org/show_bug.cgi?id=138368
> (Altering resource assignments does not trigger save reminder)

The correct fix should be done in the model rather than the view, i.e.
somewhere in libplanner/.

> http://bugzilla.gnome.org/show_bug.cgi?id=138596
> (Confirmation dialog needed for Reset Constraints.)
> I added this as I got annoyed when I lost my constraints
> by accidently clicking the button. May/maynot be needed
> if we have UNDO but its still a user friendly feature.

This is not necessary, since we'll have undo for this soon. (Also the
"reset all constraints" item should probably be removed, it was mostly a
workaround for the fact that old files from the GNOME 1 version was
imported with constraints set for all tasks since the old version didn't
have a scheduler).

A dialog here would make the cases where the user is not making a
mistake a lot more cumbersome, and if he does a mistake, there's undo to
fix that.

> Plus workaround for Bugzilla...
> http://bugzilla.gnome.org/show_bug.cgi?id=137964
> (Planner crashes when saving file.)

There is a variant of this patch committed already.

A few mixed comments:

The removed enum in mrp-sql.c doesn't need a RIP comment ;)

In the same file: "if (strncmp(short_name,"NULL",4) == 0)" that area
shouldn't be added. If the string is set to NULL, it indicates that a
database upgrade wasn't done correctly (or if we allow null,
then ..._get_string should be changed to return an empty string
instead).

@@ -2063,6 +2069,7 @@ sql_read_tasks (SQLData *data)
 				     "note", note,
 				     "type", type,
 				     "percent_complete", percent_complete,
+				     "priority", (priority ? priority : 0),

No need for the check there, if it's 0, the value already is 0.

In mrp-task.c:

-	gshort            percent_complete;
+	gint              percent_complete;

This shouldn't be changed.

In planner-task-dialog.c:

+	gtk_notebook_set_current_page((GtkNotebook *) w, page);

The cast should be GTK_NOTEBOOK (w).  

GtkWidget *
planner_task_dialog_new (PlannerWindow *window,
-		    MrpTask      *task)
+		    MrpTask      *task,
+		    gint page)

We should use an enum for the page.

planner-task-tree.c:

+	for (l = list; l; l = l->next) {
+		count++;
+	}

Should use g_list_length ().

The warning dialog that warns that there is no undo should be removed,
we are going to get undo for that soon.


Thanks a lot for your great effort! Hope my comments don't kill your
motivation ;)

Thanks,
Richard 

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