Re: [Planner Dev] Fixes for gantt bugz # 148637, 149359 and 149651
- From: Richard Hult <richard imendio com>
- To: Planner Dev <planner-dev lists imendio com>
- Subject: Re: [Planner Dev] Fixes for gantt bugz # 148637, 149359 and 149651
- Date: Mon, 09 Aug 2004 12:38:35 +0200
On mån, 2004-08-09 at 09:08 +0100, lincoln phipps openmutual net wrote:
> The attached patch fixes these bugs,
>
> http://bugzilla.gnome.org/show_bug.cgi?id=148637
> http://bugzilla.gnome.org/show_bug.cgi?id=149359
> http://bugzilla.gnome.org/show_bug.cgi?id=149651
Nice!
Isn't it better to remove that limit in the dialog and have a limit for
dates at 2038? I don't really see the point in having two limits. See
comments below:
> Index: libplanner/mrp-task-manager.c
> ===================================================================
> RCS file: /cvs/gnome/planner/libplanner/mrp-task-manager.c,v
> retrieving revision 1.10
> diff -u -B -b -p -r1.10 mrp-task-manager.c
> --- libplanner/mrp-task-manager.c 2 Aug 2004 22:47:19
> -0000 1.10
> +++ libplanner/mrp-task-manager.c 9 Aug 2004 07:46:56 -0000
> @@ -2165,11 +2165,12 @@ mrp_task_manager_calculate_task_work (Mr
> mrptime finish)
> {
> MrpTaskManagerPriv *priv;
> - gint work = 0;
> + gint work = 0, this_work = 0;
> MrpAssignment *assignment;
> MrpResource *resource;
> GList *assignments, *a;
> - MrpCalendar *calendar;
> + MrpCalendar *calendar = NULL, *last_calendar = NULL;
> + gint units = 0, last_units = 0;
>
> priv = manager->priv;
>
> @@ -2194,9 +2195,23 @@ mrp_task_manager_calculate_task_work (Mr
> */
>
> assignments = mrp_task_get_assignments (task);
> +
> + if (!assignments) {
> + calendar = mrp_project_get_calendar (priv->project);
> +
> + work = task_manager_get_work_for_calendar (manager,
> + calendar,
> + start,
> + finish);
> + } else {
> for (a = assignments; a; a = a->next) {
> assignment = a->data;
>
> + last_units = units;
> + last_calendar = calendar;
> +
> + units = mrp_assignment_get_units
> (assignment);
> +
> resource = mrp_assignment_get_resource (assignment);
>
> calendar = mrp_resource_get_calendar (resource);
> @@ -2204,20 +2219,24 @@ mrp_task_manager_calculate_task_work (Mr
> calendar = mrp_project_get_calendar (priv-
> >project);
> }
>
> - work += task_manager_get_work_for_calendar (manager,
> - calendar,
> - start,
> - finish) *
> - mrp_assignment_get_units (assignment) / 100;
> - }
> -
> - if (!assignments) {
> - calendar = mrp_project_get_calendar (priv->project);
> + /* A simple check to see if the last calendar and units is the
> same as this */
> + /* calendar and units. If so then resuse last calculated work
> value. This MAY */
> + /* help speed up gantt if we have multiple resources on this
> task. */
> + /* TODO: Ideally we should cache the work calculations based
> on a key of .... */
> + /* manager, calendar, start, finish, units */
> +
> + if (last_calendar == calendar && last_units ==
> units) {
> + work +=
> this_work;
> + } else {
>
> - work = task_manager_get_work_for_calendar (manager,
> + /* Maths done this way to make sure we don't have an integer
> OVERFLOW on large workloads */
>
> + this_work =
> (task_manager_get_work_for_calendar (manager,
> calendar,
> start,
> - finish);
> +
> finish) / 100.0) * units;
> + work += this_work;
> + }
> + }
> }
This looks like a worthwhile optimization, but we could probably save
more if we just keep the work for the calendar and then divide by units
for each iteration. The division should take no time at all compared to
the _get_work_for_calendar call.
> return work;
> Index: libplanner/mrp-time.h
> ===================================================================
> RCS file: /cvs/gnome/planner/libplanner/mrp-time.h,v
> retrieving revision 1.1.1.1
> diff -u -B -b -p -r1.1.1.1 mrp-time.h
> --- libplanner/mrp-time.h 1 Dec 2003 17:36:21
> -0000 1.1.1.1
> +++ libplanner/mrp-time.h 9 Aug 2004 07:46:56 -0000
> @@ -33,6 +33,7 @@ typedef long mrptime;
> #define MRP_TIME_INVALID 0
> #define MRP_TIME_MIN 0
> #define MRP_TIME_MAX 2147483647
> +#define MRP_WORK_MAX 288000000
It'd be nice to only have a limit on the resulting times, like I said
above.
> mrptime mrp_time_current_time (void);
>
> Index: src/planner-gantt-header.c
> ===================================================================
> RCS file: /cvs/gnome/planner/src/planner-gantt-header.c,v
> retrieving revision 1.2
> diff -u -B -b -p -r1.2 planner-gantt-header.c
> --- src/planner-gantt-header.c 11 Dec 2003 10:52:46 -0000 1.2
> +++ src/planner-gantt-header.c 9 Aug 2004 07:47:00 -0000
> @@ -498,7 +498,7 @@ gantt_header_expose_event (GtkWidget
> PlannerGanttHeader *header;
> PlannerGanttHeaderPriv *priv;
> gint width, height;
> - gdouble hscale;
> + gdouble hscale, tresult;
> gint x;
> mrptime t0;
> mrptime t1;
> @@ -508,13 +508,27 @@ gantt_header_expose_event (GtkWidget
> gint major_width;
> GdkGC *gc;
> GdkRectangle rect;
> + gboolean overflow;
>
> header = PLANNER_GANTT_HEADER (widget);
> priv = header->priv;
> hscale = priv->hscale;
>
> - t0 = floor ((priv->x1 + event->area.x) / hscale + 0.5);
> - t1 = floor ((priv->x1 + event->area.x + event->area.width) /
> hscale + 0.5);
> + /* FIXME: This wraps at around year 2038 due to use of signed
> 32 bit fields */
> +
> + tresult = floor ((priv->x1 + event->area.x) / hscale + 0.5);
> + if (tresult >= MRP_TIME_MAX) {
> + t0 = MRP_TIME_MAX - event->area.width;
> + } else {
> + t0 = tresult;
> + }
> +
> + tresult = floor ((priv->x1 + event->area.x + event-
> >area.width) / hscale + 0.5);
> + if (tresult >= MRP_TIME_MAX) {
> + t1 = MRP_TIME_MAX;
> + } else {
> + t1 = tresult;
> + }
If I understand correctly, this is to prevent overflow, but there's also
the break on overflow in the loop. Wouldn't it be better to just check
for overflow here and clamp the values to be inside the valid range and
not break in the loop?
> Index: src/planner-gantt-row.c
> ===================================================================
>
> +typedef struct {
> + PlannerCmd base;
> + MrpTask *task;
> + gint work;
> + gint old_work;
> +} GanttRowCmdEditWork;
These would probably fit better in planner-task-cmd.c, along with the
functions.
> + /* FIXME: Certain weird conditions can cause
> x value to drop by 1/2. This */
> + /* happens on large work values e.g.
> 9000 days and when the end is */
> + /* dragged to the border of your
> screen (i.e far right side) and */
> + /* happens when the pointer exceeds
> the window size. */
Small detail, I prefer multi-line comments like to look like this:
/* blablabla
* blablabla
* blabla
*/
> @@ -1782,6 +1801,7 @@ gantt_row_event (GnomeCanvasItem *item,
> MrpProject *project;
> MrpCalendar *calendar;
> gint hours_per_day;
> + mrptime start, end, safe_max;
>
> project = mrp_object_get_project (MRP_OBJECT
> (priv->task));
> calendar = mrp_project_get_calendar (project);
> @@ -1810,11 +1830,30 @@ gantt_row_event (GnomeCanvasItem *item,
> /* Snap to quarters. */
> duration = floor (duration / SNAP + 0.5) *
> SNAP;
>
> + start = mrp_task_get_start (priv->task);
> +
> + /* FIXME: We can't go beyond 2038 on some
> systems. Need unsigned 32 or 64 bit time handling. */
> + /* We do maths this way to avoid
> wrapping signed longs. */
> + /* The safe_floor keeps the task end
> away from the end of the unix dates else it */
> + /* hits up against the gantt header
> end time. */
> +
> + safe_max = floor (MRP_TIME_MAX / SNAP) * SNAP
> - (18 * 24 * 60 * 60 + 3 * 60 * 60);
> +
> + if ((safe_max - duration) <= start) {
> + end = safe_max;
> + } else {
> + end = start + duration;
> + }
You can probably use the CLAMP macro here (a = CLAMP (val, low, high))
I'm not really following the magic safe_max value, how did you get that?
Ideally, we should just make sure that work doesn't overflow (which is
unlikely, not even sure that we need to do that), and then check for
overflow in mrp-task instead and clamp the resulting work/duration to
MRP_TIME_MAX?
> + if (work > MRP_WORK_MAX) {
> + work = MRP_WORK_MAX;
> + }
Same comment here as above about two limits...
Otherwise looks very nice, thanks!
/Richard
--
Imendio HB, http://www.imendio.com/
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
