Setting location coordinates while editing a dive

[John Van Ostrand]

On Fri, Oct 31, 2014 at 12:34 PM, Dirk Hohndel <dirk at hohndel.org> wrote:

> On Fri, Oct 31, 2014 at 11:39:27AM -0400, John Van Ostrand wrote:
> >
> > Mentioning Undo got me thinking.
> >
> > I've been mucking about trying to have globe.cpp both update the editted
> > dive *and* show the currently edit's placemark planted on the globe and
> the
> > code is uglier than I like. The idea of "undo" has me thinking of ways to
> > streamline that code and remove some oddities in the UI.
>
> He.
>
> > How about this.
> >
> > When editing dives we update the dive on each field change, storing the
> old
> > dive in an undo stack. We can accumulate similar consecutive edits to
> > reduce the number of undos.
>
> OK. That seems simple. Except when editing multiple dives. Then it gets...
> interesting. But I'll go with the idea.
>
> > This could include gas changes, events and even changes to the profile.
>
> Yep, I can see that.
>
> > Manually added dives can be done like this too.
>
> You lost me.
>

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.


>
> > 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.

> 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.

>
> > 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.

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:
        ...
     }
}


> > 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.


> > 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.


> > 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;
};


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.


> > 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.


> All this is post 4.3, though.
>

I'd agree.


-- 
John Van Ostrand
At large on sabbatical
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.subsurface-divelog.org/pipermail/subsurface/attachments/20141031/7bc3572c/attachment.html>