Hi Dirk,
On Tue, Feb 10, 2015 at 8:12 AM, Dirk Hohndel <[email protected]> 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 >
_______________________________________________ subsurface mailing list [email protected] http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
