Few bugs and a feature req from testing Subsurface

[Dirk Hohndel]

On Sat, 2014-03-15 at 10:46 -0700, Linus Torvalds wrote:
> There are two bugs that shows in subsurface:
> 
>  - the "default cylinder" behavior is broken, broken, broken. You have
> set your default cylinder to AL80 (which is actually the default if
> nothing has been set manually), and at least one of your dive
> computers supports two cylinders, so EVEN THOUGH THERE IS NO CYLINDER
> DATA, subsurface will generate these completely broken cylinder
> entries in the XML:
> 
>   <cylinder size='11.094 l' workpressure='206.843 bar'
> description='AL80' o2='32.0%' />
>   <cylinder size='11.094 l' workpressure='206.843 bar' description='AL80' />
>   <cylinder size='11.094 l' workpressure='206.843 bar' description='AL80' />
> 
>    where the two last ones are just pure garbage.
> 
>    The f*cking "default cylinder" crap needs to die, die, die. It's
> wrong.

Tell me how you really feel. Or better, don't :-)

>  I have that in my own files too from more recent dives, with
> some of my files having this:
> 
>   <cylinder size='12.0 l' workpressure='200.0 bar' description='12L 200 bar' />
>   <cylinder size='11.094 l' workpressure='206.843 bar' description='AL80' />
>   <cylinder size='11.094 l' workpressure='206.843 bar' description='AL80' />
>   <cylinder size='11.094 l' workpressure='206.843 bar' description='AL80' />
>   <cylinder size='11.094 l' workpressure='206.843 bar' description='AL80' />
>   <cylinder size='11.094 l' workpressure='206.843 bar' description='AL80' />
>   <cylinder size='11.094 l' workpressure='206.843 bar' description='AL80' />
>   <cylinder size='11.094 l' workpressure='206.843 bar' description='AL80' />
> 
> because the Suunto HelO2 supports up to eight cylinders. But 7 of
> those cyliners are just crap.
> 
> Dirk, this is all your fault. It goes back to commit 904c20feef90 and
> friends, as far as I can tell. It's broken. It's wrong. It's annoying.
> At *most*, the default cylinder should affect the first cylinder only.
> Or that thing should just be deleted, because it's worse than not
> having a default cylinder at all.

I accept the blame. I still think that the underlying idea isn't
completely wrong, but I agree that the way I implemented it has more
unintended and annoying side effects than the value intended.

I still think that the underlying idea has value, so let's talk about
how it should work to be useful and not annoying.

Most recreational divers dive with the same one cylinder most of the
time. Either because they own the cylinder (I have a TON of dives with
my 119), or because they usually dive on dive boars (and usually get the
darned AL80). So since almost all dive computers don't give you no
information about the cylinder you used (the exceptions, IIRC, are the
Uemis which gives you cylinder data (of course that data is bogus and
broken for imperial cylinder sizes), and the Atomics Aquatics Cobalt. I
implemented that for both of those computers, I don't think any other
support is available), it might be useful to offer a default cylinder
for the users who want that.

So here's what I'm proposing to fix the current implementation

a) if no default cylinder is set, don't just use AL80. Disable the
default cylinder logic

b) if a default cylinder is set, ONLY use it for the first cylinder of a
DC

Linus, will this address your issues?

>  - more seriously, the cylinder equipment editing is odd. When I load
> your XML file, I see only two cylinders (probably because the third
> one is empty and not mentioned by any dive computer), but when I try
> to delete the second bogus one, it *works*, but the equipment list
> goes empty. I know it works because if I save the file, it gets saved
> with one fewer cylinder entries. So this is a GUI error.
> Tomaz, mind looking at that one? It's a pretty serious GUI error. Just
> load Miika's xml file and try to delete the second cylinder.

Tomaz is out and quite sick (I'm sure you saw his brief response).

We have a few other people with solid Qt experience - would one of you
mind looking into this issue?

Boris? 

> See what happened?
> 
> So no, we do *not* generate gas change event when diving on air, and
> your computers never generated any at all, it's subsurface that just
> reminds itself "this dive computer is associated with cylinder 1, not
> cylinder 0". And not starting on cylinder 0 is considered a gas
> change.
> 
> In other words, "gas change" is a bit of a misnomer. It's a "cylinder
> index change". It's just that we call it gas change, because that's
> what it means in normal situations.
> 
> Anyway, what we probably *should* do (after the two bugs above get
> sorted out) is likely to say that when you delete a cylinder, you
> delete all gas change events associated with it too. That would give
> us a way to fix up the above issue by the user just saying "I really
> only had the first cylinder, so delete that bogus second cylinder",
> and everything would work out right. But right now, deleting that
> second cylinder has bigger problems than "the gas change event
> remains".

I like this. When I understood what caused the odd situation Miika was
seeing I was wondering what to do about it after the fact. This seems
like a clean solution.

/D