Re: [Planner Dev] Planner Phase undo/redo v0.2 for review.

[Richard Hult <richard imendio com>]

On tor, 2004-04-29 at 11:28 +0100, lincoln phipps openmutual net wrote:

> >>        if (found) {
> >>                g_object_set (data->project, "phases", list, NULL);
> >>+               cmd = (PhaseCmdRemove*) phase_cmd_remove
> >>(data->main_window, g_strdup (phase) );
> > 
> > 
> > A stray space before the last ")". Should there be both the g_object_set
> > and cmd_remove...?
> 
> OK and probably yes to both adjusting the phases and the remove cmd.
> I'll check.

Hm, but will that part be handled by the undo/redo stuff if it's done
directly like that?

> > I'd just do the phase_cmd_insert here, instead of using a found
> > variable.
> 
> It was to fix the problem of duplicate names for phases being added.
> It just walks through the list and remembers if the current list has
> the same phase and then doesn't do anything if it did find it.
> 
> I'll double check my logic but it seems to work well and fixes
> a separate problem in Planner today (blank or duplicate phase
> names).

I meant that the found variable is unnecessary since the
phase_cmd_insert command could be moved up to the check.

> > Missing { }, we always use them even for one-line blocks.
> 
> Ok - will do, though maybe because I copied from the existing
> stuff  ...I've seens a few scattered in the code ;)

Yeah, I'm sure a few have slipped by :)

> >>+       /* Insert undo taken from phase_dialog_remove_phase
> >>+        */
> >>+       g_object_get (cmd->project, "phases", &list, NULL);
> >>+
> >>+       for (l = list; l; l = l->next) {
> >>+               if (!strcmp (cmd->phase, l->data)) {
> >>+                       g_free (l->data);
> >>+                       list = g_list_remove_link (list, l);
> >>+                       found = TRUE;
> >>+                       break;
> >>+               }
> >>+       }
> >>+
> >>+       if (found) {
> >>+               g_object_set (cmd->project, "phases", list, NULL);
> >>+       }
> > 
> > 
> > I'd put the g_object_set directly in the look instead of using the found
> > variable.
> 
> OK but don't think thats right - the for() walks the list and if
> it finds any matching phase name then it unlinks it. Only if it
> finds a match does it bother to set the project phase.

Shouldn't the result be the exact same?

> > Missing { }
> 
> OK - will track where original code came from an clean that too.

Great!

Oh, one more thing I spotted. I'd prefer if

+phase_cmd_insert (PlannerWindow *main_window, 
+                 gchar *phase) 

is changed to

+phase_cmd_insert (PlannerWindow *main_window, 
+                 const gchar *phase) 

And the g_strdup is put inside this function instead of done in the
caller.

Thanks,
Richard

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