Setting location coordinates while editing a dive

Dirk Hohndel dirk at hohndel.org
Fri Oct 31 14:43:30 PDT 2014


On Fri, Oct 31, 2014 at 04:36:29PM -0400, John Van Ostrand wrote:
> 
> In other words when manually adding a dive an empty dive would be added to
> the dive list with fields added to the dive in as they are entered in the
> form.

Hmm. I find that somewhat awkward. But I'm not categorically rejecting it.

> > > This would have a few benefits
> > >
> > > 1. We eliminate the "This dive is being edited" indicator and all the
> > code
> > > referencing "isEditing()".
> >
> > So this is easy to write in text, but once you deal with the widgets it
> > gets a little tricky.
> >
> > Example. You edit something (let's say a tag), your cursor is still in
> > that field, and then click on a different dive in the dive list. What
> > happens? You have a drop down open (date, cylinder type) and you change to
> > a different dive...
> >
> > What I'm trying to say is that the definition of "I'm editing something"
> > is very intuitive, until you try to implement the finer details.
> 
> I figured we could use a change signal of some kind so we can react to
> changes. It looks like some widgets are already enabled. Others may need
> some modification.

That's part of the 'how'. But the question in many instances is "when do
you issue that signal". E.g., when changing a text field, do you issue
this with every key press? If not, then when DO you do it? We have learned
the hard way that it's not always simple to figure out when focus has
moved...

> > 2. We don't have anything funny to do with placemarks on the map.
> > > 3. Oops, like accidental double clicks can be backed out.
> > > 4. Eliminate non-intuitive location/placemark functionality.
> > > 5. Any for_each_dive will automatically include the dive being edited so
> > UI
> > > updates like placemarks don't need special cases.
> >
> > That last one doesn't make sense to me. Can you elaborate.
> >
> 
> The repopulateLabels function in globe.cpp uses the dive list to create
> flags. It would be cleaner if the data being editing was being updated into
> the dive list as each field was changed. That way functions like
> repopulateLabels wouldn't have to retrieve data from the form to place a
> flag.

Ah, OK. That's what I suggested with adding the displayed_dive there.
But please DO NOT break the logic that the displayed_dive is distinct and
different from the corresponding dive on the divelist...

> > > There would be a few challenges:
> > >
> > > 1. Single dive edits would be easy but multi-dive edits would more
> > > complicated. They should be associated as one undo.
> >
> > OK
> >
> > > 2. Trips would be another undo case.
> >
> > What does that mean? You mean trip changes to the divelist?
> > Uhhh, I want to see the data structure in which you do that :-)
> 
> We may want to undo a trip grouping or undo edits to a trip. We need to set
> up different types of undos. A TRIP_GROUP undo might be one type and
> TRIP_EDIT undo might be another. The undo stack would have a struct that
> contains a type and a pointer to a blob of data, and undo action would send
> that blob to the function that handles that specific undo type. The blob
> would be used by that function to perform the undo. This provides a
> framework for allowing a wide range of different undos.

Wow. Oops. Err. Yeah. That sounds easy...

> A example might be this (imagine expanding struct undo_dive_edit to hold a
> list of dives.)
> 
> struct undo_item {
>     enum undo_type type;
>     void *data;
> };
> 
> struct undo_dive_edit {
>    int divenr;
>    struct dive *dive;
> };
> 
> struct undo_item undo_stack[16];
> int undo_stack_head = 0;
> int undo_stack_tail = 0;
> 
> // called when date field is updated
> void MainTab::updateDate(const QDateTime &)
> {
>      undo_dive_create();   // create and undo for this change
>      <update dive in dive list with new date>
> }
> 
> 
> void undo_dive_edit_create()
> {
>      struct undo_item undo = undo_get_current();   // Get most recent undo
>      struct undo_dive_edit *ude = undo->undo_data;
> 
>      // Only create an undo if this change affects a different set of dives
> than the current undo.
>      if (undo.type != UNDO_DIVE_EDIT || ude->dive == selected_dive)  {
>          // Create undo item.
>          struct undo_dive_edit *new_undo = malloc(sizeof(struct
> undo_dive_edit));
>          new_undo->divenr = selected_dive;
>          new_undo->dive = <copy of dive structure>
>          undo_push(UNDO_DIVE_EDIT, (void *) new_undo);
>      }
> }
> 
> 
> // Dive Edit Undo
> void undo_dive_edit(void *undo_data)
> {
>      struct undo_dive_edit *undo = undo->undo_data;
> 
>      <copy saved dive over current version>
> }
> 
> 
> void undo_pop()
> {
> 
>      if (undo_stack_head == undo_stack_tail)
>          return; // nothing to undo
> 
>      struct undo_item undo = undo_stack[undo_stack_head--];
> 
>      switch (undo.type) {
>      case UNDO_DIVE_EDIT:
>         undo_dive_edit(undo->data);
>         break;
>      case UNDO_DIVE_DELETE:
>         ...
>      }
> }

