[parted-devel] History with undo redo

[Otavio Salvador]

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

>> > --- /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?

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

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