[PATCH] Re: Minor planner bugs

[K. "pestophagous" Heller]

On Tue, Oct 20, 2015 at 1:04 AM, Robert Helling <helling at atdotde.de> wrote:
> Hi,
>
> On 19.10.2015, at 08:11, Martin Měřinský <mermar at centrum.cz> wrote:
>
> Hi.
> I have found a few minor bugs in 4.5.0 planner. I mentioned it in
> "Proposal on new planner layout" thread, but was asked to send it
> separately.
>
> Start Subsurface (no log file).
> Ctrl+l (Log -> Plan dive).
> I get warning "Too many gas mixes".
> Warning text is there twice.
> It doesn't make sense, because I have nothing in Available gases.
>
> Close the warning.
> Hit plus button, to add gas.
> I get warning "Too many gas mixes" again.
>
> When going to plan dive, there is automatically one dive planner point,
> but no gas. I would suppose to have one gas as well.
>
>
> I could not reproduce that behavior. But looking at the code, there might be
> the case that there is a current dive but that that dive does not have any
> cylinder. Here is a patch that is supposed to catch that case and add a
> default cylinder instead. Could you please test that this patch solves this
> problem?

I can provoke the "Too many gas mixes" warning, but for me it doesn't
happen right away.

What I am doing to trigger the "Too many gas mixes" warning is
definitely different than what Martin is doing.  Ideally i would
prefer to reproduce Martin's bug report just as it was originally
reported.  Meanwhile I do not want to "steal" this thread and make it
about my own crazy steps to reproduce. Hopefully some of what follows
will be of use...

I have a partial diagnosis for what is broken when I see "Too many gas mixes".

Perhaps some of my partial diagnosis will end up helping us solve
Martin's case, too.

The steps that led me to "Too many gas mixes" happened while I was
trying to reproduce this other bug:
http://trac.subsurface-divelog.org/ticket/745

When I see "Too many gas mixes", there are two problems:

   1. violated assumptions about 'divedatapoint::time' of 0 having
special meaning.
   2. an apparently unintended cascade of Qt signals/slots leading to
ProfileWidget2::plotDive getting called while "in the middle of
switching dives"

Concretely, you can see which assumptions are violated by looking at
this small commit:

   https://github.com/pestophagous/subsurface/commit/6f2532fa8bf7215bba6250d64e13f16bbb7bb8e5
   (^^ code is clearer than my prose. look. ^^)

All four places in that commit where i added FASSERT (in my local
codebase) show where things are going wrong leading up to the "Too
many gas mixes" warning.  Note the wording i used for one assert in
verify_gas_exists: "totally null cylinder list. displayed_dive seems
problematic."  That matches Robert's hypothesis exactly.

For my path through the code (inspired by the videos by Poltsi at Trac
ticket 745), Robert's patch to
'DivePlannerPointsModel::setupCylinders' does not make any difference.
The reason it makes no difference has to do with the "cascade of Qt
signals/slots" that I mentioned.

When I click "Apply changes", i end up in MainTab::acceptChanges.
>From there, the following (and more) happens:

   clone_dive
   record_dive
   MainWindow::instance()->dive_list()->unselectDives();
   MainWindow::instance()->dive_list()->reload

(MainTab::acceptChanges is a big function, and those four lines are
only a small fraction of what it does.)

The clone_dive and record_dive calls result in the correct dive being
added to the log.

PROBLEM: dive_list()->unselectDives() triggers slots, and makes
ProfileWidget2::plotDive get called, which calls create_dive_from_plan
.... but "displayed_dive" is a totally zeroed-out dive. (it just got
zeroed by clone_dive).

The "Too many gas mixes" warnings (for me) is emitted during this
'plotDive' invocation triggered by dive_list()->unselectDives().  On
the plus side, that shows that the warning about the gas mixes is
totally spurious.  It isn't based on any "real" dive saved in the log.

By the time MainTab::acceptChanges is finished,  a "real" dive ends up
being selected again.  As far as I can see,
'DivePlannerPointsModel::setupCylinders' (with or without the new
patch) does not get called during this MainTab::acceptChanges slot
cascade, unfortunately.

Hope that helps,
K Heller