GNOME.org
 

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



Richard Hult wrote:


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.


I know but....



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.


Yup - I remember that. I'm working on a deadline flag which isn't
finished yet but hadn't gutted all the partially completed code out
before shipping to you.



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


OK - it was just to highlight the arbitary maximum I defined for
the priority setting.





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.


Well at least this one got in and its really nice to use !



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.


Once you start to use it then it does become useful. The dialogs
are also hotkeyed too. Planner is a end-user tool but until
people become fully conversant with it the menus should help guide
users as to whats available. Having the dialog submenu entry also
helps driving people over-the-phone (if it ever comes to that
when we have world domination ;)

To get to a task edit you can either,
Right-mouse,
Hotkey/accelerator,
Action -> Edit...->Focus...

So all possible users are handled from experienced ones who'll use
right-mouse or hotkey to less experienced who will wander through
the Action menu trying to find what they want; I've seen quite
a wide variety of users as I have had some contract IT work at a
local school and so watch how kids from 4 to 12 yrs handle dialogs
and Menus (Windows obviously but same as GNOME). I use my own kids
(4 and 8yrs) as typical users too to see how they navigate menus
(e.g. GDM needs fixing as my 4yr old can't logon thats why his PC
uses KDM but runs GNOME as his Windows manager). They cue on the
menus and the icons quite fast as you can see icons faster than
you can read (assuming you can read at all).



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


OK - I sort of inherited the existing names so I'll review and
rename to be consistent across the UI and code.





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


I think its because task_dialog_resource_units_cell_edited() should
be using assignment_set_property() and not just g_object_set() and then
I'd add the mrp_object_changed (MRP_OBJECT (resource)) to assignment_set_property().

Is that a better way ?.





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.



OK - just I lost my constraints on a  project because I clicked the button
and was on a cut+paste fenzy with creating dialogs !




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.



OK.


A few mixed comments:

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


Yes - but I was worried it has some historical significance.
I usually add an in-line comment when dead code is removed and
then later remove the comments. This then shows up in
the changesets.



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


I sort of agree but I'd like to keep this hack as it is because we do
not yet have an upgrade routine so this hides any potential mess in the
database from the user. The code can be gutted out later but it does a job
right now.




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


I'll have to think about what I'm doing here to make sure I'm handling
nulls in the priority field OK.



In mrp-task.c:

-	gshort            percent_complete;
+	gint              percent_complete;

This shouldn't be changed.


OK - it was when I was greping for where "priority" was used I actually
searched on "complete" as that was working code. I'd noticed this
difference. Why is it left as gshort where the rest of the code uses
gint ?



In planner-task-dialog.c:

+	gtk_notebook_set_current_page((GtkNotebook *) w, page);
The cast should be GTK_NOTEBOOK (w). 

Yup - OK I'll change it as I see others use this - I was looking
at the GNOME API docs as examples thus why I used GtkNotebook .



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

We should use an enum for the page.


Notebook pages use a gint for the page which numbers from 0 upwards.
I usually think of enums as having hidden values. My defines are
in planner-window.h I'll see what I need to so to make this
an enum e.g....

Typedef enum {
	TASK_PAGE_GENERAL = 0,
	TASK_PAGE_RESOURCES   = 1,
	TASK_PAGE_PREDECESSORS  =   2,
	TASK_PAGE_NOTES          =  3,
} EditDialogPage;





planner-task-tree.c:

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

Should use g_list_length ().


OK - will do that.



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


OK - will check that.



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


Ha ha ha - wait until I get really stuck in like the database structure
stuff...

- table prefixes,
- project locking,
- project user access controls (allowing for web portals to hook
	into planner projects easier with finer access controls
	which is also what I want to do like a Postnuke block or
	something to frontend the Planner db),
-database upgrades,
-project upgrades (which is whats needed to help fix the things like
	NULLS in fields),
- database/project manage dialogs,

as well as deadline date, multi-currency support (3 years in forex
development sort of ingrained to have multi-currency) and
resource per-use costs..... I'll try and split code changes :)



Thanks,
Richard
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]