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

[Richard Hult <richard imendio com>]

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

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

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

Also, a missing space after the strlen.

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

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

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

> +       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 { }

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

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