Request For Comments - Undo/Redo framework
Dirk Hohndel
dirk at hohndel.org
Mon Feb 9 21:12:01 PST 2015
Hi Grace,
sorry it took me so long to respond. Today was just crazy...
On Mon, Feb 09, 2015 at 01:11:41PM +0300, Grace Karanja wrote:
>
> I've been working on implementing an undo/redo framework. The
> attached patches are what I've done so far. I only have undo working
> for the deleted dives.
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.
> This is not in anyway fully working, just a view of what I have done so that
> you can point me in the right direction
I have a few comments below :-)
> Subject: [PATCH 1/4] Create UndoBuffer class
>
> Add an empty UndoBuffer class. This will be built up on to
> implement a working undo/redo mechanism.
This looked pretty good. I have a few concerns about the API that I'll
explain later...
> Subject: [PATCH 2/4] Add menu entries for undo/redo
>
> Add an edit menu with undo and redo submenus, and connect them to
> the UndoBuffer class. The submenus are only enabled when needed.
>
> Signed-off-by: Grace Karanja <gracie.karanja89 at gmail.com>
>
> diff --git a/qt-ui/mainwindow.cpp b/qt-ui/mainwindow.cpp
> index 3142f5a..57af5e0 100644
> --- a/qt-ui/mainwindow.cpp
> +++ b/qt-ui/mainwindow.cpp
> @@ -77,6 +78,7 @@ MainWindow::MainWindow() : QMainWindow(),
> connect(DivePlannerPointsModel::instance(), SIGNAL(planCreated()), this, SLOT(planCreated()));
> connect(DivePlannerPointsModel::instance(), SIGNAL(planCanceled()), this, SLOT(planCanceled()));
> connect(ui.printPlan, SIGNAL(pressed()), ui.divePlannerWidget, SLOT(printDecoPlan()));
> + connect(ui.menu_Edit, SIGNAL(aboutToShow()), this, SLOT(checkForUndoAndRedo()));
pardon my ignorance... what does the "aboutToShow()" signal do?
Ahh, I think I got it, see below...
> +
> +void MainWindow::on_action_Undo_triggered()
> +{
> + undoBuffer->undo();
> +}
> +
> +void MainWindow::on_action_Redo_triggered()
> +{
> + undoBuffer->redo();
> +}
> +
> +void MainWindow::checkForUndoAndRedo()
> +{
> + ui.action_Undo->setEnabled(undoBuffer->canUndo());
> + ui.action_Redo->setEnabled(undoBuffer->canRedo());
> +}
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?
> diff --git a/qt-ui/mainwindow.ui b/qt-ui/mainwindow.ui
> index 8ffb8bb..9507b5f 100644
> --- a/qt-ui/mainwindow.ui
> +++ b/qt-ui/mainwindow.ui
> @@ -813,6 +821,22 @@ p, li { white-space: pre-wrap; }
> <string>User &survey</string>
> </property>
> </action>
> + <action name="action_Undo">
> + <property name="text">
> + <string>&Undo</string>
> + </property>
> + <property name="shortcut">
> + <string>Ctrl+Z</string>
> + </property>
> + </action>
> + <action name="action_Redo">
> + <property name="text">
> + <string>&Redo</string>
> + </property>
> + <property name="shortcut">
> + <string>Ctrl+Shift+Z</string>
> + </property>
> + </action>
Ctrl-Z and Ctrl-Shift-Z seem like a good choice.
> Subject: [PATCH 3/4] Add UndoCommand class
>
> Add a class to handle all undo/redo events. Whenever a user
> action affects a dive, an undo command will be created. A list of
> these commands will be stored in the UndoBuffer, to allow for
> moving forwards/backwards in the list.
>
> Signed-off-by: Grace Karanja <gracie.karanja89 at gmail.com>
>
> diff --git a/qt-ui/undobuffer.cpp b/qt-ui/undobuffer.cpp
> index 92c0dee..b0c1d26 100644
> --- a/qt-ui/undobuffer.cpp
> +++ b/qt-ui/undobuffer.cpp
> @@ -40,3 +40,21 @@ void UndoBuffer::recordAfter(dive *affectedDive)
>
> }
>
> +
> +
> +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.
> diff --git a/qt-ui/divelistview.cpp b/qt-ui/divelistview.cpp
> index ec54af9..89b851a 100644
> --- a/qt-ui/divelistview.cpp
> +++ b/qt-ui/divelistview.cpp
> @@ -730,6 +731,9 @@ void DiveListView::deleteDive()
> for_each_dive (i, d) {
> if (!d->selected)
> 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)
> diff --git a/qt-ui/undobuffer.cpp b/qt-ui/undobuffer.cpp
> index b0c1d26..4ee0cf6 100644
> --- a/qt-ui/undobuffer.cpp
> +++ b/qt-ui/undobuffer.cpp
> @@ -1,8 +1,9 @@
> #include "undobuffer.h"
> +#include "mainwindow.h"
>
> UndoBuffer::UndoBuffer(QObject *parent) : QObject(parent)
> {
> -
> + curIdx = 0;
> }
>
> UndoBuffer::~UndoBuffer()
> @@ -12,32 +13,46 @@ UndoBuffer::~UndoBuffer()
>
> bool UndoBuffer::canUndo()
> {
> -
> + return curIdx > 0;
> }
Do you really want an unbounded growing undo buffer? Or wouldn't this be
better as some kind of ring buffer of fixed length?
> bool UndoBuffer::canRedo()
> {
> -
> + return curIdx < list.count();
> }
>
> void UndoBuffer::redo()
> {
> -
> + current()->redo();
> + curIdx++;
> + if (curIdx > list.count())
> + curIdx = list.count() - 1;
Is that right? You test for curIdx < list.count() when you check
canRedo()...
> 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?
> @@ -50,11 +65,14 @@ UndoCommand::UndoCommand(QString commandName, dive *affectedDive)
>
> void UndoCommand::undo()
> {
> -
> + if (name == "Delete Dive") {
> + record_dive(stateBefore);
> + MainWindow::instance()->recreateDiveList();
> + }
> }
>
> void UndoCommand::redo()
> {
> -
> + //To be implemented
> }
>
> diff --git a/qt-ui/undobuffer.h b/qt-ui/undobuffer.h
> index beae8e3..9fac147 100644
> --- a/qt-ui/undobuffer.h
> +++ b/qt-ui/undobuffer.h
> @@ -12,7 +12,7 @@ private:
>
> public:
> explicit UndoCommand(QString commandName, dive* affectedDive);
> - void setStateAfter(dive* affectedDive);
> + void setStateAfter(dive* affectedDive) { stateAfter = affectedDive; }
Hmm, so you create a new object, each with their own overhead, for each
individual undo step.
Again, I think it would be much better if you had lists of dives affected
and had the undo/redo functions walk those lists...
So that would turn your data structure into a ring buffer that contains
pairs of undoType and QList<struct dive *>
And then you would have different undo functions for each undoType and
call those functions with the list of dives.
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.
> void undo();
> void redo();
> };
> @@ -25,7 +25,9 @@ public:
> ~UndoBuffer();
> bool canUndo();
> bool canRedo();
> + UndoCommand *current() const { return list.at(curIdx - 1); }
ditto.
Actually, double ditto: a) I don't like this in the header and
b) I don't understand the '-1'. I thought curIdx is an index into the
list. So "-1" is an invalid number, 0 is the first entry, etc. So
returning list.at(curIdx -1) seems... odd.
If I run this code and delete a dive, undo that operation and try to redo
the operation (I realize, that's not implemented, but just calling it...)
curIdx is 0 and trying to access list.at(-1) makes things crash.
This needs some work :-)
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 :-(
/D
More information about the subsurface
mailing list