Setting location coordinates while editing a dive

[John Van Ostrand]

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

> 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 :-)
>

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.

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