[parted-devel] History with undo redo

[Matt Davis]

Otavio,
I deeply appreciate your suggestions and comments.  I agree with your
suggestions as annotated below.

On 6/28/07, Otavio Salvador <otavio at debian.org> wrote:
> Matt Davis <mattdavis9 at gmail.com> writes:
>
> > --- a/include/parted/disk.h
> > +++ b/include/parted/disk.h
> > @@ -261,10 +261,13 @@ extern PedDisk* ped_disk_new_fresh (PedDevice* dev,
> >  extern PedDisk* ped_disk_duplicate (const PedDisk* old_disk);
> >  extern void ped_disk_destroy (PedDisk* disk);
> >  extern int ped_disk_commit (PedDisk* disk);
> > -extern int ped_disk_commit_to_dev (PedDisk* disk);
> >  extern int ped_disk_commit_to_os (PedDisk* disk);
> >  extern int ped_disk_check (const PedDisk* disk);
> >  extern void ped_disk_print (const PedDisk* disk);
> > +/* Located in parted/history.h
> > + * extern void ped_disk_commit_to_history (const PedDisk* disk);
> > + */
> > +extern int ped_disk_commit_to_dev (PedDisk* disk);
> >
> >  extern int ped_disk_get_primary_partition_count (const PedDisk* disk);
> >  extern int ped_disk_get_last_partition_num (const PedDisk* disk);
>
> I prefer ped_disk_commit_to_history on disk.h like others...

I avoided placing the ped_disk_commit_to_history() in disk.h since
knowledge of the struct Command was necessary, and that would require
the use of publishing Command.h in parted/include.


> > --- a/libparted/disk.c
> > +++ b/libparted/disk.c
> > @@ -510,6 +510,17 @@ ped_disk_commit (PedDisk* disk)
> >  }
> >
> >  /**
> > + * Adds the changes to disk to the command history.
> > + * This allows the commands to have an undo/redo effect
> > + * without actually performing any (possibly unwanted)
> > + * disk actions.
> > + *
> > + * This is actually defined in parted/history.c
> > +void
> > +ped_disk_commit_to_history(const PedDisk *disk)
> > +*/
>
> Same to this.
>
> > --- a/parted/Makefile.am
> > +++ b/parted/Makefile.am
> > @@ -9,6 +9,8 @@ parted_SOURCES = command.c    \
> >                strlist.h      \
> >                ui.c           \
> >                ui.h           \
> > +         history.c  \
> > +         history.h  \
> >                table.c        \
> >                table.h
>
> Wrongly indented

Yetch.. sorry about that! (I dislike mal-formatting.... ouch sorry)


>
> > --- /dev/null
> > +++ b/parted/history.c
> > @@ -0,0 +1,193 @@
> ...
> > +void ped_disk_commit_to_history(const PedDisk *disk)
> > +{
> > +    PedHistMgr.end->disk = ped_disk_duplicate(disk);
> > +}
>
> Please, move it to disk.{c,h}
>
> > +static void history_pr(PedHistObj *obj)
> > +{
> > +    /* Only print disk changes */
> > +    if (!obj->disk)
> > +      return;
> > +
> > +    printf("[History %d]\t", obj->id);
> > +    str_list_print_wrap(obj->cmd.names, screen_width(), 8, 8);
> > +    if (obj->ignore)
> > +      printf(" (UNDONE)");
> > +    fputs("\n", stdout);
> > +}
> > +
> > +
> > +/* Print range (or specific if start == end) */
> > +void history_print(int start, int end)
> > +{
> > +    int         i;
> > +    PedHistObj *ii;
> > +
> > +    for (i=0, ii=PedHistMgr.begin;
> > +         ii && i>=start && i<end;
> > +         ii=ii->next, i++)
> > +    {
> > +        history_pr(ii);
> > +    }
> > +}
>
> history_pr could be written inside of history_print. I see no reason
> for the spliting.

Just wanted some flexability in printing, one interface to print a
single 'history' so that a single, all, or range of histories can all
be printed.  But I do not see much utility in printing a range or
single entry, since the only use is to view what history has been
collected (print all should be fine).


>
> > --- /dev/null
> > +++ b/parted/history.h
> > +/* Remember changes
> > + * Note: This is not defined in disk.h
> > + *       This preserves encapsulation between
> > + *       PedCommands and libparted
> > + */
> > +extern void ped_disk_commit_to_history(const PedDisk *disk);
>
> Can you elaborate this comment?
>
> One thing I do think is important here is to have tests for it. It's a
> quite big change and would be good to be sure we don't keep stupid
> bugs on it.

Yep, having automated tests is of course very important.  I just
wanted to provide an initial checkin so that the community know whats
up, and obtaining feed back is good too :-).

Thanks alot!

-Matt




>
> --
>         O T A V I O    S A L V A D O R
> ---------------------------------------------
>  E-mail: otavio at debian.org      UIN: 5906116
>  GNU/Linux User: 239058     GPG ID: 49A5F855
>  Home Page: http://otavio.ossystems.com.br
> ---------------------------------------------
> "Microsoft sells you Windows ... Linux gives
>  you the whole house."
>