Bugs i found recently

[Dirk Hohndel]

On Tue, 2013-11-05 at 09:33 -0500, Tomaz Canabrava wrote:

> 
>         > Some easy and straight forward ones:
>         > * When pressing "ok" after edited a dive, all the open trips
>         in
>         >   the dive list gets folded. This is quite annoying, because
>         it moves
>         >   the scroll to.
>         
>         
>         This seems to be caused by us calling DiveListView::reload()
>         from
>         MainWindow::refreshDisplay() - doing so loses the 'expanded'
>         state of
>         trips.
>         
>         When playing around with this I found that we also always fell
>         back to
>         sorting by column 0 in descending order. I was able to fix
>         that one, but
>         got lost trying to figure out how to remember the expanded
>         state for the
>         trips.
>         
>         We have a bit for that in our data structures, but when trying
>         to
>         connect to the expanded() signal of the TreeView in order to
>         keep that
>         bit in sync with what's displayed... I still have a lot to
>         learn about
>         Qt programming, I guess :-/
>         
Tomaz - can you help with this one, too? We rebuild the divelist because
editing may have changed things around - I'm actually not 100% sure
that's even necessary, but in the past that has been the way we went
because there were too many odd corner cases.

So we have two directions we could go here...
a) figure out how to maintain the 'expanded' state of trips across the
rebuild
b) figure out if we can avoid recreating the model and just make sure
that it updates correctly

>         > * Merge selected dives only merges two of tree selected. Now
>         when i
>         >   tested with 5 selected dives it merged the first tree of
>         them.
>         
>         
>         Urgs. This was a subtle bug. When looping over the dive list
>         with
>         for_each_dive we increment 'i' in every step - but if two
>         dives get
>         merged, then the indices of all following dives change (we
>         have one
>         fewer dive in the list after all).
>         
>         Playing with that while testing showed that there still are
>         bugs hidden
>         in the dive merging code if you merge dives that shouldn't be
>         merged.
>         E.g. when the dives are not close together (I think we should
>         just
>         refuse to merge those), but there appears to also be an issue
>         with
>         getting the tank pressure in the samples correct. If the
>         second dive is
>         on a fresh tank (which basically means that most likely you
>         shouldn't
>         have merged these two dives, right?) then that isn't correctly
>         reflected
>         in the merged dive.
>         
>         > * Unable to remove pressure information from a cylinder. The
>         old value
>         >   comes back.
>         > * When rescaling the window and the different pains, the
>         >   information-overlay can end up off-screen.
>         
>         
>         Those two sound very much like something that Tomaz should
>         look into :-)
>         
>         
> 
> 
> Fixed. :)

Thank you. Where can I pull the fix? :-)
(not that I'm likely to do that right now - it's 12:30am and I just
arrived in Palau after 24h of travel)

> 
>         > And a couple of harder:
>         > * multiple times i noted that the ui just went bad, and
>         refused to
>         >   switch between dives, or kept showing the divetrip info
>         instead of the
>         >   dive pain. This happened after working with the ui for 10+
>         minutes.
>         
>         
>         That's scary.
>         
>         > * I noted that either via selecting multiple dives or some
>         bug i
>         >   accidentally edited the wrong dives, and wrote over other
>         dives. This
>         >   might be due to ui messing up the selections, or due to
>         other bugs.
>         >   A problem here is that the ui doesn't give you any
>         feedback when you
>         >   are actually editing multiple dives.
>         
>         
>         Even more scary. That's the type of bugs I really want to get
>         rid of
>         before a release. I know this sucks, but can you try and
>         create a
>         sequence of steps that reproduces this? Ideally with the test
>         dives so
>         that the developers can reproduce it? Or you can do this with
>         your dive
>         file if you are welling to share that with us.
>         
>         I stressed Subsurface with my medium sized XML file for a
>         while (I have
>         about 260 dives in it, but the file is large and complex as
>         many of
>         these dives have multiple dive computers) and haven't managed
>         to get
>         things out of sync in a way that corrupted data.
>         
>         But I agree with you that we need visual feedback when editing
>         multiple
>         dives. So I implemented at least that.
> 
> 
> 
> One thing that can help finding those bugs:
> 
> Since we are working with Qt model system, we *must* not select or
> deselect dives manually, but we need to ask the model to desselect it.
> ( this is also true on the Gtk Version, not qt related, but model view
> related. )
> 
> 
> When one part of the code 'select_dive' and that doesn't update the
> Model, the current selection for the user is what's painted on the
> dive list, not what's correctly selected via code, so the user edits
> something thinking that he's editing the *visual selection* whereas
> he's really editing the newly dive created by add dive. :)


> I'v found a couple of places that a 'select_dive' or 'unselect_dive'
> is being used outside of the model control, and this is bad and can
> cause this kind of wrong behavior.
>  
> 

Is there a helper function we should call to make sure things stay in
sync? That's usually the easiest way to ensure we have a consistent view
everywhere.
>         
>         > And one missing feature that we had in the gtk version:
>         > * Ability to add gaschange and bookmark events to a dive.
>         
>         
>         I vaguely remember having started to work at this at some
>         point... but
>         clearly nothing ever came of it. Something for the todo list.
>         

I'm still trying to figure out what happened to the code that I wrote to
implement this...

/D