Request For Comments - Undo/Redo framework

[Grace Karanja]

Hi Dirk,


On Tue, Feb 10, 2015 at 8:12 AM, Dirk Hohndel <dirk at hohndel.org> wrote:

>
> sorry it took me so long to respond. Today was just crazy...
>
> Thank you for your response.

> Thanks for working on that - it's one of those things that I've wanted for
> a long time and never managed to focus on.
>
> I have a few comments below :-)
>
>
>
> pardon my ignorance... what does the "aboutToShow()" signal do?
> Ahh, I think I got it, see below...
>
> I'm guessing the aboutToShow allows us to mess with the menu just before
> it gets shown? How do we deal with this when hotkeys are used?
>

I think for hot keys  the aboutToShow won't work. I will have to come up
with a workaround.

>
> > +UndoCommand::UndoCommand(QString commandName, dive *affectedDive)
> > +{
> > +     name = commandName;
> > +     stateBefore = affectedDive;
> > +}
>
> I assume the commandName is what you would show as the action to undo /
> redo (haven't seen you show that anywhere, yet, but I'm sure that's still
> coming). But in a way storing a string here is silly, right? You have to
> have special handling for each type of operation. So you should have an
> enum here that lists the actions that you can undo / redo.
>
> enum { DELETE_ACTION, EDIT_ACTION, RENUMBER_ACTION,... } undoTypes;
>
> Then you store that enum and the affected dive(s) (more on that later).
>
> And when you implement the part where you show what the next / previous
> action in the undo/redo buffer are, you can simply have a mapping from
> that enum to a list of translated(!) strings.
>

Thanks. That seems cleaner.


> >                       continue;
> > +             struct dive* undo_entry = alloc_dive();
> > +             copy_dive(get_dive(i), undo_entry);
> > +             MainWindow::instance()->undoBuffer->recordbefore("Delete
> Dive", undo_entry);
>
> So this stores every single dive as an undo. When you delete ten selected
> dives, you'd have ten undo steps. I don't think that's what the user would
> expect to happen...
>
> It's not "wrong", but I don't think it's "right", either.
>
> This is caused by the other problem with the API you created... it only
> ever can handle one dive.
>
> Wouldn't it be nicer to have a something like this?
>
> recordbefore(enum undoType ut, QList<struct dive *> undo_entries)
>

Yes, that would be much nicer.


>
> >  void UndoBuffer::recordAfter(dive *affectedDive)
> >  {
> > -
> > +     list.at(curIdx - 1)->setStateAfter(affectedDive);
> >  }
>
> That deserves a comment, I think. Can you explain what this does? Why the
> -1?
>

Now that I look at it, I think I'll have to change it.

>
> Completely unrelated, as a matter of style I'm not a huge fan of having
> the function body in the .h file - I think we have this in a couple of
> other spots, but I much prefer all the code in the cpp files.
>
Noted

>
> So what's the summary.
>
> First, thanks for working on this. It's a complex piece of code that will
> take some work to get right and I'm very glad you are stepping up to do
> this.
>
> I think there is room to make the API better - it will make the code
> cleaner and the user experience better. But it seems like a step in the
> right direction.
>
> Especially early on in the release cycle (like right now, right after a
> release - in what Linus would call the merge window) I'm willing to take
> code and push it into master even if I have concerns and want stuff
> reworked. I'll assume that you will continue to work on this and will
> consider the changes that I have requested and I'll apply these patches
> and push them out. After all, based on my testing they do in fact
> implement a working undo for deleting dives :-)
> On the flip side, redo() gives me a crash :-(
>


Thanks.


>
> /D
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.subsurface-divelog.org/pipermail/subsurface/attachments/20150210/8d58162a/attachment.html>