El jue, 29-04-2004 a las 09:39, Richard Hult escribió:
> 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 ;)
Yes, I have seen also the patch and it seems to follow very well the
undo/redo system. Maybe I need to take a more carefully view to the
"free" functions ;-)
Great work, Lincoln!!!
>
> 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
Attachment:
signature.asc
Description: Esta parte del mensaje =?ISO-8859-1?Q?est=E1?= firmada digitalmente