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

["lincoln phipps openmutual net" <lincoln phipps openmutual net>]

Richard Hult wrote:

> On tis, 2004-04-27 at 19:53 +0100, lincoln phipps openmutual net wrote:
>
> > All,
> > 	my first attempt at undo/redo. It works on my simple
> > test cases.
>
>
> Great! I'm sure Alvaro will be very happy too ;)
>
> A few comments, mostly on style issues:
>
>
> > Index: src/planner-phase-dialog.c
>
>
> > @@ -174,6 +209,10 @@ planner_phase_dialog_new (PlannerWindow 
> >        
> >        data->remove_button = glade_xml_get_widget (glade,
> > "remove_button");
> >
> > +       /* turn remove button off initally until something later
> > selected.*/
> > +       
> > +       gtk_widget_set_sensitive (data->remove_button, 0); 
>
>
> I'd prefer to set this in the glade file actually.

OK - I'll change the .glade to default to not being
sensitive.

>        
>
> > @@ -374,6 +413,8 @@ phase_dialog_remove_phase (DialogData  *
> >        GtkListStore *store;
> >        gboolean      found = FALSE;
> >
> > +       PhaseCmdRemove *cmd;
>
>
> I'm not sure if you're using -B or -b for diff (the whitespace one), or
> if the indentation is wrong here. It's probably better to send patches
> with the whitespace changes included, I guess I need to change my
> preferred diff options ;)

for CVS I use...

cvs diff -ubBp 1>../mydiffy.diff

so yes, if I'm adding in a new if{} and existing lines get tabbed
out  a bit that'll get missed.

> > @@ -393,6 +434,7 @@ phase_dialog_remove_phase (DialogData  *
> >
> >        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.

> > @@ -435,19 +480,216 @@ phase_dialog_new_dialog_run (DialogData 
> >                          G_CALLBACK
> > (phase_dialog_new_name_changed_cb),
> >                          data);
> >
> > +       g_object_get (data->project, "phases", &list, NULL);
> > +       
> >        if (gtk_dialog_run (GTK_DIALOG (dialog)) == GTK_RESPONSE_OK) {
> >                name = gtk_entry_get_text (GTK_ENTRY (entry));
> >
> > -               /* Get all phases from the project, add the new one,
> > re-set the
> > -                * list.
> > +               if (strlen(name) > 0) { /* Don't add if its blank i.e.
> > User just hit OK on little dialog. */
> > +                       found = FALSE;
> > +                       for (l = list; l ; l = l->next) {
> > +                               if (strcmp (name, (char *) l->data) ==
> > 0 ) { 
> > +                                       found = TRUE; /* Will be a
> > duplicated name which is messy */
>
>
> 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).

> Also, a missing space after the strlen.

OK - yup.

> > +static void
> > +phase_cmd_insert_free (PlannerCmd *cmd_base)
> > +{
> > +       PhaseCmdInsert  *cmd;
> > +
> > +       cmd = (PhaseCmdInsert*) cmd_base;       
> > +
> > +       cmd->phase = NULL;  /* TODO: Check if this right regarding
> > leaks ?*/
> > +       cmd->project = NULL;
>
>
> Should free the phase here.

Ah - thats what I like to see. I was uncertain of this thus
my comment.

> > +
> > +static PlannerCmd *
> > +phase_cmd_insert (PlannerWindow *main_window, 
> > +                 gchar *phase) 
> > +{
> > +       PlannerCmd      *cmd_base;
> > +       PhaseCmdInsert  *cmd;
> > +
> > +       cmd = g_new0 (PhaseCmdInsert, 1);
> > +
> > +       cmd_base = (PlannerCmd*) cmd;
> > +
> > +       cmd_base->label = g_strdup (_("Insert phase"));
> > +       cmd_base->do_func = phase_cmd_insert_do;
> > +       cmd_base->undo_func = phase_cmd_insert_undo;
> > +       cmd_base->free_func = phase_cmd_insert_free;
> > +
> > +       cmd->project = planner_window_get_project (main_window);
> > +
> > +       cmd->phase = g_strdup (phase);
> > +
> > +       planner_cmd_manager_insert_and_do
> > (planner_window_get_cmd_manager (main_window),
> > +                                          cmd_base);
> > +
> > +       return cmd_base;
> > +}
> > +
> > +static void
> > +phase_cmd_remove_do (PlannerCmd *cmd_base)
> > +{
> > +       PhaseCmdRemove *cmd;
> > +       GList          *list, *l;
> > +       gchar          *assigned_phase;
> > +       gboolean      found = FALSE;
> > +
> > +       cmd = (PhaseCmdRemove*) cmd_base;
> > +
> > +       mrp_object_get (cmd->project, "phase", &assigned_phase, NULL);
> > +       if (assigned_phase == cmd->phase)
> > +               cmd->is_assigned = TRUE;
>
>
> 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 ;)

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

> > +       mrp_string_list_free (list);
> > +}
> > +
> > +static void
> > +phase_cmd_remove_undo (PlannerCmd *cmd_base)
> > +{
> > +       PhaseCmdRemove *cmd;
> > +       GList          *list;
> > +       
> > +       cmd = (PhaseCmdRemove*) cmd_base;
> > +
> > +       /* We need to recover the phase deleted */
> > +       
> > +       g_assert (cmd->phase);
> > +       
> > +       /* Insert do code added from phase_dialog_new_dialog_run()
> > with changes.
> > +        */
>
>
> We have CVS for history ;) No need for such comments.

It where I cut+paste the code from :) aide memoire for me.
I chop it now the stuff works.
> > +       g_object_get (cmd->project, "phases", &list, NULL);
> > +       list = g_list_append (list, g_strdup (cmd->phase)); 
> > +       g_object_set (cmd->project, "phases", list, NULL);
> > +       mrp_string_list_free (list);    
> > +       
> > +
> > +       if (cmd->is_assigned) 
> > +               mrp_object_set (cmd->project, "phase", cmd->phase,
>
>
> Missing { }

OK - will track where original code came from an clean that too.

> Otherwise looks nice, minus a few indentation/whitespace issues, AFAICS,
> thanks!
> Richard


Cool - amazed only one missing free !.
Lincoln.

>