<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">Hi Dirk,</div><div class="gmail_quote"><br></div><div class="gmail_quote"><br></div><div class="gmail_quote">On Tue, Feb 10, 2015 at 8:12 AM, Dirk Hohndel <span dir="ltr"><<a href="mailto:dirk@hohndel.org" target="_blank">dirk@hohndel.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><br>
sorry it took me so long to respond. Today was just crazy...<span><br>
<br></span></blockquote><div>Thank you for your response. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span>
</span>Thanks for working on that - it's one of those things that I've wanted for<br>
a long time and never managed to focus on.<br>
<span><br></span>I have a few comments below :-)<br>
<br><br>
<br>
pardon my ignorance... what does the "aboutToShow()" signal do?<br>
Ahh, I think I got it, see below...<br>
<br>I'm guessing the aboutToShow allows us to mess with the menu just before<br>
it gets shown? How do we deal with this when hotkeys are used?<br></blockquote><div> </div><div>I think for hot keys the aboutToShow won't work. I will have to come up with a workaround.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><br>
> +UndoCommand::UndoCommand(QString commandName, dive *affectedDive)<br>
> +{<br>
> + name = commandName;<br>
> + stateBefore = affectedDive;<br>
> +}<br>
<br>
I assume the commandName is what you would show as the action to undo /<br>
redo (haven't seen you show that anywhere, yet, but I'm sure that's still<br>
coming). But in a way storing a string here is silly, right? You have to<br>
have special handling for each type of operation. So you should have an<br>
enum here that lists the actions that you can undo / redo.<br>
<br>
enum { DELETE_ACTION, EDIT_ACTION, RENUMBER_ACTION,... } undoTypes;<br>
<br>
Then you store that enum and the affected dive(s) (more on that later).<br>
<br>
And when you implement the part where you show what the next / previous<br>
action in the undo/redo buffer are, you can simply have a mapping from<br>
that enum to a list of translated(!) strings.<br></blockquote><div><br></div><div>Thanks. That seems cleaner.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
> continue;<br>
> + struct dive* undo_entry = alloc_dive();<br>
> + copy_dive(get_dive(i), undo_entry);<br>
> + MainWindow::instance()->undoBuffer->recordbefore("Delete Dive", undo_entry);<br>
<br>
So this stores every single dive as an undo. When you delete ten selected<br>
dives, you'd have ten undo steps. I don't think that's what the user would<br>
expect to happen...<br>
<br>
It's not "wrong", but I don't think it's "right", either.<br>
<br>
This is caused by the other problem with the API you created... it only<br>
ever can handle one dive.<br>
<br>
Wouldn't it be nicer to have a something like this?<br>
<br>
recordbefore(enum undoType ut, QList<struct dive *> undo_entries)<br></blockquote><div><br></div><div>Yes, that would be much nicer.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
> void UndoBuffer::recordAfter(dive *affectedDive)<br>
> {<br>
> -<br>
> + <a href="http://list.at" target="_blank">list.at</a>(curIdx - 1)->setStateAfter(affectedDive);<br>
> }<br>
<br>
That deserves a comment, I think. Can you explain what this does? Why the -1?<br></blockquote><div><br></div><div>Now that I look at it, I think I'll have to change it. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>Completely unrelated, as a matter of style I'm not a huge fan of having<br>
the function body in the .h file - I think we have this in a couple of<br>
other spots, but I much prefer all the code in the cpp files.<br></blockquote><div>Noted </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>So what's the summary.<br>
<br>
First, thanks for working on this. It's a complex piece of code that will<br>
take some work to get right and I'm very glad you are stepping up to do<br>
this.<br>
<br>
I think there is room to make the API better - it will make the code<br>
cleaner and the user experience better. But it seems like a step in the<br>
right direction.<br>
<br>
Especially early on in the release cycle (like right now, right after a<br>
release - in what Linus would call the merge window) I'm willing to take<br>
code and push it into master even if I have concerns and want stuff<br>
reworked. I'll assume that you will continue to work on this and will<br>
consider the changes that I have requested and I'll apply these patches<br>
and push them out. After all, based on my testing they do in fact<br>
implement a working undo for deleting dives :-)<br>
On the flip side, redo() gives me a crash :-(<br></blockquote><div><br></div><div><br></div><div>Thanks.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span><font color="#888888"><br>
/D<br>
</font></span></blockquote></div><br></div></div>