[parted-devel] History with undo redo

[Matt Davis]

On 6/29/07, Otavio Salvador <otavio at debian.org> wrote:
> "Matt Davis" <mattdavis9 at gmail.com> writes:
>
> > Otavio,
> > I deeply appreciate your suggestions and comments.  I agree with your
> > suggestions as annotated below.
>
> No problem :-) Let's continue with the discussion ;-)
>
> > On 6/28/07, Otavio Salvador <otavio at debian.org> wrote:
> >> 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.
>
> I see the technical reason now but again we need to improve it.
>
> If we're adding it on the library we ought to have it completely there
> or we ought provide just the basis for it and leave to the UI the code
> that uses that.

To be honest, the only use of the struct Command in the history object
is to refer to the name of the command when history is printed.  I can
avoid having to use Command.h by just putting a str_list for the
command's name into the HistoryObject, instead of an entire struct
Command.

>
> >> > --- /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}
>
> Where it uses the Command.h?

Yes I agree, placing the definition of ped_disk_commit_to_history() in
disk.h, define it in disk.c (which makes a call to history.{c,h}).
Further, placing history as a library routine would make this easier.

>
> >> > +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).
>
> I agree and that's why I suggested you to merge it.
>
> >> 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 :-).
>
> Yes, I understand it.
>
> I think that before start to refactory your proposed patch you should
> write the tests to avoid breaking things without notice.
>
> Another thing is that your patch could be splited in a patchset.
>
> I personally see the following patch:
>
>  - history.[ch], Makefile.am: Add history support
>  - commands.c: Add every ran command on the history
>  - parted.c: Uses the new history support to allow do and undo functionality
>
> Probably it can be improved but doing a brief look on it, those
> are easy to identify.
>
> I'm wondering if parted or libparted is the right place to support
> it. I'm starting to think that it should be done on libparted and
> parted itself just use this support.
>
> Doing it on libparted  makes easy to others to use it too and also
> avoid code duplication for them.
>
> Thoughts?

Making this simplest addition useful for other tools is not a bad
idea.    With that said, it might also provide some more additions to
the history api extending usefulness.

I will try to work some more on this, this upcoming week.  And split
it up into separate patch sets.

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