That looks like an interesting start. And hellishly fragile.
I'd be very careful to make sure we understand how to create atomic
transactions here - i.e. how to make sure that we can move forward and
backward without breaking our existing data structures.

> > > 3. We'd still have the issue of accidental edits, like an accidental
> > > double-click on the globe.
> > > 4. Other cases: Deletes, renumber, photo add/delete, imports, copy and
> > > paste, undos (i.e. redo)
> >
> > Several of those would be crazy complicated to keep undo information
> > about. Renumbers for example. We need to draw the line somewhere.
> >
> 
> We would have to flush the undo stack if we encountered something, like a
> renumber, that can't be undone.

Yes.

> > > 5. Future changes will create more cases.
> > > 6. Edited dives may appear incomplete in the dive list.
> >
> > What do you mean here?
> 
> If we keep with the idea of edits directly affecting the dive list then
> additions should directly affect the dive list too. It would start by
> adding an empty dive to the list which might break some code that isn't
> expecting that.

So you would end the separation of current_dive and displayed_dive (which
is what I asked a few paragraphs earlier that you don't do). That's MAJOR
surgery.

> > > To implement:
> > >
> > > 1. Create a structure for an undo that supports the known cases. Each
> > > struct is an atomic undo, an array of them is the undo stack. Maybe a
> > > complementary redo stack can be done too.
> >
> > OK. See above. I'd like to see the data structures that you have in mind
> > here.
> >
> > > 2. Determine what actions should accumulate into an atomic undo. (e.g.
> > > per-field undo or entire dive undo.)
> >
> > I'd go with a dive undo. But once again - what does that mean in the
> > context of multi dive edits?
> >
> 
> The undo would allow a group of dives to be part of one atomic unit. Think
> of it as copying all of the dives affected into an array and putting that
> onto the undo stack. An undo pops them off the stack, sends them to a
> "undo_dive_edit(void *blob)" function which typecasts the blob to the
> structure the function expects and copies the dives back to the list. Maybe
> a struct like this
> 
> struct undo_dive_edit {
>     int dive_count;
>     int *divenr;
>     struct **dive dives;
> };

Oww, Oww, Oww. Yeah, that will be fun.
But fundamentally it's doable, I agree.

> It could also copy the undone dives to an array and add them to a redo
> stack.
> 
> >
> > > 2. Create an undo function for each case.
> > > 3. Alter edits to directly change dive_list.
> >
> > Please explain more.
> >
> 
> As I mentioned above have each widget issue a "changed" signal so when each
> field is changed in the form the dive in the dive list is modified right
> away. That modification triggers an undo stack push if needed. If the undo
> holds the full dive structure then only one undo needs to be pushed even if
> several fields are changed.

See above. That's a massive change of the innermost inards of our data
structures and of the way the UI interacts with our data. Yikes.

How long did you say your sabbatical was?

> > > I've never done something like this before so I'm not sure if there are
> > > some really sneaky details lurking out of sight.
> >
> > I promise, there are a MOUNTAIN of snarky details... But I like the basic
> > idea. Let's start drilling down on some details and see where we get.
> >
> >
> That's what I'm afraid of. I think it's a big undertaking.

Try MASSIVE and HUGE.

> > All this is post 4.3, though.
> 
> I'd agree.

Good :-)

/D


More information about the subsurface mailing list