<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Oct 31, 2014 at 5:43 PM, 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Fri, Oct 31, 2014 at 04:36:29PM -0400, John Van Ostrand wrote:<br>
><br>
> In other words when manually adding a dive an empty dive would be added to<br>
> the dive list with fields added to the dive in as they are entered in the<br>
> form.<br>
<br>
</span>Hmm. I find that somewhat awkward. But I'm not categorically rejecting it.<br>
<span class=""><br>
> > > This would have a few benefits<br>
> > ><br>
> > > 1. We eliminate the "This dive is being edited" indicator and all the<br>
> > code<br>
> > > referencing "isEditing()".<br>
> ><br>
> > So this is easy to write in text, but once you deal with the widgets it<br>
> > gets a little tricky.<br>
> ><br>
> > Example. You edit something (let's say a tag), your cursor is still in<br>
> > that field, and then click on a different dive in the dive list. What<br>
> > happens? You have a drop down open (date, cylinder type) and you change to<br>
> > a different dive...<br>
> ><br>
> > What I'm trying to say is that the definition of "I'm editing something"<br>
> > is very intuitive, until you try to implement the finer details.<br>
><br>
> I figured we could use a change signal of some kind so we can react to<br>
> changes. It looks like some widgets are already enabled. Others may need<br>
> some modification.<br>
<br>
</span>That's part of the 'how'. But the question in many instances is "when do<br>
you issue that signal". E.g., when changing a text field, do you issue<br>
this with every key press? If not, then when DO you do it? We have learned<br>
the hard way that it's not always simple to figure out when focus has<br>
moved...<br>
<span class=""><br>
> > 2. We don't have anything funny to do with placemarks on the map.<br>
> > > 3. Oops, like accidental double clicks can be backed out.<br>
> > > 4. Eliminate non-intuitive location/placemark functionality.<br>
> > > 5. Any for_each_dive will automatically include the dive being edited so<br>
> > UI<br>
> > > updates like placemarks don't need special cases.<br>
> ><br>
> > That last one doesn't make sense to me. Can you elaborate.<br>
> ><br>
><br>
> The repopulateLabels function in globe.cpp uses the dive list to create<br>
> flags. It would be cleaner if the data being editing was being updated into<br>
> the dive list as each field was changed. That way functions like<br>
> repopulateLabels wouldn't have to retrieve data from the form to place a<br>
> flag.<br>
<br>
</span>Ah, OK. That's what I suggested with adding the displayed_dive there.<br>
But please DO NOT break the logic that the displayed_dive is distinct and<br>
different from the corresponding dive on the divelist...<br>
<span class=""><br>
> > > There would be a few challenges:<br>
> > ><br>
> > > 1. Single dive edits would be easy but multi-dive edits would more<br>
> > > complicated. They should be associated as one undo.<br>
> ><br>
> > OK<br>
> ><br>
> > > 2. Trips would be another undo case.<br>
> ><br>
> > What does that mean? You mean trip changes to the divelist?<br>
> > Uhhh, I want to see the data structure in which you do that :-)<br>
><br>
> We may want to undo a trip grouping or undo edits to a trip. We need to set<br>
> up different types of undos. A TRIP_GROUP undo might be one type and<br>
> TRIP_EDIT undo might be another. The undo stack would have a struct that<br>
> contains a type and a pointer to a blob of data, and undo action would send<br>
> that blob to the function that handles that specific undo type. The blob<br>
> would be used by that function to perform the undo. This provides a<br>
> framework for allowing a wide range of different undos.<br>
<br>
</span>Wow. Oops. Err. Yeah. That sounds easy...<br>
<div><div class="h5"><br>
> A example might be this (imagine expanding struct undo_dive_edit to hold a<br>
> list of dives.)<br>
><br>
> struct undo_item {<br>
> enum undo_type type;<br>
> void *data;<br>
> };<br>
><br>
> struct undo_dive_edit {<br>
> int divenr;<br>
> struct dive *dive;<br>
> };<br>
><br>
> struct undo_item undo_stack[16];<br>
> int undo_stack_head = 0;<br>
> int undo_stack_tail = 0;<br>
><br>
> // called when date field is updated<br>
> void MainTab::updateDate(const QDateTime &)<br>
> {<br>
> undo_dive_create(); // create and undo for this change<br>
> <update dive in dive list with new date><br>
> }<br>
><br>
><br>
> void undo_dive_edit_create()<br>
> {<br>
> struct undo_item undo = undo_get_current(); // Get most recent undo<br>
> struct undo_dive_edit *ude = undo->undo_data;<br>
><br>
> // Only create an undo if this change affects a different set of dives<br>
> than the current undo.<br>
> if (undo.type != UNDO_DIVE_EDIT || ude->dive == selected_dive) {<br>
> // Create undo item.<br>
> struct undo_dive_edit *new_undo = malloc(sizeof(struct<br>
> undo_dive_edit));<br>
> new_undo->divenr = selected_dive;<br>
> new_undo->dive = <copy of dive structure><br>
> undo_push(UNDO_DIVE_EDIT, (void *) new_undo);<br>
> }<br>
> }<br>
><br>
><br>
> // Dive Edit Undo<br>
> void undo_dive_edit(void *undo_data)<br>
> {<br>
> struct undo_dive_edit *undo = undo->undo_data;<br>
><br>
> <copy saved dive over current version><br>
> }<br>
><br>
><br>
> void undo_pop()<br>
> {<br>
><br>
> if (undo_stack_head == undo_stack_tail)<br>
> return; // nothing to undo<br>
><br>
> struct undo_item undo = undo_stack[undo_stack_head--];<br>
><br>
> switch (undo.type) {<br>
> case UNDO_DIVE_EDIT:<br>
> undo_dive_edit(undo->data);<br>
> break;<br>
> case UNDO_DIVE_DELETE:<br>
> ...<br>
> }<br>
> }<br>
<br>
</div></div>That looks like an interesting start. And hellishly fragile.<br>
I'd be very careful to make sure we understand how to create atomic<br>
transactions here - i.e. how to make sure that we can move forward and<br>
backward without breaking our existing data structures.<br>
<span class=""><br>
> > > 3. We'd still have the issue of accidental edits, like an accidental<br>
> > > double-click on the globe.<br>
> > > 4. Other cases: Deletes, renumber, photo add/delete, imports, copy and<br>
> > > paste, undos (i.e. redo)<br>
> ><br>
> > Several of those would be crazy complicated to keep undo information<br>
> > about. Renumbers for example. We need to draw the line somewhere.<br>
> ><br>
><br>
> We would have to flush the undo stack if we encountered something, like a<br>
> renumber, that can't be undone.<br>
<br>
</span>Yes.<br>
<span class=""><br>
> > > 5. Future changes will create more cases.<br>
> > > 6. Edited dives may appear incomplete in the dive list.<br>
> ><br>
> > What do you mean here?<br>
><br>
> If we keep with the idea of edits directly affecting the dive list then<br>
> additions should directly affect the dive list too. It would start by<br>
> adding an empty dive to the list which might break some code that isn't<br>
> expecting that.<br>
<br>
</span>So you would end the separation of current_dive and displayed_dive (which<br>
is what I asked a few paragraphs earlier that you don't do). That's MAJOR<br>
surgery.<br>
<span class=""><br>
> > > To implement:<br>
> > ><br>
> > > 1. Create a structure for an undo that supports the known cases. Each<br>
> > > struct is an atomic undo, an array of them is the undo stack. Maybe a<br>
> > > complementary redo stack can be done too.<br>
> ><br>
> > OK. See above. I'd like to see the data structures that you have in mind<br>
> > here.<br>
> ><br>
> > > 2. Determine what actions should accumulate into an atomic undo. (e.g.<br>
> > > per-field undo or entire dive undo.)<br>
> ><br>
> > I'd go with a dive undo. But once again - what does that mean in the<br>
> > context of multi dive edits?<br>
> ><br>
><br>
> The undo would allow a group of dives to be part of one atomic unit. Think<br>
> of it as copying all of the dives affected into an array and putting that<br>
> onto the undo stack. An undo pops them off the stack, sends them to a<br>
> "undo_dive_edit(void *blob)" function which typecasts the blob to the<br>
> structure the function expects and copies the dives back to the list. Maybe<br>
> a struct like this<br>
><br>
> struct undo_dive_edit {<br>
> int dive_count;<br>
> int *divenr;<br>
> struct **dive dives;<br>
> };<br>
<br>
</span>Oww, Oww, Oww. Yeah, that will be fun.<br>
But fundamentally it's doable, I agree.<br>
<span class=""><br>
> It could also copy the undone dives to an array and add them to a redo<br>
> stack.<br>
><br>
> ><br>
> > > 2. Create an undo function for each case.<br>
> > > 3. Alter edits to directly change dive_list.<br>
> ><br>
> > Please explain more.<br>
> ><br>
><br>
> As I mentioned above have each widget issue a "changed" signal so when each<br>
> field is changed in the form the dive in the dive list is modified right<br>
> away. That modification triggers an undo stack push if needed. If the undo<br>
> holds the full dive structure then only one undo needs to be pushed even if<br>
> several fields are changed.<br>
<br>
</span>See above. That's a massive change of the innermost inards of our data<br>
structures and of the way the UI interacts with our data. Yikes.<br>
<br>
How long did you say your sabbatical was?<br>
<span class=""><br>
> > > I've never done something like this before so I'm not sure if there are<br>
> > > some really sneaky details lurking out of sight.<br>
> ><br>
> > I promise, there are a MOUNTAIN of snarky details... But I like the basic<br>
> > idea. Let's start drilling down on some details and see where we get.<br>
> ><br>
> ><br>
> That's what I'm afraid of. I think it's a big undertaking.<br>
<br>
</span>Try MASSIVE and HUGE.<br>
<span class=""><br>
> > All this is post 4.3, though.<br>
><br>
> I'd agree.<br>
<br>
</span>Good :-)<span class="HOEnZb"><font color="#888888"><br></font></span></blockquote><div><br></div><div>Okay, you've convinced me. This is too big for me right now. I'll fiddle with smaller bits until I get a much better understanding of the code. <br></div></div><br>-- <br><div class="gmail_signature"><div dir="ltr"><div>John Van Ostrand<br></div><div>At large on sabbatical<br></div><br></div></div>
</div></div>